[PATCH] platform/x86: asus-wmi: Add quirk for ROG Ally X

[PATCH] platform/x86: asus-wmi: Add quirk for ROG Ally X

[Luke D. Jones]

The new ROG Ally X functions the same as the previus model so we can use
the same method to ensure the MCU USB devices wake and reconnect
correctly.

Given that two devices marks the start of a trend, this patch also adds
a quirk table to make future additions easier if the MCU is the same.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            |  2 +-
 include/linux/platform_data/x86/asus-wmi.h | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 2b968003cb9b..bac2945b0e48 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4694,7 +4694,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
 	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
-						&& dmi_match(DMI_BOARD_NAME, "RC71L");
+						&& dmi_check_system(asus_ally_mcu_quirk);
 
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
 		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 74b32e1d6735..fba9751cda5b 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -196,4 +196,19 @@ static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
 	{ },
 };
 
+/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
+static const struct dmi_system_id asus_ally_mcu_quirk[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
+		},
+	},
+	{ },
+};
+
 #endif	/* __PLATFORM_DATA_X86_ASUS_WMI_H */
-- 
2.45.2

Re: [PATCH] platform/x86: asus-wmi: Add quirk for ROG Ally X

[Ilpo Järvinen]

On Thu, 25 Jul 2024, Luke D. Jones wrote:

> The new ROG Ally X functions the same as the previus model so we can use
> the same method to ensure the MCU USB devices wake and reconnect
> correctly.
> 
> Given that two devices marks the start of a trend, this patch also adds
> a quirk table to make future additions easier if the MCU is the same.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            |  2 +-
>  include/linux/platform_data/x86/asus-wmi.h | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 2b968003cb9b..bac2945b0e48 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -4694,7 +4694,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
>  	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
>  	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> -						&& dmi_match(DMI_BOARD_NAME, "RC71L");
> +						&& dmi_check_system(asus_ally_mcu_quirk);
>  
>  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
>  		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 74b32e1d6735..fba9751cda5b 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -196,4 +196,19 @@ static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
>  	{ },
>  };
>  
> +/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
> +static const struct dmi_system_id asus_ally_mcu_quirk[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> +		},
> +	},
> +	{ },
> +};

Why is this in the header? I see your comment but this means every file 
which includes asus-wmi.h will have a duplicate copy of this array.


-- 
 i.

Re: [PATCH] platform/x86: asus-wmi: Add quirk for ROG Ally X

[Hans de Goede]

Hi,

On 7/29/24 1:44 PM, Ilpo Järvinen wrote:
> On Thu, 25 Jul 2024, Luke D. Jones wrote:
> 
>> The new ROG Ally X functions the same as the previus model so we can use
>> the same method to ensure the MCU USB devices wake and reconnect
>> correctly.
>>
>> Given that two devices marks the start of a trend, this patch also adds
>> a quirk table to make future additions easier if the MCU is the same.
>>
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>> ---
>>  drivers/platform/x86/asus-wmi.c            |  2 +-
>>  include/linux/platform_data/x86/asus-wmi.h | 15 +++++++++++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index 2b968003cb9b..bac2945b0e48 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -4694,7 +4694,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>>  	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
>>  	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
>>  	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
>> -						&& dmi_match(DMI_BOARD_NAME, "RC71L");
>> +						&& dmi_check_system(asus_ally_mcu_quirk);
>>  
>>  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
>>  		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>> index 74b32e1d6735..fba9751cda5b 100644
>> --- a/include/linux/platform_data/x86/asus-wmi.h
>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>> @@ -196,4 +196,19 @@ static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
>>  	{ },
>>  };
>>  
>> +/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
>> +static const struct dmi_system_id asus_ally_mcu_quirk[] = {
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
>> +		},
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
>> +		},
>> +	},
>> +	{ },
>> +};
> 
> Why is this in the header? I see your comment but this means every file 
> which includes asus-wmi.h will have a duplicate copy of this array.

You are right, although asus-wmi.h is not included that often it is only
used in:

drivers/platform/x86/asus-wmi.c
drivers/platform/x86/eeepc-wmi.c
drivers/platform/x86/asus-nb-wmi.c
drivers/hid/hid-asus.c

But all 4 of those usually get build as separate modules, so it would
still be better to move this into a .c file.

Luke, the comment says that this will be used in hid-asus.c too, but
I think that is a copy and paste error resulting froom copying the comment
from the asus_use_hid_led_dmi_ids[] DMI matches array.

It seems that this DMI matches array will only be used inside asus-wmi.c,
in that case it would be better to just declare it there instead of
in the shared asus-wmi.h file ?

Regards,

Hans