Re: [PATCH v2 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:
- [Medium] hwmon: registering hwmon device directly from platform driver violates layering
- [Low] pm: incorrect wrapper pm_sleep_ptr() used for dev_pm_ops structure pointer
--

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

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

This commit adds a driver for the embedded controller found on ASUS
Zenbook A14 and A16 laptops. It exposes fan monitoring, temperature
sensor readouts, keyboard backlight controls, and event reporting over I2C.

> diff --git a/drivers/platform/arm64/asus-glymur-ec.c b/drivers/platform/arm64/asus-glymur-ec.c
> new file mode 100644
> index 0000000000000..dec9b137b820d
> --- /dev/null
> +++ b/drivers/platform/arm64/asus-glymur-ec.c
[ ... ]
> @@ -546,12 +546,12 @@ static int asus_glymur_ec_probe(struct i2c_client *client)
>  	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]
Does registering the hwmon device directly from this platform driver violate
the hwmon subsystem layering rules? The hardware monitoring subsystem guidelines
state that if the main functionality of a chip is not hardware monitoring, its
hardware monitoring functionality should be implemented as an auxiliary device
driver, and the hardware monitoring driver should reside 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;
>  }
[ ... ]
> @@ -578,11 +578,11 @@ static const struct i2c_device_id asus_glymur_ec_id[] = {
>  	{ "asus-glymur-ec" },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, asus_glymur_ec_id);
>  
>  static struct i2c_driver asus_glymur_ec_driver = {
>  	.driver = {
>  		.name		= "asus-glymur-ec",
>  		.of_match_table	= asus_glymur_ec_of_match,
>  		.pm		= pm_sleep_ptr(&asus_glymur_ec_pm_ops),

[Severity: Low]
Is pm_sleep_ptr() the correct wrapper for the dev_pm_ops structure pointer?
The PM subsystem guide indicates that pm_ptr() should be used for wrapping
dev_pm_ops structure pointers, while pm_sleep_ptr() is meant for wrapping
individual sleep callbacks. This may result in dead code or warnings when PM
support configurations vary.

>  	},
>  	.probe		= asus_glymur_ec_probe,
>  	.id_table	= asus_glymur_ec_id,
>  };

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