Re: [RFC PATCH v2 1/2] platform: arm64: qcom-hamoa-ec: monitor Slim 7x legacy sensors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Hardware monitoring device is registered directly from a platform driver, violating HWMON subsystem layering rules.
- [Low] The modified struct i2c_device_id array does not use named initializers.

Pre-existing issues:
- [High] Missing unwind for qcom_ec_sci_evt_control on probe error paths.
--

commit f56d14065bf54cf3279988ab062adb53bc90787e
Author: Birk Skyum <birk.skyum@pm.me>

platform: arm64: qcom-hamoa-ec: monitor Slim 7x legacy sensors

This commit adds monitoring for legacy sensors on the Yoga Slim 7x EC.
It exposes a read-only path for the Lenovo compatible device to expose fan
RPM and thermistor via hwmon. The strict capability validation on reference
boards is kept unchanged.

Link: https://github.com/aarch64-laptops/build/blob/2e58842f5fa2f87771c2df017ae4d8c65225ef10/misc/lenovo-yoga-slim-7x/acpi/dsdt.dsl
Link: https://lkml.iu.edu/hypermail/linux/kernel/2607.3/08806.html

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index 4d2ad042a7f84..3a4ca8a0de5c8 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c

[ ... ]

> +static int qcom_ec_hwmon_probe(struct qcom_ec *ec)
> +{
> +	struct device *dev = &ec->client->dev;
> +	struct device *hwmon;
> +	long rpm;
> +	int ret;
> +
> +	if (!i2c_check_functionality(ec->client->adapter,
> +				     I2C_FUNC_I2C | I2C_FUNC_SMBUS_READ_BYTE_DATA))
> +		return -EOPNOTSUPP;
> +
> +	ret = qcom_ec_read_fan_rpm(ec, &rpm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read fan RPM\n");
> +
> +	hwmon = devm_hwmon_device_register_with_info(dev, "qcom_ec", ec,
> +						     &qcom_ec_hwmon_chip_info, NULL);

[Severity: Low]
Does this code register a hardware monitoring device directly from a platform
driver? The subsystem guidelines explicitly require hwmon functionality of
multi-function chips to be implemented as an auxiliary device driver in
drivers/hwmon/ rather than directly in drivers/platform/arm64/.

> +	return PTR_ERR_OR_ZERO(hwmon);
> +}

[ ... ]

> @@ -356,14 +481,24 @@ static int qcom_ec_probe(struct i2c_client *client)

[ ... ]

>  	ret = qcom_ec_sci_evt_control(dev, true);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to enable SCI events\n");
>  
>  	ret = qcom_ec_thermal_capabilities(dev);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to read thermal capabilities\n");
>  
>  	if (ec->thermal_cap.fan_cnt == 0) {
>  		dev_warn(dev, FW_BUG "Failed to get fan count, firmware update required\n");
>  		return 0;
>  	}
>  
>  	ec->ec_cdev = devm_kcalloc(dev, ec->thermal_cap.fan_cnt, sizeof(*ec->ec_cdev), GFP_KERNEL);
>  	if (!ec->ec_cdev)
>  		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this code leave SCI events enabled on
error paths during probe? If qcom_ec_thermal_capabilities() or devm_kcalloc()
fail in qcom_ec_probe() after qcom_ec_sci_evt_control(dev, true) is called,
it returns an error without unwinding the EC SCI events configuration. 
Could this leave the hardware generating unhandled SCI interrupts for a driver
that was not successfully loaded?

[ ... ]

> @@ -421,14 +559,21 @@ static void qcom_ec_remove(struct i2c_client *client)

[ ... ]

>  static const struct i2c_device_id qcom_ec_i2c_id_table[] = {
> -	{ "qcom-hamoa-ec", },
> +	{ "qcom-hamoa-ec", (kernel_ulong_t)&qcom_ec_reference },

[Severity: Low]
Does this array element fail to use named initializers for struct
i2c_device_id? The I2C subsystem guidelines mandate that initialized arrays of
this type must use named initializers (e.g., .name = ..., .driver_data = ...).

>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, qcom_ec_i2c_id_table);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908135801.81436-1-birk.skyum@pm.me?part=1