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

[Guenter Roeck]

On 8/4/26 02:17, Qingshuang Fu wrote:
> From: Qingshuang Fu <fuqingshuang@xxxxxxxxxx>
>
> ads7828_probe() issues a dummy regmap_read() to enable the internal
> reference voltage when ext_vref is false. The original code ignores the
> return value of regmap_read().
>
> If the I2C read fails, the internal reference voltage will not be enabled,
> and subsequent ADC readings return incorrect values without any error
> indication.

This only applies to ADS7830, and the command does not have to succeed for
this to work; the chip only needs to see a start/stop sequence.
Worst case, the next command would enable it.

Given that, I do not think this is a real problem (much less one requiring a stable
backport), and I am not going to apply this patch.

> Check the return value of regmap_read(), log an error message, and abort
> probe on failure. This prevents registering a non-functional hwmon device.

FWIW, that logic would apply to every single driver which doesn't access the
chip as part of the probe function. I don't even want to get there.

Guenter

> Fixes: 7a18afe80977 ("hwmon: (ads7828) Enable internal reference")
> Signed-off-by: Qingshuang Fu <fuqingshuang@xxxxxxxxxx>
> ---
>   drivers/hwmon/ads7828.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index 149cfcec78dc..91ef0c4a08c4 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)
> @@ -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,
>
> base-commit: 848acc8ffe1b7cd5f1bf427b93069becfebc2c9d