| Message ID | 20240325054938.489732-2-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | asus-wmi: add new features, clean up, fixes | expand |
On Mon, 25 Mar 2024, Luke D. Jones wrote: > Support the 2024 mini-led backlight and adjust the related functions > to select the relevant dev-id. Also add `available_mini_led_mode` to the > platform sysfs since the available mini-led levels can be different. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > .../ABI/testing/sysfs-platform-asus-wmi | 8 ++ > drivers/platform/x86/asus-wmi.c | 74 +++++++++++++++++-- > include/linux/platform_data/x86/asus-wmi.h | 1 + > 3 files changed, 76 insertions(+), 7 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi > index 8a7e25bde085..61a745d2476f 100644 > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi > @@ -126,6 +126,14 @@ Description: > Change the mini-LED mode: > * 0 - Single-zone, > * 1 - Multi-zone > + * 2 - Multi-zone strong (available on newer generation mini-led) > + > +What: /sys/devices/platform/<platform>/available_mini_led_mode > +Date: Jun 2023 > +KernelVersion: 6.10 > +Contact: "Luke Jones" <luke@ljones.dev> > +Description: > + List the available mini-led modes. > > What: /sys/devices/platform/<platform>/ppt_pl1_spl > Date: Jun 2023 > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 18be35fdb381..54ce0fb26f42 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -297,6 +297,7 @@ struct asus_wmi { > > bool panel_overdrive_available; > bool mini_led_mode_available; > + u32 mini_led_dev_id; > > struct hotplug_slot hotplug_slot; > struct mutex hotplug_lock; > @@ -2109,10 +2110,27 @@ static ssize_t mini_led_mode_show(struct device *dev, > struct asus_wmi *asus = dev_get_drvdata(dev); > int result; > > - result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE); > - if (result < 0) > - return result; > + result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); > > + /* Remap the mode values to match previous generation mini-led. > + * Some BIOSes return -19 instead of 2, which is "mini-LED off", this > + * appears to be a BIOS bug. > + */ > + if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) { > + switch (result) { > + case 0: > + result = 1; > + break; > + case 1: > + result = 2; > + break; > + case 2: > + case -19: Can you confirm this -19 really does come from BIOS? Because I suspect it's -ENODEV error code from from one of the functions on the driver side (which is why I asked you to change it into -ENODEV).
On Tue, 26 Mar 2024, at 2:47 AM, Ilpo Järvinen wrote: > On Mon, 25 Mar 2024, Luke D. Jones wrote: > > > Support the 2024 mini-led backlight and adjust the related functions > > to select the relevant dev-id. Also add `available_mini_led_mode` to the > > platform sysfs since the available mini-led levels can be different. > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > --- > > .../ABI/testing/sysfs-platform-asus-wmi | 8 ++ > > drivers/platform/x86/asus-wmi.c | 74 +++++++++++++++++-- > > include/linux/platform_data/x86/asus-wmi.h | 1 + > > 3 files changed, 76 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi > > index 8a7e25bde085..61a745d2476f 100644 > > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi > > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi > > @@ -126,6 +126,14 @@ Description: > > Change the mini-LED mode: > > * 0 - Single-zone, > > * 1 - Multi-zone > > + * 2 - Multi-zone strong (available on newer generation mini-led) > > + > > +What: /sys/devices/platform/<platform>/available_mini_led_mode > > +Date: Jun 2023 > > +KernelVersion: 6.10 > > +Contact: "Luke Jones" <luke@ljones.dev> > > +Description: > > + List the available mini-led modes. > > > > What: /sys/devices/platform/<platform>/ppt_pl1_spl > > Date: Jun 2023 > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > > index 18be35fdb381..54ce0fb26f42 100644 > > --- a/drivers/platform/x86/asus-wmi.c > > +++ b/drivers/platform/x86/asus-wmi.c > > @@ -297,6 +297,7 @@ struct asus_wmi { > > > > bool panel_overdrive_available; > > bool mini_led_mode_available; > > + u32 mini_led_dev_id; > > > > struct hotplug_slot hotplug_slot; > > struct mutex hotplug_lock; > > @@ -2109,10 +2110,27 @@ static ssize_t mini_led_mode_show(struct device *dev, > > struct asus_wmi *asus = dev_get_drvdata(dev); > > int result; > > > > - result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE); > > - if (result < 0) > > - return result; > > + result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); > > > > + /* Remap the mode values to match previous generation mini-led. > > + * Some BIOSes return -19 instead of 2, which is "mini-LED off", this > > + * appears to be a BIOS bug. > > + */ > > + if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) { > > + switch (result) { > > + case 0: > > + result = 1; > > + break; > > + case 1: > > + result = 2; > > + break; > > + case 2: > > + case -19: > > Can you confirm this -19 really does come from BIOS? Because I suspect > it's -ENODEV error code from from one of the functions on the driver side > (which is why I asked you to change it into -ENODEV). Yes it does. It is rather annoying. What happens in this case is that `2` is written to the WMI endpoint to turn off the MINI-Led feature, this works fine and it is turned off, there are no errors from the write at all - verifying the accepted limits in dsdt also shows it is correct. However, after that, the read fails once. And only if that `2` was written. `0` and `1` write fine, and read fine also. I hope I've managed to describe and clarify what I'm seeing here. I'm happy to change -ENODEV. No problem, queued on my todo list. Cheers, Luke.
On Tue, 26 Mar 2024, Luke Jones wrote: > On Tue, 26 Mar 2024, at 2:47 AM, Ilpo Järvinen wrote: > > On Mon, 25 Mar 2024, Luke D. Jones wrote: > > > > > Support the 2024 mini-led backlight and adjust the related functions > > > to select the relevant dev-id. Also add `available_mini_led_mode` to the > > > platform sysfs since the available mini-led levels can be different. > > > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > > --- > > > @@ -2109,10 +2110,27 @@ static ssize_t mini_led_mode_show(struct device *dev, > > > struct asus_wmi *asus = dev_get_drvdata(dev); > > > int result; > > > > > > - result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE); > > > - if (result < 0) > > > - return result; > > > + result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); > > > > > > + /* Remap the mode values to match previous generation mini-led. > > > + * Some BIOSes return -19 instead of 2, which is "mini-LED off", this > > > + * appears to be a BIOS bug. > > > + */ > > > + if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) { > > > + switch (result) { > > > + case 0: > > > + result = 1; > > > + break; > > > + case 1: > > > + result = 2; > > > + break; > > > + case 2: > > > + case -19: > > > > Can you confirm this -19 really does come from BIOS? Because I suspect > > it's -ENODEV error code from from one of the functions on the driver side > > (which is why I asked you to change it into -ENODEV). > > Yes it does. It is rather annoying. What happens in this case is that > `2` is written to the WMI endpoint to turn off the MINI-Led feature, > this works fine and it is turned off, there are no errors from the write > at all - verifying the accepted limits in dsdt also shows it is correct. > > However, after that, the read fails once. Hi, I'm left a bit unsure how to interpret your response. If "read fails", it would indicate that -ENODEV originates from asus_wmi_evaluate_method3(), asus_wmi_get_devstate() or asus_wmi_get_devstate_bits(), not from BIOS? So which way it is? After reading some more code, I think I figured out the answer myself. However, that raises another question... So lets now take a step back and walk through the code: Your patch does: result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); asus_wmi_get_devstate_simple() calls asus_wmi_get_devstate_bits() with ASUS_WMI_DSTS_STATUS_BIT mask that is 0x00000001. If there's no error, retval is masked with that ASUS_WMI_DSTS_STATUS_BIT forcing the return value to 0-1 range so: a) I don't think -19 can originate from BIOS but comes from kernel side. b) How can it ever return 2 (mini-LED off) ????? > And only if that `2` was > written. `0` and `1` write fine, and read fine also. I hope I've managed > to describe and clarify what I'm seeing here. > > I'm happy to change -ENODEV. No problem, queued on my todo list.
On Wed, 27 Mar 2024, at 12:49 AM, Ilpo Järvinen wrote: > On Tue, 26 Mar 2024, Luke Jones wrote: > > On Tue, 26 Mar 2024, at 2:47 AM, Ilpo Järvinen wrote: > > > On Mon, 25 Mar 2024, Luke D. Jones wrote: > > > > > > > Support the 2024 mini-led backlight and adjust the related functions > > > > to select the relevant dev-id. Also add `available_mini_led_mode` to the > > > > platform sysfs since the available mini-led levels can be different. > > > > > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > > > --- > > > > > @@ -2109,10 +2110,27 @@ static ssize_t mini_led_mode_show(struct device *dev, > > > > struct asus_wmi *asus = dev_get_drvdata(dev); > > > > int result; > > > > > > > > - result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE); > > > > - if (result < 0) > > > > - return result; > > > > + result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); > > > > > > > > + /* Remap the mode values to match previous generation mini-led. > > > > + * Some BIOSes return -19 instead of 2, which is "mini-LED off", this > > > > + * appears to be a BIOS bug. > > > > + */ > > > > + if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) { > > > > + switch (result) { > > > > + case 0: > > > > + result = 1; > > > > + break; > > > > + case 1: > > > > + result = 2; > > > > + break; > > > > + case 2: > > > > + case -19: > > > > > > Can you confirm this -19 really does come from BIOS? Because I suspect > > > it's -ENODEV error code from from one of the functions on the driver side > > > (which is why I asked you to change it into -ENODEV). > > > > Yes it does. It is rather annoying. What happens in this case is that > > `2` is written to the WMI endpoint to turn off the MINI-Led feature, > > this works fine and it is turned off, there are no errors from the write > > at all - verifying the accepted limits in dsdt also shows it is correct. > > > > However, after that, the read fails once. > > Hi, > > I'm left a bit unsure how to interpret your response. If "read fails", it > would indicate that -ENODEV originates from asus_wmi_evaluate_method3(), > asus_wmi_get_devstate() or asus_wmi_get_devstate_bits(), not from BIOS? So > which way it is? > > After reading some more code, I think I figured out the answer myself. > However, that raises another question... So lets now take a step back and > walk through the code: > > Your patch does: > result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); > > asus_wmi_get_devstate_simple() calls asus_wmi_get_devstate_bits() with > ASUS_WMI_DSTS_STATUS_BIT mask that is 0x00000001. > > If there's no error, retval is masked with that ASUS_WMI_DSTS_STATUS_BIT > forcing the return value to 0-1 range so: > > a) I don't think -19 can originate from BIOS but comes from kernel side. > b) How can it ever return 2 (mini-LED off) ????? You're right. *facepalm* *grumble*. Honestly if I were getting paid for this work I'd invest a bit more time in it and catch these silly little things myself. I'll update the code with all feedback, including using a more appropriate WMI function whcih I really should have seen on my own. Thank you for your time so far.
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi index 8a7e25bde085..61a745d2476f 100644 --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi @@ -126,6 +126,14 @@ Description: Change the mini-LED mode: * 0 - Single-zone, * 1 - Multi-zone + * 2 - Multi-zone strong (available on newer generation mini-led) + +What: /sys/devices/platform/<platform>/available_mini_led_mode +Date: Jun 2023 +KernelVersion: 6.10 +Contact: "Luke Jones" <luke@ljones.dev> +Description: + List the available mini-led modes. What: /sys/devices/platform/<platform>/ppt_pl1_spl Date: Jun 2023 diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 18be35fdb381..54ce0fb26f42 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -297,6 +297,7 @@ struct asus_wmi { bool panel_overdrive_available; bool mini_led_mode_available; + u32 mini_led_dev_id; struct hotplug_slot hotplug_slot; struct mutex hotplug_lock; @@ -2109,10 +2110,27 @@ static ssize_t mini_led_mode_show(struct device *dev, struct asus_wmi *asus = dev_get_drvdata(dev); int result; - result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE); - if (result < 0) - return result; + result = asus_wmi_get_devstate_simple(asus, asus->mini_led_dev_id); + /* Remap the mode values to match previous generation mini-led. + * Some BIOSes return -19 instead of 2, which is "mini-LED off", this + * appears to be a BIOS bug. + */ + if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) { + switch (result) { + case 0: + result = 1; + break; + case 1: + result = 2; + break; + case 2: + case -19: + result = 0; + } + } else if (result < 0) { + return result; + } return sysfs_emit(buf, "%d\n", result); } @@ -2129,11 +2147,28 @@ static ssize_t mini_led_mode_store(struct device *dev, if (result) return result; - if (mode > 1) + if (mode > 1 && asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE) return -EINVAL; + if (mode > 2 && asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) + return -EINVAL; + /* + * Remap the mode values so expected behaviour is the same as the last + * generation of mini-LED + */ + if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) { + switch (mode) { + case 0: + mode = 2; + break; + case 1: + mode = 0; + break; + case 2: + mode = 1; + } + } - err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MINI_LED_MODE, mode, &result); - + err = asus_wmi_set_devstate(asus->mini_led_dev_id, mode, &result); if (err) { pr_warn("Failed to set mini-LED: %d\n", err); return err; @@ -2150,6 +2185,23 @@ static ssize_t mini_led_mode_store(struct device *dev, } static DEVICE_ATTR_RW(mini_led_mode); +static ssize_t available_mini_led_mode_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct asus_wmi *asus = dev_get_drvdata(dev); + + switch (asus->mini_led_dev_id) { + case ASUS_WMI_DEVID_MINI_LED_MODE: + return sysfs_emit(buf, "0 1\n"); + case ASUS_WMI_DEVID_MINI_LED_MODE2: + return sysfs_emit(buf, "0 1 2\n"); + } + + return sysfs_emit(buf, "0\n"); +} + +static DEVICE_ATTR_RO(available_mini_led_mode); + /* Quirks *********************************************************************/ static void asus_wmi_set_xusb2pr(struct asus_wmi *asus) @@ -4174,6 +4226,7 @@ static struct attribute *platform_attributes[] = { &dev_attr_nv_temp_target.attr, &dev_attr_panel_od.attr, &dev_attr_mini_led_mode.attr, + &dev_attr_available_mini_led_mode.attr, NULL }; @@ -4496,10 +4549,17 @@ static int asus_wmi_add(struct platform_device *pdev) asus->nv_dyn_boost_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_DYN_BOOST); asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET); asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD); - asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE); asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE) && dmi_match(DMI_BOARD_NAME, "RC71L"); + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE)) { + asus->mini_led_mode_available = true; + asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE; + } else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE2)) { + asus->mini_led_mode_available = true; + asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2; + } + err = fan_boost_mode_check_present(asus); if (err) goto fail_fan_boost_mode; diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h index ab1c7deff118..9cadce10ad9a 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -71,6 +71,7 @@ #define ASUS_WMI_DEVID_LID_FLIP 0x00060062 #define ASUS_WMI_DEVID_LID_FLIP_ROG 0x00060077 #define ASUS_WMI_DEVID_MINI_LED_MODE 0x0005001E +#define ASUS_WMI_DEVID_MINI_LED_MODE2 0x0005002E /* Storage */ #define ASUS_WMI_DEVID_CARDREADER 0x00080013
Support the 2024 mini-led backlight and adjust the related functions to select the relevant dev-id. Also add `available_mini_led_mode` to the platform sysfs since the available mini-led levels can be different. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- .../ABI/testing/sysfs-platform-asus-wmi | 8 ++ drivers/platform/x86/asus-wmi.c | 74 +++++++++++++++++-- include/linux/platform_data/x86/asus-wmi.h | 1 + 3 files changed, 76 insertions(+), 7 deletions(-)