Re: [PATCH] hwmon: (ads7828) Check return value of regmap_read() in probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver ignores `-EPROBE_DEFER` and other errors from `devm_regulator_get_optional()`, improperly falling back to the internal VREF.
--

--- Patch [1]: [PATCH] hwmon: (ads7828) Check return value of regmap_read() in probe ---
commit 2a7377992af717bb6d251109d28baf026bc4a665
Author: Qingshuang Fu <fuqingshuang@kylinos.cn>
Subject: hwmon: (ads7828) Check return value of regmap_read() in probe

This patch checks the return value of a dummy regmap_read() used to enable
the internal reference voltage in ads7828_probe(). If the read fails, it
logs an error and aborts the probe instead of silently failing and
registering a non-functional device.

> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index 149cfcec78dcc..91ef0c4a08c4e 100644
> --- a/drivers/hwmon/ads7828.c
> +++ b/drivers/hwmon/ads7828.c
> @@ -112,6 +112,7 @@ static int ads7828_probe(struct i2c_client *client)
>  	unsigned int regval;
>  	enum ads7828_chips chip;
>  	struct regulator *reg;
> +	int ret;
>  
>  	data = devm_kzalloc(dev, sizeof(struct ads7828_data), GFP_KERNEL);
>  	if (!data)

[Severity: High]
This is a pre-existing issue, but does the driver mishandle errors like
-EPROBE_DEFER from devm_regulator_get_optional()?

Looking slightly further down in ads7828_probe() in drivers/hwmon/ads7828.c:

    } else if (dev->of_node) {
        diff_input = of_property_read_bool(dev->of_node,
                                           "ti,differential-input");
        reg = devm_regulator_get_optional(dev, "vref");
        if (!IS_ERR(reg)) {
            vref_uv = regulator_get_voltage(reg);

If devm_regulator_get_optional() returns -EPROBE_DEFER because the
regulator driver has not yet completed probing, the driver ignores the
error. It proceeds as if there is no external reference and incorrectly
falls back to enabling the internal reference.

If an external voltage reference is physically connected and specified in
the device tree, could driving the internal reference onto the same pin
result in incorrect ADC readings or potentially hardware damage due to an
electrical conflict?

[ ... ]

> @@ -166,8 +167,13 @@ static int ads7828_probe(struct i2c_client *client)
>  	 * voltage needs to settle before getting valid ADC data. So perform a
>  	 * dummy read to enable the internal reference voltage.
>  	 */
> -	if (!ext_vref)
> -		regmap_read(data->regmap, data->cmd_byte, &regval);
> +	if (!ext_vref) {
> +		ret = regmap_read(data->regmap, data->cmd_byte, &regval);
> +		if (ret) {
> +			dev_err(dev, "dummy read failed to enable internal VREF: %d\n", ret);
> +			return ret;
> +		}
> +	}
>  
>  	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>  							   data,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804091719.651479-1-fffsqian@163.com?part=1