| 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: <06B-xdz9pdNnj8DeXMMmdUi9Z7NAmQd3mFSHALyKMARSKZmal3FTLvXYKeF7SX_h78ko8RCpbC3t9wUanK6T2nNz1sYVtrSSGEcsfWAxtXY=@protonmail.com>
Date: Sat, 06 Aug 2022 17:30:04 +0000
From: Barnabás Pőcze <pobrn@...tonmail.com>
To: "Luke D. Jones" <luke@...nes.dev>
Cc: hdegoede@...hat.com, markgross@...nel.org,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard RGB
Hi
2022. augusztus 5., péntek 10:19 keltezéssel, Luke D. Jones <luke@...nes.dev> írta:
> 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
> default mode of the keyboard to static. This overwrites the booted
> state of the keyboard.
>
> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> ---
> drivers/platform/x86/asus-wmi.c | 89 ++++++++++++++++++++++
> include/linux/platform_data/x86/asus-wmi.h | 3 +
> 2 files changed, 92 insertions(+)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 0e7fbed8a50d..33384e3321bb 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -25,6 +25,7 @@
> #include <linux/input/sparse-keymap.h>
> #include <linux/kernel.h>
> #include <linux/leds.h>
> +#include <linux/led-class-multicolor.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> #include <linux/pci_hotplug.h>
> @@ -190,6 +191,11 @@ struct fan_curve_data {
> u8 percents[FAN_CURVE_POINTS];
> };
>
> +struct keyboard_rgb_led {
> + struct led_classdev_mc dev;
> + struct mc_subled subled_info[3]; /* r g b */
> +};
> +
> struct asus_wmi {
> int dsts_id;
> int spec;
> @@ -234,6 +240,9 @@ struct asus_wmi {
> bool dgpu_disable_available;
> bool dgpu_disable;
>
> + bool keyboard_rgb_mode_available;
I think this variable could be introduced in the next patch, it is not used in this one.
> + struct keyboard_rgb_led keyboard_rgb_mode;
> +
> bool throttle_thermal_policy_available;
> u8 throttle_thermal_policy_mode;
>
> @@ -1028,6 +1037,35 @@ static enum led_brightness lightbar_led_get(struct led_classdev *led_cdev)
> return result & ASUS_WMI_DSTS_LIGHTBAR_MASK;
> }
>
> +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);
> +
> + 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);
> + if (err) {
> + pr_err("Unable to set TUF RGB data?\n");
> + return err;
> + }
> + return 0;
> +}
> +
> +static enum led_brightness tuf_rgb_brightness_get(struct led_classdev *cdev)
> +{
> + return cdev->brightness;
> +}
If you can't query the brightness from the hardware, I think you can leave
`led_classdev::brightness_get` to be `NULL`. This callback is only used from
`led_update_brightness()` as far as I can see.
> +
> static void asus_wmi_led_exit(struct asus_wmi *asus)
> {
> led_classdev_unregister(&asus->kbd_led);
> @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> &asus->lightbar_led);
> }
>
> + 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;
> +
> + /*
> + * 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,
> + 3,
> + sizeof(*mc_led_info),
> + GFP_KERNEL | __GFP_ZERO);
> +
I am a bit confused as to why dynamic allocation is needed here. Haven't you
already "allocated" the storage in `keyboard_rgb_led::subled_info`?
> + 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;
`led_mc_calc_color_components()` uses `led_classdev_mc::num_colors`, so I think
it needs to be set before calling it. But that function sets the subled brightness
based on the intensity, so it will overwrite the brightness values that have just
been set.
> +
> + rv = led_classdev_multicolor_register(&asus->platform_device->dev, mc_cdev);
> + }
> +
> error:
> if (rv)
> asus_wmi_led_exit(asus);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index a571b47ff362..d63c9945a17d 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -98,6 +98,9 @@
> /* dgpu on/off */
> #define ASUS_WMI_DEVID_DGPU 0x00090020
>
> +/* TUF laptop RGB control */
> +#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
> +
> /* DSTS masks */
> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001
> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002
> --
> 2.37.1
>
>
Regards,
Barnabás Pőcze
Powered by blists - more mailing lists