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.
