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

On 8/4/26 03:14, Qingshuang Fu wrote:
> 
> At 2026-08-04 17:24:11, [email protected] 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 <[email protected]>
>> 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/[email protected]?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