| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <N7V6GR.XFSPFYLCEU9X2@ljones.dev>
Date: Sat, 06 Aug 2022 22:16:35 +1200
From: Luke Jones <luke@...nes.dev>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
Mark Gross <markgross@...nel.org>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard
RGB
Hi Andy, thanks for the feedback:
On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@...nes.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.
>
I used hid-playstation.c as a reference and followed that ordering.
> ...
>
>> +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.
Okay cool. Done.
>
>> + 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?
They were place-holders in testing that I forgot to change in the
second patch which adds mode configuration :(
Should be "save | (mode << 8) | (r << 16) | (g << 24), (b) | (speed <<
8), &ret);", two bytes.
>
>> + if (err) {
>> + pr_err("Unable to set TUF RGB data?\n");
>
> Why not dev_err() ?
I didn't know about it? Is there an example or doc on its use?
>
>> + return err;
>> + }
>> + return 0;
>
> return err;
Something like this then?
if (err) {
pr_err("Unable to set TUF RGB data?\n");
}
return err;
If so, done.
>
>> +}
>
> ...
>
>> + 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.
Done.
>
>> + /*
>> + * 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;
>
Done.
>> + 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.
Done.
>
>> + }
>
> --
> With Best Regards,
> Andy Shevchenko
Powered by blists - more mailing lists