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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.






[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux