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

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 11:43 PM Luke Jones <luke@xxxxxxxxxx> wrote:

...

> >>  +       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).

This is a good justification, but easy to workaround by using another
node like "submit" or unifying it with one of the existing nodes
implicitly (like setting savee will submit all changes at once).

> 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.

The thing is you can't "finalize" this and go to another type of ABI,
because we come with two ABIs - we may not drop the old one once it's
established (yes, there are exceptions, but it's rare). So, knowing
that we would drop an ABI we mustn't introduce it in the first place.

> I suppose one alternative would be to store speed and mode as
> attributes, but not write out until the "save" node is written to?

Yes!

> 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..

Yep, they should be reset either by hardware, or provided somehow
(device tree?) from the platform knowing what firmware sets and what
kernel may use if you don't want to change the settings.

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

Okay!

-- 
With Best Regards,
Andy Shevchenko