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

[Hans de Goede <hdegoede@redhat.com>]

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