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. Best regards, Kristian
