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

[Luke Jones <luke@ljones.dev>]

Hi Andy,

On Sat, Aug 6 2022 at 11:56:58 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> 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