hwmon: (ads7828) Check return value of regmap_read() in probe

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

Posted by Qingshuang Fu 1 month, 4 weeks ago

From: Qingshuang Fu <fuqingshuang@kylinos.cn>

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.

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

Fixes: 7a18afe80977 ("hwmon: (ads7828) Enable internal reference")
Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
---
 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
-- 
2.25.1

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

Posted by Guenter Roeck 1 month, 4 weeks ago

On 8/4/26 02:17, Qingshuang Fu wrote:
> From: Qingshuang Fu <fuqingshuang@kylinos.cn>
> 
> 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@kylinos.cn>
> ---
>   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