Re: [PATCH] hwmon: (ads7828) Properly handle -EPROBE_DEFER from devm_regulator_get_optional()

On 8/4/26 19:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The driver masks genuine errors from `devm_regulator_get_optional()` by explicitly only checking for `-EPROBE_DEFER`.
> - [High] The driver fetches an optional external VREF regulator but never calls `regulator_enable()` to turn it on.
> --
> 
> --- Patch [1]: [PATCH] hwmon: (ads7828) Properly handle -EPROBE_DEFER from devm_regulator_get_optional() ---
> commit 78230277b5f05df9416cedbd8b4b4816183523e1
> Author: Qingshuang Fu <[email protected]>
> 
> hwmon: (ads7828) Properly handle -EPROBE_DEFER from devm_regulator_get_optional()
> 
> This commit explicitly checks for -EPROBE_DEFER when calling
> devm_regulator_get_optional() in ads7828_probe(). It ensures the error is
> propagated to trigger probe deferral, rather than incorrectly falling back
> to the internal VREF.
> 
>> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
>> index 149cfcec78dcc..1c4a04cb023c0 100644
>> --- a/drivers/hwmon/ads7828.c
>> +++ b/drivers/hwmon/ads7828.c
>> @@ -133,6 +133,8 @@ static int ads7828_probe(struct i2c_client *client)
>>   			    vref_mv > ADS7828_EXT_VREF_MV_MAX)
>>   				return -EINVAL;
>>   			ext_vref = true;
> 
> [Severity: High]
> This is a pre-existing issue, but does this code need to enable the external
> regulator?
> 
> After retrieving the optional regulator, the driver queries its voltage but
> never calls regulator_enable() to turn it on. If the external regulator is
> not configured as 'always-on' in the device tree, the VREF pin will remain
> unpowered, causing the ADC to return zero or floating conversion results.
> 

This should probably call devm_regulator_get_enable_optional().

>> +		} else if (PTR_ERR(reg) == -EPROBE_DEFER) {
>> +			return PTR_ERR(reg);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this error handling mask
> genuine failures from the regulator core?
> 
> By only propagating -EPROBE_DEFER, legitimate errors from
> devm_regulator_get_optional() like -ENOMEM or -EINVAL are treated as if no
> regulator exists (-ENODEV). This causes the driver to incorrectly fall back
> to the internal VREF.
> 
> If the pin is physically connected to an external regulator, driving it
> internally could create a voltage conflict and lead to hardware damage.
> Should we check for -ENODEV to decide whether to fall back, and propagate
> all other errors?
> 

My point from the other reply.

Guenter