Re: [PATCH v2 2/6] asus-wmi: Implement TUF laptop keyboard LED modes

[Luke Jones <luke@ljones.dev>]

Hi Andy,

> 
>>  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 
>> 3)
>>  +               return -EINVAL;
> 
> Same comment as per v1.
> 

You wrote:

 > Usually we have three separate nodes for that, but they are kinda
 > hidden in one driver, so I don't care much.

I think that is the wrong direction to take. Doing so would mean that 
every write to one of these values has to write-out to device. I don't 
know how long writes on an i2c device take, but on the USB connected 
versions they are 1ms, which means that to individually set colour, 
save, mode, speed (also direction and sometimes a 2nd colour on USB) 
adds up to 4-6ms - and I don't know what sort of impact that has in the 
kernel itself, but I do know that users expect there to be fancy 
effects available on par with Windows (like audio equalizer visuals on 
the RGB, something that is in progress in asusctl).

Using multicolor LED class already breaks away from having a single 
packet write, but the gain in API scope was worth the compromise. 
Hopefully we can keep the single set of parameters here?

Pavel suggested using triggers, I've yet to look at that, but will do 
so after finalising this.

I suppose one alternative would be to store speed and mode as 
attributes, but not write out until the "save" node is written to? So 
this raises the question of: we can't read from device, and speed+mode 
must be saved in module for use with "save" now, should I then allow 
showing these values in a _show? On fresh boot they will be incorrect..

I'm going to go ahead and split those parameters in to individual nodes 
now anyway - it may help with later work using triggers.


> ...
> 
>>  +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? 
>> mode : 0x0a;
> 
> Same comment as per v1.
> 

I missed it sorry. Done now.

> ...
> 
>>  +       switch (speed) {
>>  +       case 0:
>>  +               asus->keyboard_rgb_mode.speed = 0xe1;
>>  +               break;
>>  +       case 1:
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
>>  +               break;
>>  +       case 2:
>>  +               asus->keyboard_rgb_mode.speed = 0xf5;
>>  +               break;
>>  +       default:
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
> 
> break;

Done

> 
>>  +       }
> 
> ...
> 
>>  +
> 
> A blank line is not needed here.

Okay thanks, I'll go through previous patches and check this.

Kind regards,
Luke.
>