Re: [PATCH 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 3 potential issue(s) to consider:
- [Low] hwmon: registering device outside drivers/hwmon/ violates subsystem layering
- [Low] i2c: i2c_device_id array uses positional initializer instead of named
- [Low] kconfig: unnecessary depends on INPUT for EC_ASUS_GLYMUR config
--

commit 48807c08f64129a6603cbfbeafcd0f00c0ee954d
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 driver to communicate with the custom Embedded
Controller found on ASUS Zenbook A14 and A16 laptops based on Qualcomm
Snapdragon X2 Elite processors. It enables capabilities like fan speed
monitoring, keyboard backlight control, and receiving various sideband events.

> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
[ ... ]
> +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 depend on the input subsystem?

Looking at asus-glymur-ec.c, it does not include linux/input.h, register any
input devices, or call any input APIs. Is it possible to drop this dead
dependency to prevent unnecessarily restricting the build target?

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

[ ... ]

> diff --git a/drivers/platform/arm64/asus-glymur-ec.c b/drivers/platform/arm64/asus-glymur-ec.c
> --- /dev/null
> +++ b/drivers/platform/arm64/asus-glymur-ec.c
[ ... ]
> +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 hardware monitoring device directly from the platform
directory bypass the hwmon subsystem layering rules?

According to the subsystem guidelines, if a chip's main functionality is not
hardware monitoring, its hwmon features should be implemented as an auxiliary
device driver residing inside 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;
> +}
[ ... ]
> +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 it possible to use named initializers for this i2c_device_id array?

The I2C subsystem guidelines require initialized arrays of this type to use the
named format (e.g. { .name = "asus-glymur-ec" }) rather than a positional
initializer.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-topic-asus_ec-v1-0-373516d347ae@oss.qualcomm.com?part=2