From: Luke Jones <luke@ljones.dev>
To: "Barnabás Pőcze" <pobrn@protonmail.com>
Cc: hdegoede@redhat.com, markgross@kernel.org,
platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard RGB
Date: Sun, 07 Aug 2022 13:54:52 +1200 [thread overview]
Message-ID: <GN28GR.5DRC7JSLXRDT3@ljones.dev> (raw)
In-Reply-To: <06B-xdz9pdNnj8DeXMMmdUi9Z7NAmQd3mFSHALyKMARSKZmal3FTLvXYKeF7SX_h78ko8RCpbC3t9wUanK6T2nNz1sYVtrSSGEcsfWAxtXY=@protonmail.com>
Hi Barnabás,
Thank you for taking the time to review
On Sat, Aug 6 2022 at 17:30:04 +0000, Barnabás Pőcze
<pobrn@protonmail.com> wrote:
> Hi
>
>
> 2022. augusztus 5., péntek 10:19 keltezéssel, Luke D. Jones
> <luke@ljones.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@ljones.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.
>
Thank you for spotting this. I must have missed it when I split the
patches.
>
>> + 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.
>
I did wonder about this. Thanks, I've nulled it.
>
>> +
>> 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`?
Honestly, I write rust 90% of the time which is quite clear on these
things, so here I wasn't very sure about what to do - I read the
hid-playstation.c and it had something similar so I used it and
completely forgot about the struct allocation.
I've updated to `struct mc_subled *mc_led_info =
asus->keyboard_rgb_mode.subled_info;` and removed the dynamic alloc.
>
>
>> + 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.
Ah thanks, I don't think I understood that before. I've removed
mc_led_info[i].brightness setting now.
>
>
>> +
>> + 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
next prev parent reply other threads:[~2022-08-07 1:55 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-05 8:19 [PATCH 0/5] asus-wmi: Add support for RGB keyboards Luke D. Jones
2022-08-05 8:19 ` [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard RGB Luke D. Jones
2022-08-06 9:44 ` Andy Shevchenko
2022-08-06 10:16 ` Luke Jones
2022-08-06 10:27 ` Andy Shevchenko
2022-08-06 10:27 ` Andy Shevchenko
2022-08-06 17:30 ` Barnabás Pőcze
2022-08-07 1:54 ` Luke Jones [this message]
2022-08-05 8:19 ` [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control Luke D. Jones
2022-08-06 9:56 ` Andy Shevchenko
2022-08-06 10:33 ` Luke Jones
2022-08-06 11:12 ` Andy Shevchenko
2022-08-05 8:19 ` [PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states Luke D. Jones
2022-08-05 12:08 ` Pavel Machek
2022-08-05 21:29 ` Luke Jones
2022-08-07 7:44 ` Luke Jones
2022-08-07 12:41 ` Pavel Machek
2022-08-09 23:03 ` Luke Jones
2022-08-05 8:19 ` [PATCH 4/5] asus-wmi: Document many of the undocumented API Luke D. Jones
2022-08-05 21:43 ` kernel test robot
2022-08-06 10:00 ` Andy Shevchenko
2022-08-05 8:19 ` [PATCH 5/5] asus-wmi: Convert all attr _show to use sysfs_emit Luke D. Jones
2022-08-06 10:05 ` Andy Shevchenko
2022-08-05 12:05 ` [PATCH 0/5] asus-wmi: Add support for RGB keyboards Pavel Machek
2022-08-05 21:30 ` Luke Jones
2022-08-06 9:10 ` Andy Shevchenko
2022-08-06 9:32 ` Luke Jones
2022-08-06 9:48 ` Andy Shevchenko
2022-08-06 10:02 ` Andy Shevchenko
2022-08-06 10:48 ` Luke Jones
2022-08-06 11:16 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=GN28GR.5DRC7JSLXRDT3@ljones.dev \
--to=luke@ljones.dev \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).