Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Hans de Goede <hdegoede@redhat.com>]

Hi Ilpo,

Thank you for reviewing this.

On 7/11/24 10:41 AM, Ilpo Järvinen wrote:
> On Sun, 7 Jul 2024, Carlos Ferreira wrote:
> 
>> This driver adds supports for 4 zone keyboard rgb on omen laptops
>> and maps the wmi backlight toggle event to KEY_KBDILLUMTOGGLE.
>> For the backlight, it uses the multicolor led api.
>>
>> Tested on the HP Omen 15-en1001np.
>>
>> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
>> ---
>> Changes in v3:
>>  - Moved to the multicolor led api
>>  - Mapped the wmi backlight toggle event to KEY_KBDILLUMTOGGLE
>>  - Some other minor changes
>> Changes in v2:
>>  - Rearranged code to remove forward declarations
>>  - Changed from sprintf() to sysfs_emit()
>>  - Fixed some identation and coding style problems
>>  - Switched from manual bit manipulation to GENMASK(x, y) + FIELD_PREP(XX, )
>>  - #define'ed magic constants
>> ---
>>  drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
>>  1 file changed, 239 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>> index 5fa553023842..5eae47961f76 100644
>> --- a/drivers/platform/x86/hp/hp-wmi.c
>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>> @@ -14,6 +14,8 @@
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>>  #include <linux/kernel.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/leds.h>
>>  #include <linux/module.h>
>>  #include <linux/init.h>
>>  #include <linux/slab.h>
>> @@ -24,6 +26,7 @@
>>  #include <linux/platform_profile.h>
>>  #include <linux/hwmon.h>
>>  #include <linux/acpi.h>
>> +#include <linux/bits.h>
> 
> You need to add #include <linux/bitfield.h> as LKP found out. The 
> MODULE_DESCRIPTION() ones you can ignore as LKP unfortunately spams some 
> kernel-wide problems to all patches even if they're entirely unrelated
> to the patch in question.

Actually the LKP / kernel test robot email with the MODULE_DESCRIPTION()
warnings does contain useful, new errors. The problem though is that
the bot prefixes new (not seen on builds without this patch) messages
with '>>' making them show up as quoted (replied-to) text making the
actual new errors harder to spot :|   The 2 new actual errors in the
email with all the MODULE_DESCRIPTION warnings are:

ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!

I agree with all the other review remarks and I'll do my own review
soon.

Regards,

Hans