Re: [PATCH v5 09/11] HID: asus: add basic RGB support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4928 bytes --]

On Wed, 26 Mar 2025, Antheas Kapenekakis wrote:
> On Wed, 26 Mar 2025 at 09:54, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 25 Mar 2025, Antheas Kapenekakis wrote:
> >
> > > Adds basic RGB support to hid-asus through multi-index. The interface
> > > works quite well, but has not gone through much stability testing.
> > > Applied on demand, if userspace does not touch the RGB sysfs, not
> > > even initialization is done. Ensuring compatibility with existing
> > > userspace programs.
> > >
> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > > ---
> > >  drivers/hid/Kconfig    |   1 +
> > >  drivers/hid/hid-asus.c | 171 +++++++++++++++++++++++++++++++++++++----
> > >  2 files changed, 156 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > index dfc245867a46a..d324c6ab997de 100644

> > > +     };
> > > +     unsigned long flags;
> > > +     uint8_t colors[3];
> > > +     bool rgb_init, rgb_set;
> > > +     int ret;
> > > +
> > > +     spin_lock_irqsave(&led->lock, flags);
> > > +     rgb_init = led->rgb_init;
> > > +     rgb_set = led->rgb_set;
> > > +     led->rgb_set = false;
> > > +     colors[0] = led->rgb_colors[0];
> > > +     colors[1] = led->rgb_colors[1];
> > > +     colors[2] = led->rgb_colors[2];
> > > +     spin_unlock_irqrestore(&led->lock, flags);
> > > +
> > > +     if (!rgb_set)
> > > +             return;
> > > +
> > > +     if (rgb_init) {
> > > +             ret = asus_kbd_init(led->hdev, FEATURE_KBD_LED_REPORT_ID1);
> > > +             if (ret < 0) {
> > > +                     hid_err(led->hdev, "Asus failed to init RGB: %d\n", ret);
> > > +                     return;
> > > +             }
> > > +             spin_lock_irqsave(&led->lock, flags);
> > > +             led->rgb_init = false;
> > > +             spin_unlock_irqrestore(&led->lock, flags);
> > > +     }
> > > +
> > > +     /* Protocol is: 54b3 zone (0=all) mode (0=solid) RGB */
> >
> > BTW, this comment is very cryptic to me and I'm unable to connect it with
> > the code below. My only guess is that each non-parenthesized word is
> > explaining one index but things don't add up given what rgb_buf[0][0] and
> > [0][1] have.
> 
> Maybe i fatfingered 54 and it should be 5a. Protocol is 54b3 zone mode
> R G B. So colors go to indexes 4, 5, 6

Ah. I suggest you add the spaces between the bytes to make it more 
obvious. Although, this could be a constructed as struct as well in which 
case the struct itself would document the format without need to 
cryptic comments nor use of numeric indexes.

> > > +     rgb_buf[0][4] = colors[0];
> > > +     rgb_buf[0][5] = colors[1];
> > > +     rgb_buf[0][6] = colors[2];
> > > +
> > > +     for (size_t i = 0; i < ARRAY_SIZE(rgb_buf); i++) {
> > > +             ret = asus_kbd_set_report(led->hdev, rgb_buf[i], sizeof(rgb_buf[i]));
> > > +             if (ret < 0) {
> > > +                     hid_err(led->hdev, "Asus failed to set RGB: %d\n", ret);
> > > +                     return;
> > > +             }
> > > +     }
> > > +}

> > >       ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
> > > -     if (ret < 0) {
> > > -             /* No need to have this still around */
> > > -             devm_kfree(&hdev->dev, drvdata->kbd_backlight);
> > > +     /* Asus-wmi might not be accessible so this is not fatal. */
> > > +     if (!ret)
> > > +             hid_warn(hdev, "Asus-wmi brightness listener not registered\n");
> >
> > Is the condition correct way around given the message?
> 
> You are right.
> 
> > Please also note that you don't need to send an update every day or so
> > after minor comments like this. We're in merge window currently which
> > means I likely won't be applying any next material until -rc1 has been
> > released.
> 
> If this is 6.16 material I am happy to put a pause on this for the
> next 1-3 weeks.

You don't need to "pause" for the merge window, in some subsystem 
there's mandatory pause during merge window but I find that unnecessary.
I know people on pdx86 do review during merge window so no need to wait 
when working with patches related to pdx86. Just don't expect patches 
get applied during the merge window or right after it (the latter tends to 
be the most busiest time of cycle for me) :-).

It's more about the frequency, how often to send a series which is 
relatively large. Large number of versions end up just filling inboxes 
(and patchwork's pending patches list) and we don't have time to read them 
all through so I suggest waiting like 3 days at minimum between versions 
when the series is large or complex to give time to go through the series.

This is not a hard rule, so if there are e.g. many significant changes, 
feel free to "violate" it in that case.

-- 
 i.