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

Hans de Goede
2024-07-11T15:17:29Z

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