[PATCH v2 0/1] asus-wmi: add support variant of TUF RGB

[PATCH v2 0/1] asus-wmi: add support variant of TUF RGB

[Luke D. Jones]

Changelog:
- v2
  - remove unrequired kbd_rgb_mode_available check
-v1
  - add missing define for WMI method

Luke D. Jones (1):
  platform/x86: asus-wmi: add support variant of TUF RGB

 drivers/platform/x86/asus-wmi.c            | 13 +++++++++++--
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.44.0

[PATCH v2 1/1] platform/x86: asus-wmi: add support variant of TUF RGB

[Luke D. Jones]

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
@@ -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
-- 
2.44.0

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support variant of TUF RGB

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

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.

-- 
 i.

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support variant of TUF RGB

[Luke Jones]

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.