lkml.org 
[lkml]   [2022]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard RGB
On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> Adds support for TUF laptop RGB control via the multicolor LED API.
>
> As this is the base essentials for adjusting the RGB, it sets the

these are
...or...
essential

> default mode of the keyboard to static. This overwrites the booted
> state of the keyboard.

...

> #include <linux/leds.h>
> +#include <linux/led-class-multicolor.h>

Not sure about the ordering ('-' vs. 's') in locale C.

...

> +static int tuf_rgb_brightness_set(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + u8 r, g, b;
> + int err;
> + u32 ret;

> +
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);

No need to put blank lines in the definition block. Also it would be
better to move the longest line to be first.

> + led_mc_calc_color_components(mc_cdev, brightness);
> + r = mc_cdev->subled_info[0].brightness;
> + g = mc_cdev->subled_info[1].brightness;
> + b = mc_cdev->subled_info[2].brightness;
> +
> + /* Writing out requires some defaults. This will overwrite boot mode */
> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE,
> + 1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret);

What the point in those ' | 0' additions?

> + if (err) {
> + pr_err("Unable to set TUF RGB data?\n");

Why not dev_err() ?

> + return err;
> + }
> + return 0;

return err;

> +}

...

> + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) {
> + struct led_classdev_mc *mc_cdev;
> + struct mc_subled *mc_led_info;
> + u8 brightness = 127;

> + mc_cdev = &asus->keyboard_rgb_mode.dev;

Join this with the definition above. It's fine if it's a bit longer
than 80 characters.

> + /*
> + * asus::kbd_backlight still controls a base 3-level backlight and when
> + * it is on 0, the RGB is not visible at all. RGB should be treated as
> + * an additional step.
> + */
> + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight";
> + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + mc_cdev->led_cdev.brightness_set_blocking = tuf_rgb_brightness_set;
> + mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get;
> +
> + /* Let the multicolour LED own the info */
> + mc_led_info = devm_kmalloc_array(
> + &asus->platform_device->dev,

With a temporary variable you may make this one line shorter and nicer looking

struct device *dev = &asus->platform_device->dev;

> + 3,
> + sizeof(*mc_led_info),
> + GFP_KERNEL | __GFP_ZERO);
> +
> + if (!mc_led_info)
> + return -ENOMEM;
> +
> + mc_led_info[0].color_index = LED_COLOR_ID_RED;
> + mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> + mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> +
> + /*
> + * It's not possible to get last set data from device so set defaults
> + * to make it safe for a user to change either RGB or modes. We don't
> + * write these defaults to the device because they will overwrite a
> + * users last saved boot setting (in NVRAM).
> + */
> + mc_cdev->led_cdev.brightness = brightness;
> + mc_cdev->led_cdev.max_brightness = brightness;
> + mc_led_info[0].intensity = brightness;
> + mc_led_info[0].brightness = mc_cdev->led_cdev.brightness;
> + mc_led_info[1].brightness = mc_cdev->led_cdev.brightness;
> + mc_led_info[2].brightness = mc_cdev->led_cdev.brightness;
> + led_mc_calc_color_components(mc_cdev, brightness);
> +
> + mc_cdev->subled_info = mc_led_info;
> + mc_cdev->num_colors = 3;
> +
> + rv = led_classdev_multicolor_register(&asus->platform_device->dev, mc_cdev);

This also becomes shorter.

> + }

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2022-08-06 11:45    [W:0.159 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site