On Mon, Aug 8, 2022 at 5:09 AM Luke D. Jones <luke@xxxxxxxxxx> wrote:
>
> Adds support for setting various power states of TUF keyboards.
> These states are combinations of:
> - boot, set if a boot animation is shown on keyboard
> - awake, set if the keyboard LEDs are visible while laptop is on
> - sleep, set if an animation is displayed while the laptop is suspended
> - keyboard (unknown effect)
>
> Adds two sysfs attributes to asus-nb-wmi:
> - keyboard_rgb_state
> - keyboard_rgb_state_index
...
> + flags = 0;
This can be done before 'if (boot)'
> + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, &awake, &sleep, &keyboard) != 5)
> + return -EINVAL;
Same Q here: wouldn't it be better to put each of the parameters to a
separate sysfs node? Or look at the LED ABI (that what Pavel mentioned
for multi-color patterns) and see if there are already some
established ways of how to represent necessary information?
> + save = save == 0 ? 0x0100 : 0x0000;
if (save)
flags = BIT(8);
> + if (boot)
> + flags |= 0x02;
> + if (awake)
> + flags |= 0x08;
> + if (sleep)
> + flags |= 0x20;
> + if (keyboard)
> + flags |= 0x80;
Use BIT() for flags.
...
> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
> + ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | (flags << 16), 0, &ret);
Why not provide flags to be a full 32-bit value?
Also 0xBD can be lower-cased and explained somehow?
...
> +
No need for a blank line.
> +static DEVICE_ATTR_WO(keyboard_rgb_state);
...
> +
> +static DEVICE_ATTR_RO(keyboard_rgb_state_index);
Ditto and same for many other similar cases.
...
> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001
BIT(0) ? (This might require to add bits.h inclusion)
--
With Best Regards,
Andy Shevchenko
