Messages in this thread Patch in this message | | | Date | Fri, 05 Aug 2022 10:31:00 +1200 | From | Luke Jones <> | Subject | Re: [PATCH v2 1/1] asus-wmi: Add support for TUF laptop keyboard states |
| |
Hi Hans,
Thanks for the valuable review,
On Thu, Aug 4 2022 at 15:22:00 +0200, Hans de Goede <hdegoede@redhat.com> wrote: > Hi Luke, > > On 8/4/22 01:13, Luke D. Jones wrote: >> Adds support for the TUF series laptop power states. This means >> control of if the LED's are on while awake, or an animation is >> shown while booting or suspended. >> >> /sys/devices/platform/asus-nb-wmi/tuf_krgb_state_index provides >> labels for the index fields as "save boot awake sleep keyboard" >> >> /sys/devices/platform/asus-nb-wmi/tuf_krgb_state has the following >> as input options via boolean "b b b b b": >> - Save or set, if set, then settings revert on cold boot >> - Boot, if true, the keyboard displays animation on boot >> - Awake, if true, the keyboard LED's are on while device is awake >> - Sleep, if true, the keyboard shows animation while device is >> suspended >> - Keybaord, appears to have no effect > > Keybaord typo / spelling issue. > > Please make this an extra attribute for the led_class_device, > you can do this by adding this attribute to a separate > attribute_group, lets say e.g. "tuf_rgb_attributes" and then > in the "[PATCH] asus-wmi: Add support for TUF laptop keyboard RGB" > code add: > > mc_cdev->led_cdev.groups = tuf_rgb_attributes; > > and then the "tuf_krgb_state" file should show up as: > /sys/class/leds/asus::multicolour/tuf_krgb_state
Oh this is very cool! I was honestly wondering about if that could be done but was unsure how and finding the right docs isn't always easy.
> > Also I'm not sure what to think of the tuf_krgb_state_index file, > having a sysfs file just to show some help text feels weird / wrong.
I modelled that after the multicolor version in led-class-multicolor.c:
static ssize_t multi_index_show(struct device *dev, struct device_attribute *multi_index_attr, char *buf) { struct led_classdev *led_cdev = dev_get_drvdata(dev); struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev); int len = 0; int index; int i;
for (i = 0; i < mcled_cdev->num_colors; i++) { index = mcled_cdev->subled_info[i].color_index; len += sprintf(buf + len, "%s", led_colors[index]); if (i < mcled_cdev->num_colors - 1) len += sprintf(buf + len, " "); }
buf[len++] = '\n'; return len; } static DEVICE_ATTR_RO(multi_index);
Since it was done there it seemed okay?
> > Please instead document the expected format in the existing: > Documentation/ABI/testing/sysfs-platform-asus-wmi > > file; and talking about that file, it seems that this file > could use some love to document other recently addes asus-wmi > features too. >
Oof, yep I'll hop on that.
> Related to the tuf_krgb_state_index file, please use the > new sysfs_emit helper for all new show functions. And bonus > points for a patch (series?) converting old show functions > over to it.
For sure I'll do both. I guess this will turn in to a series of patches now. If only I could get paid for the work..
Many thanks, Luke.
> > Regards, > > Hans > > > > > >> Signed-off-by: Luke D. Jones <luke@ljones.dev> >> --- >> drivers/platform/x86/asus-wmi.c | 95 >> ++++++++++++++++++++++ >> include/linux/platform_data/x86/asus-wmi.h | 3 + >> 2 files changed, 98 insertions(+) >> >> diff --git a/drivers/platform/x86/asus-wmi.c >> b/drivers/platform/x86/asus-wmi.c >> index 0e7fbed8a50d..bbfb054ff3b2 100644 >> --- a/drivers/platform/x86/asus-wmi.c >> +++ b/drivers/platform/x86/asus-wmi.c >> @@ -234,6 +234,8 @@ struct asus_wmi { >> bool dgpu_disable_available; >> bool dgpu_disable; >> >> + bool tuf_krgb_state_available; >> + >> bool throttle_thermal_policy_available; >> u8 throttle_thermal_policy_mode; >> >> @@ -734,6 +736,86 @@ static ssize_t egpu_enable_store(struct device >> *dev, >> >> static DEVICE_ATTR_RW(egpu_enable); >> >> +/* TUF Laptop Keyboard RGB States >> *********************************************/ >> +static int tuf_krgb_state_check_present(struct asus_wmi *asus) >> +{ >> + u32 result; >> + int err; >> + >> + asus->tuf_krgb_state_available = false; >> + >> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_STATE, >> &result); >> + if (err) { >> + if (err == -ENODEV) >> + return 0; >> + return err; >> + } >> + >> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) >> + asus->tuf_krgb_state_available = true; >> + >> + return 0; >> +} >> + >> +static ssize_t tuf_krgb_state_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + int err; >> + u32 ret; >> + bool tmp; >> + char *data, *part, *end; >> + u8 save, flags, res, arg_num; >> + >> + save = flags = arg_num = 0; >> + data = end = kstrdup(buf, GFP_KERNEL); >> + >> + while ((part = strsep(&end, " ")) != NULL) { >> + if (part == NULL) >> + return -1; >> + >> + res = kstrtobool(part, &tmp); >> + if (res) >> + return -1; >> + >> + if (tmp) { >> + if (arg_num == 0) // save : set >> + save = tmp == 0 ? 0x0100 : 0x0000; >> + else if (arg_num == 1) >> + flags |= 0x02; // boot >> + else if (arg_num == 2) >> + flags |= 0x08; // awake >> + else if (arg_num == 3) >> + flags |= 0x20; // sleep >> + else if (arg_num == 4) >> + flags |= 0x80; // keyboard >> + } >> + >> + arg_num += 1; >> + } >> + >> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, >> + ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | (flags << 16), 0, >> &ret); >> + if (err) { >> + return err; >> + } >> + >> + kfree(data); >> + return count; >> +} >> + >> +static DEVICE_ATTR_WO(tuf_krgb_state); >> + >> +static ssize_t tuf_krgb_state_index_show(struct device *device, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + int len = sprintf(buf, "%s", "save boot awake sleep keyboard\n"); >> + return len; >> +} >> + >> +static DEVICE_ATTR_RO(tuf_krgb_state_index); >> + >> /* Battery >> ********************************************************************/ >> >> /* The battery maximum charging percentage */ >> @@ -3258,6 +3340,8 @@ static struct attribute >> *platform_attributes[] = { >> &dev_attr_touchpad.attr, >> &dev_attr_egpu_enable.attr, >> &dev_attr_dgpu_disable.attr, >> + &dev_attr_tuf_krgb_state.attr, >> + &dev_attr_tuf_krgb_state_index.attr, >> &dev_attr_lid_resume.attr, >> &dev_attr_als_enable.attr, >> &dev_attr_fan_boost_mode.attr, >> @@ -3286,6 +3370,12 @@ static umode_t asus_sysfs_is_visible(struct >> kobject *kobj, >> devid = ASUS_WMI_DEVID_ALS_ENABLE; >> else if (attr == &dev_attr_egpu_enable.attr) >> ok = asus->egpu_enable_available; >> + else if (attr == &dev_attr_tuf_krgb_state.attr) >> + ok = asus->tuf_krgb_state_available; >> + else if (attr == &dev_attr_tuf_krgb_state_index.attr) >> + ok = asus->tuf_krgb_state_available; >> + else if (attr == &dev_attr_dgpu_disable.attr) >> + ok = asus->dgpu_disable_available; >> else if (attr == &dev_attr_dgpu_disable.attr) >> ok = asus->dgpu_disable_available; >> else if (attr == &dev_attr_fan_boost_mode.attr) >> @@ -3557,6 +3647,10 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> if (err) >> goto fail_dgpu_disable; >> >> + err = tuf_krgb_state_check_present(asus); >> + if (err) >> + goto fail_tuf_krgb_state; >> + >> err = fan_boost_mode_check_present(asus); >> if (err) >> goto fail_fan_boost_mode; >> @@ -3671,6 +3765,7 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> fail_fan_boost_mode: >> fail_egpu_enable: >> fail_dgpu_disable: >> +fail_tuf_krgb_state: >> fail_platform: >> fail_panel_od: >> kfree(asus); >> diff --git a/include/linux/platform_data/x86/asus-wmi.h >> b/include/linux/platform_data/x86/asus-wmi.h >> index a571b47ff362..a95b37385e66 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 power/state */ >> +#define ASUS_WMI_DEVID_TUF_RGB_STATE 0x00100057 >> + >> /* DSTS masks */ >> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 >> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 >
| |