Re: [PATCH v4 2/4] platform: arm64: Add a driver for the EC found on ASUS Glymur machines

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] hwmon: Subsystem layering violation by directly registering hwmon device from platform driver
- [Low] i2c: Missing named initializer in struct i2c_device_id array
- [Low] leds: Hardcoded LED name bypasses Device Tree color/function properties
- [Low] kconfig: Unnecessary INPUT dependency in Kconfig
--

commit 084ff1f8f9529e75d60a737e61f5d594cff0bfe1
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

platform: arm64: Add a driver for the EC found on ASUS Glymur machines
    
Some Qualcomm Snapdragon X2 Elite (Extreme)-based ASUS laptops (at
least the Zenbook A14 and A16) expose a custom, somewhat
Qualcomm reference-derived EC interface. Introduce a driver to
communicate with said EC, allowing for fan speed reporting, keyboard
backlight setting and receiving of a number of miscellaneous sideband
events.

> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index e32e01b2a9bdd..84c475c62990b 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -33,6 +33,22 @@ config EC_ACER_ASPIRE1
>  	  laptop where this information is not properly exposed via the
>  	  standard ACPI devices.
>  
> +config EC_ASUS_GLYMUR
> +	tristate "ASUS Zenbook A16 (Qualcomm) Embedded Controller driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on I2C
> +	depends on HWMON
> +	depends on INPUT
                   ^^^^^

[Severity: Low]
Does this driver actually need to depend on INPUT?

The driver does not appear to include <linux/input.h> or use any input
subsystem APIs (the hotkey event is only logged via dev_dbg_ratelimited).
Could this dependency be dropped to avoid restricting the driver from being
built on systems without input support unnecessarily?

> +	depends on PM
> +	depends on THERMAL || THERMAL=n
> +	select NEW_LEDS
> +	select LEDS_CLASS

[ ... ]

> diff --git a/drivers/platform/arm64/asus-glymur-ec.c b/drivers/platform/arm64/asus-glymur-ec.c
> new file mode 100644
> index 0000000000000..532610592d1fd
> --- /dev/null
> +++ b/drivers/platform/arm64/asus-glymur-ec.c

[ ... ]

> +static int asus_glymur_ec_probe(struct i2c_client *client)
> +{

[ ... ]

> +	dev_dbg(dev, "ASUS Zenbook EC version %u.%u.%u\n", major, minor, patch);
> +
> +	ec->kbd_led.name = "asus::kbd_backlight";
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Could this LED name be defined via Device Tree color and function properties
instead?

The LED subsystem guidelines prefer using DT `color` and `function` properties
to define LEDs, rather than deprecated fixed labels, to enable standard system
layouts like `<color>:<function>`. Hardcoding the name here embeds the naming
scheme into the driver and prevents standard property extraction from the
device tree.

> +	ec->kbd_led.max_brightness = ASUS_EC_KBD_MAX_BRIGHTNESS;
> +	ec->kbd_led.brightness_set_blocking = asus_ec_kbd_led_set;
> +	ec->kbd_led.flags = LED_CORE_SUSPENDRESUME;
> +
> +	ret = devm_led_classdev_register(dev, &ec->kbd_led);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register keyboard LED\n");
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL, asus_ec_irq,
> +					IRQF_ONESHOT, dev_name(dev), ec);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> +
> +	ec->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +							     "asus_glymur_ec",
> +							     ec,
> +							     &asus_ec_hwmon_chip_info,
> +							     NULL);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Should this hwmon functionality be implemented as an auxiliary device driver
residing in drivers/hwmon/ instead?

The HWMON subsystem guidelines state that if the main functionality of a chip
is not hardware monitoring (such as an Embedded Controller), its hardware
monitoring functionality must be implemented as an auxiliary device driver
residing in drivers/hwmon/.

Directly calling devm_hwmon_device_register_with_info() from a platform driver
like this is a structural layering violation that bypasses HWMON maintainer
review.

> +	if (IS_ERR(ec->hwmon_dev)) {
> +		return dev_err_probe(dev, PTR_ERR(ec->hwmon_dev),
> +				     "Failed to register hwmon\n");
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static const struct of_device_id asus_glymur_ec_of_match[] = {
> +	{ .compatible = "asus,zenbook-a14-ux3407na-ec" },
> +	{ .compatible = "asus,zenbook-a16-ux3607oa-ec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, asus_glymur_ec_of_match);
> +
> +static const struct i2c_device_id asus_glymur_ec_id[] = {
> +	{ "asus-glymur-ec" },
          ^^^^^^^^^^^^^^^^

[Severity: Low]
Is this missing a named initializer for the i2c_device_id?

The I2C subsystem guidelines require that initialized arrays of type
struct i2c_device_id use named initializers, such as:
	{ .name = "asus-glymur-ec" },

> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, asus_glymur_ec_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-topic-asus_ec-v4-0-af6305ffcb10@oss.qualcomm.com?part=2