| Message ID | 20240724222852.44378-1-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Ilpo Järvinen |
| Headers | show |
| Series | platform/x86: asus-wmi: Add quirk for ROG Ally X | expand |
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.
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
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 */
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(-)