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

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

[Qingshuang Fu]

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

[sashiko-bot]

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

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

[Qingshuang Fu]

At 2026-08-04 17:24:11, sashiko-bot@kernel.org wrote:
>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


Thanks for the review.

The mishandling of -EPROBE_DEFER from devm_regulator_get_optional() is a pre-existing issue and is not addressed by this patch. This change only adds error checking for the dummy regmap_read() call.

Qingshuang Fu

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

[Guenter Roeck]

On 8/4/26 03:14, Qingshuang Fu wrote:
> 
> At 2026-08-04 17:24:11, sashiko-bot@kernel.org wrote:
>> 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
> 
> 
> Thanks for the review.
> 
> The mishandling of -EPROBE_DEFER from devm_regulator_get_optional() is a pre-existing issue and is not addressed by this patch. This change only adds error checking for the dummy regmap_read() call.
> 

In other words, it fixes a non-problem while ignoring the real one.

Guenter

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@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