| Message ID | 20240320011442.11608-2-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | asus-wmi: add support variant of TUF RGB | expand |
On Wed, 20 Mar 2024, Luke D. Jones wrote: > Adds support for a second TUF RGB wmi call that some versions of the TUF > laptop come with. Also adjusts existing support to select whichever is > available. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > drivers/platform/x86/asus-wmi.c | 13 +++++++++++-- > include/linux/platform_data/x86/asus-wmi.h | 1 + > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index b9a2fb8007c0..0d8a2b82cc06 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -4544,6 +4545,14 @@ static int asus_wmi_add(struct platform_device *pdev) > asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO; > } > > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { The patch itself is fine, Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> However, There's a major problem in the way you're submitting these. This patch is built on top of the GPU_MUX_VIVO patch as can be seen from the context above. Yet, you're sending these independently instead of series. I suspect there are other similar problems among these patches that there's hidden dependency order in which these should be applied. This will cause problems if maintainer applies the patches in wrong order (they may even misapply with fuzz). Only if the patches are truly independent, that is, focus on solving entirely differently thing (functional independency) and do not have any linewise conflicts (code locality independecy) either, it's fine to send patches as independent ones without making a series out of them. But clearly it's not the case here.
On Thu, 21 Mar 2024, at 12:36 AM, Ilpo Järvinen wrote: > On Wed, 20 Mar 2024, Luke D. Jones wrote: > > > Adds support for a second TUF RGB wmi call that some versions of the TUF > > laptop come with. Also adjusts existing support to select whichever is > > available. > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > --- > > drivers/platform/x86/asus-wmi.c | 13 +++++++++++-- > > include/linux/platform_data/x86/asus-wmi.h | 1 + > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > > index b9a2fb8007c0..0d8a2b82cc06 100644 > > --- a/drivers/platform/x86/asus-wmi.c > > +++ b/drivers/platform/x86/asus-wmi.c > > > @@ -4544,6 +4545,14 @@ static int asus_wmi_add(struct platform_device *pdev) > > asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO; > > } > > > > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { > > The patch itself is fine, > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > However, > > There's a major problem in the way you're submitting these. This patch is > built on top of the GPU_MUX_VIVO patch as can be seen from the context > above. Yet, you're sending these independently instead of series. I > suspect there are other similar problems among these patches that there's > hidden dependency order in which these should be applied. This will cause > problems if maintainer applies the patches in wrong order (they may even > misapply with fuzz). > > Only if the patches are truly independent, that is, focus on solving > entirely differently thing (functional independency) and do not have any > linewise conflicts (code locality independecy) either, it's fine to send > patches as independent ones without making a series out of them. But > clearly it's not the case here. Honestly, yeah I should have made them a series. I was sick at the time of submission and shouldn't have been near a computer at all but I have a long backlog. I'll go through your other reviews and then turn the lot in as a series to prevent any mishaps.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index b9a2fb8007c0..0d8a2b82cc06 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -280,6 +280,7 @@ struct asus_wmi { bool nv_temp_tgt_available; bool kbd_rgb_mode_available; + u32 kbd_rgb_dev; bool kbd_rgb_state_available; bool throttle_thermal_policy_available; @@ -870,6 +871,7 @@ static ssize_t kbd_rgb_mode_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { + struct asus_wmi *asus = dev_get_drvdata(dev); u32 cmd, mode, r, g, b, speed; int err; @@ -906,7 +908,7 @@ static ssize_t kbd_rgb_mode_store(struct device *dev, speed = 0xeb; } - err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE, + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, asus->kbd_rgb_dev, cmd | (mode << 8) | (r << 16) | (g << 24), b | (speed << 8), NULL); if (err) return err; @@ -4515,7 +4517,6 @@ static int asus_wmi_add(struct platform_device *pdev) asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU); asus->egpu_connect_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU_CONNECTED); asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU); - asus->kbd_rgb_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE); asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE); asus->ppt_pl2_sppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL2_SPPT); asus->ppt_pl1_spl_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL1_SPL); @@ -4544,6 +4545,14 @@ static int asus_wmi_add(struct platform_device *pdev) asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO; } + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { + asus->kbd_rgb_mode_available = true; + asus->kbd_rgb_dev = ASUS_WMI_DEVID_TUF_RGB_MODE; + } else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE2)) { + asus->kbd_rgb_mode_available = true; + asus->kbd_rgb_dev = ASUS_WMI_DEVID_TUF_RGB_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 b48b024dd844..3e9a01467c67 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -132,6 +132,7 @@ /* TUF laptop RGB modes/colours */ #define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056 +#define ASUS_WMI_DEVID_TUF_RGB_MODE2 0x0010005A /* TUF laptop RGB power/state */ #define ASUS_WMI_DEVID_TUF_RGB_STATE 0x00100057
Adds support for a second TUF RGB wmi call that some versions of the TUF laptop come with. Also adjusts existing support to select whichever is available. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 13 +++++++++++-- include/linux/platform_data/x86/asus-wmi.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-)