Re: [PATCH v3 2/3] 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 2 potential issue(s) to consider:
- [Low] hwmon: direct registration in platform driver violates subsystem layering
- [Low] kconfig: unnecessary dependency on INPUT subsystem
--

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

platform: arm64: Add a driver for the EC found on ASUS Glymur machines

This commit introduces a new platform driver for the EC found on ASUS Glymur
machines. It allows for fan speed reporting, keyboard backlight setting, and
receiving 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]
Is this dependency on the INPUT subsystem necessary? The driver source code
does not appear to include linux/input.h, register any input devices, or
use any input APIs, which restricts build combinations unnecessarily.

> +	depends on PM
> +	depends on THERMAL || THERMAL=n
> +	select NEW_LEDS
> +	select LEDS_CLASS
> +	help
> +	  Say M or Y here to enable the EC driver for the Qualcomm Snapdragon-
> +	  based ASUS Zenbook A16 (UX3607OA) and similar machines. It provides
> +	  temperature sensors readout, fan monitoring, keyboard and EC event
> +	  reporting.
> +
>  config EC_HUAWEI_GAOKUN
>  	tristate "Huawei Matebook E Go Embedded Controller driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/platform/arm64/asus-glymur-ec.c b/drivers/platform/arm64/asus-glymur-ec.c
> new file mode 100644
> index 0000000000000..3e7706e3bdaac
> --- /dev/null
> +++ b/drivers/platform/arm64/asus-glymur-ec.c
[ ... ]
> @@ -0,0 +1,600 @@
[ ... ]
> +static int asus_glymur_ec_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ec->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +							     "asus_glymur_ec",
> +							     ec,
> +							     &asus_ec_hwmon_chip_info,
> +							     NULL);

[Severity: Low]
Does registering the hwmon device directly from this platform driver bypass the
hwmon subsystem architecture rules?

According to the hwmon subsystem guidelines, if a multifunction device supports
hardware monitoring as a secondary functionality, its hardware monitoring
features should be implemented as an auxiliary driver located in
drivers/hwmon/.

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

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