lkml.org 
[lkml]   [2022]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control
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


\
 
 \ /
  Last update: 2022-08-06 12:35    [W:0.064 / U:1.752 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site