On Sat, 2023-07-15 at 20:40 +0300, Kristian Angelov wrote: > On 15/07/23 09:40, Hans de Goede wrote: > > Hi Kristian, > > > > On 7/14/23 22:44, Kristian Angelov wrote: > > > This patch fixes setting the cmd values to 0xb3 and 0xb4. > > > This is necessary on some TUF laptops in order to set the RGB > > > mode. > > > > > > Closes: > > > https://lore.kernel.org/platform-driver-x86/443078148.491022.1677576298133@xxxxxxxxxxx > > > Signed-off-by: Kristian Angelov <kristiana2000@xxxxxx> > > > --- > > > V1 -> V2. Make setting 0xb3 and 0xb4 the default logic > > > > > > drivers/platform/x86/asus-wmi.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/platform/x86/asus-wmi.c > > > b/drivers/platform/x86/asus-wmi.c > > > index 1038dfdcdd32..eb82ed723b42 100644 > > > --- a/drivers/platform/x86/asus-wmi.c > > > +++ b/drivers/platform/x86/asus-wmi.c > > > @@ -738,13 +738,22 @@ static ssize_t kbd_rgb_mode_store(struct > > > device *dev, > > > struct device_attribute *attr, > > > const char *buf, size_t count) > > > { > > > - u32 cmd, mode, r, g, b, speed; > > > + u32 cmd, mode, r, g, b, speed; > > > int err; > > > > > > if (sscanf(buf, "%d %d %d %d %d %d", &cmd, &mode, &r, &g, > > > &b, &speed) != 6) > > > return -EINVAL; > > > > > > - cmd = !!cmd; > > > + /* B3 is set and B4 is save to BIOS. Only set by > > > default*/ > > > + switch (cmd) { > > > + default: > > > + case 0: > > > + cmd = 0xb3; > > > + break; > > > + case 1: > > > + cmd = 0xb4; > > > + break; > > > + } > > > > You are now leaving the value of cmd unmodified for values which > > are not 0 and 1. > > > > I think you need to add a: > > > > default: > > return -EINVAL; > > > > here to catch cmd not being either 0 or 1. > > > > Luke, what do you think ? > > > > Regards, > > > > Hans > > > Hi Hans, > > > If you would notice I did: > > switch (cmd) { > > default: > > case 0: > > cmd = 0xb3; > > break; > > > In the case of cmd not being 1 or 0. It would default to 0 which > would only set > the RGB mode, not save it to BIOS. I guess this would depend on > preference. > Is it better to fail out on invalid input or simply ignore it and > perform the > "default" action. Setting the RGB mode is not persistent, only saving > it is. So > defaulting to this seems reasonable, but if not I'll do a v3 with > EINVAL fail. Much of, if not all of asus-wmi follows a strict i/o and will EINVAL if the input is not correct. It would be best to keep this behaviour consistent. It would be misleading to folks if any value except 1 produced the same result, and then if there was reason to make a 2 produce some other effect then it breaks someone's expectation somewhere.
