On Wed, 26 Mar 2025, Antheas Kapenekakis wrote:

On Wed, 26 Mar 2025 at 09:54, Ilpo Järvinen
[off-list ref] wrote:

quoted

On Tue, 25 Mar 2025, Antheas Kapenekakis wrote:

quoted

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

quoted

quoted

+     };
+     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.

quoted

quoted

+     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;
+             }
+     }
+}

quoted

quoted

      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.

quoted

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.