Hi Luke,
Thank you for the new version, much better.
On 5/29/24 3:28 AM, Luke D. Jones wrote:
> On almost all ASUS ROG series laptops the MCU used for the USB keyboard
> also has a HID packet used for setting the brightness. This is usually
> the same as the WMI method. But in some laptops the WMI method either
> is missing or doesn't work, so we should default to the HID control.
>
> Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
> ---
> drivers/hid/hid-asus.c | 6 ++++
> drivers/platform/x86/asus-wmi.c | 35 +++++++++++++++++++++-
> include/linux/platform_data/x86/asus-wmi.h | 10 +++++++
> 3 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 02de2bf4f790..4cba8e143031 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -492,12 +492,18 @@ static void asus_kbd_backlight_work(struct work_struct *work)
> */
> static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
> {
> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> u32 value;
> int ret;
>
> if (!IS_ENABLED(CONFIG_ASUS_WMI))
> return false;
>
> + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD && asus_use_hid_led()) {
> + hid_info(hdev, "using HID for asus::kbd_backlight\n");
> + return false;
> + }
> +
> ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS,
> ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, &value);
> hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value);
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 3f9b6285c9a6..54cb07c79fcf 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -144,6 +144,15 @@ module_param(fnlock_default, bool, 0444);
>
> static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
>
> +static const char * const use_hid_led_matches[] = {
> + "ROG Zephyrus",
> + "ROG Strix",
> + "ROG Flow",
> + "GA403",
> + "GU605",
> + "RC71L",
> +};
> +
> static int throttle_thermal_policy_write(struct asus_wmi *);
>
> static bool ashs_present(void)
> @@ -1642,6 +1651,29 @@ static int micmute_led_set(struct led_classdev *led_cdev,
> return err < 0 ? err : 0;
> }
>
> +bool asus_use_hid_led(void)
> +{
> + const char *product, *board;
> + int i;
> +
> + product = dmi_get_system_info(DMI_PRODUCT_FAMILY);
> + if (!product)
> + return false;
> +
> + board = dmi_get_system_info(DMI_BOARD_NAME);
> + if (!board)
> + return false;
> +
> + for (i = 0; i < ARRAY_SIZE(use_hid_led_matches); i++) {
> + if (strstr(product, use_hid_led_matches[i]))
> + return true;
> + if (strstr(board, use_hid_led_matches[i]))
> + return true;
> + }
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(asus_use_hid_led);
> +
With the new helper in this form you are pretty much re-inventing
dmi_check_system().
So IMHO you should just replace use_hid_led_matches[] with
a dmi_system_id array and simplify asus_use_hid_led() to just
a single "dmi_check_system(asus_use_hid_led_dmi_ids)" call.
Using dmi_system_id-s also allows you to specify if the string
which is being matched should be matched against DMI_PRODUCT_FAMILY
or DMI_BOARD_NAME. Note the normal DMI_MATCH() macro to fill in
dmi_system_id array entries does non exact matching using strstr()
just like you do above.
Note you need to terminate the dmi_system_id array with an empty
{} entry.
> static void asus_wmi_led_exit(struct asus_wmi *asus)
> {
> led_classdev_unregister(&asus->kbd_led);
> @@ -1681,7 +1713,8 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> goto error;
> }
>
> - if (!kbd_led_read(asus, &led_val, NULL)) {
> + if (!kbd_led_read(asus, &led_val, NULL) && !asus_use_hid_led()) {
> + pr_info("using asus-wmi for asus::kbd_backlight\n");
> asus->kbd_led_wk = led_val;
> asus->kbd_led.name = "asus::kbd_backlight";
> asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 3eb5cd6773ad..6833035f7006 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -160,4 +160,14 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> }
> #endif
>
> +/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
> +#if IS_ENABLED(CONFIG_ASUS_WMI)
> +bool asus_use_hid_led(void);
> +#else
> +static inline bool asus_use_hid_led(void)
> +{
> + return true;
> +}
> +#endif
> +
> #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
Regards,
Hans
