Re: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control

[Luke Jones]

Hi Andy,

On Sat, Aug 6 2022 at 11:56:58 +0200, Andy Shevchenko 
<andy.shevchenko@xxxxxxxxx> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@xxxxxxxxxx> wrote:
> >  Adds support for TUF laptop RGB mode control.
> >
> >  Two paths are added:
> >  - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode
> >  - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode_index
>
> ...
>
> >  +static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
> >  +{
> >  +       u32 result;
> >  +       int err;
> >  +
> >  +       asus->keyboard_rgb_mode_available = false;
> >  +
> >  +       err = asus_wmi_get_devstate(asus, 
> > ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
> >  +       if (err) {
> >  +               if (err == -ENODEV)
> >  +                       return 0;
> >  +               return err;
> >  +       }
>
> >  +       if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> >  +               asus->keyboard_rgb_mode_available = true;
> >  +       }
>
> {} are not needed (except if they will be utilized in the next patches
> in the series).

I've usually been pretty good at catching these. I must not have run 
the patch check script on this one.

Fixed.

> >  +       return 0;
> >  +}
>
> ...
>
> >  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 
> > 3)
> >  +               return -EINVAL;
>
> Usually we have three separate nodes for that, but they are kinda
> hidden in one driver, so I don't care much.

I don't really understand what you mean sorry.

> ...
>
> >  +       asus->keyboard_rgb_mode.save = save > 0 ? 1 : 0;
>
> So, it's actually boolean.
>
> You may write it as
>
>     ...save = !!save;

Err okay. Done.

> >  +       /* These are the known usable modes across all TUF/ROG */
> >  +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? 
> > mode : 0x0a;
> >  +
> >  +       if (speed == 0)
> >  +               asus->keyboard_rgb_mode.speed = 0xe1;
> >  +       else if (speed == 1)
> >  +               asus->keyboard_rgb_mode.speed = 0xeb;
> >  +       else if (speed == 2)
> >  +               asus->keyboard_rgb_mode.speed = 0xf5;
>
> >  +       else
> >  +               asus->keyboard_rgb_mode.speed = 0xeb;
>
> So the 1 is default then, why not use switch-case to show this 
> explicitly?
>
> switch (speed) {
>   case 0:
>     ...
>   break;
>   case 1:
>   default:
>     ...
>   break;
>   case 2:
>     ...
>   break;
> }
>
> Yes, it's longer, but I think it's cleaner.

Agreed. Done.

> >  +       err = tuf_rgb_brightness_set(cdev, cdev->brightness);
> >  +       if (err)
> >  +               return err;
> >  +       return 0;
>
> return tuf_rgb_brightness_set(...);

This causes a hang (waiting for return somewhere?) if I don't return 
count. Especially true if the return is 0.

> >  +}
>
> --
> With Best Regards,
> Andy Shevchenko