Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 9 Aug 2022 11:22:55 +0200 | Subject | Re: [PATCH v3 2/6] asus-wmi: Implement TUF laptop keyboard LED modes |
| |
On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <luke@ljones.dev> wrote: > > Adds support for changing the laptop keyboard LED modes. These > are visible effects such as static, rainbow, pulsing, colour cycles. > > These sysfs attributes are added to asus-nb-wmi: > - keyboard_rgb_save > - keyboard_rgb_mode > - keyboard_rgb_speed
...
> +What: /sys/devices/platform/<platform>/keyboard_rgb_speed > +Date: Aug 2022 > +KernelVersion: 6.1 > +Contact: "Luke Jones" <luke@ljones.dev> > +Description: > + Set the speed of the selected RGB effect, the speed will not apply > + until the keyboard_rgb_save attribute is set (write-only): > + * 0 - slow > + * 1 - medium > + * 2 - fast
> \ No newline at end of file
^^^
...
> + u8 save; > + int err;
> + > + struct asus_wmi *asus = dev_get_drvdata(device); > + struct led_classdev *cdev = &asus->keyboard_rgb_led.dev.led_cdev;
No blank line in the definition block and try to keep "the longest line first", a.k.a. reversed xmas tree ordering.
...
> + u8 mode; > + > + struct asus_wmi *asus = dev_get_drvdata(device);
Ditto.
I would really recommend you spending some time to read the existing code (better recent one) and look for the common patterns.
...
> + /* These are the known usable modes across all TUF/ROG */ > + if (mode >= 12 || mode == 10)
The second condition was different in previous versions. Or am I confused by another patch series?
> + asus->keyboard_rgb_led.mode = 10; > + else > + asus->keyboard_rgb_led.mode = mode;
...
> + > +
Single blank line is enough.
...
> - struct mc_subled *mc_led_info = asus->keyboard_rgb_mode.subled_info; > - struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_mode.dev; > + struct mc_subled *mc_led_info = asus->keyboard_rgb_led.subled_info; > + struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_led.dev;
Not sure why this change happened...
> - asus->keyboard_rgb_mode.save = 1; > - asus->keyboard_rgb_mode.mode = 0; > - asus->keyboard_rgb_mode.speed = 0xeb; > + asus->keyboard_rgb_led.save = 1; > + asus->keyboard_rgb_led.mode = 0; > + asus->keyboard_rgb_led.speed = 0xeb;
...and this. Is it some kind of renaming? Can you split it to another patch if it was initially like that?
-- With Best Regards, Andy Shevchenko
| |