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

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

[Qingshuang Fu]

From: Qingshuang Fu <fuqingshuang@kylinos.cn>

When devm_regulator_get_optional() returns -EPROBE_DEFER, the driver
currently ignores the error and incorrectly falls back to internal VREF,
even if an external vref is defined in device tree.

This could lead to conflicting voltage sources on the VREF pin and
inaccurate ADC readings.

Check for -EPROBE_DEFER explicitly and return it to trigger probe deferral.
Treat other errors (such as -ENODEV) as "no external vref" and use internal
reference.

Fixes: a8ddfea09566 ("hwmon: (ads7828) Accept optional parameters from device tree")
Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
---
 drivers/hwmon/ads7828.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
index 149cfcec78dc..1c4a04cb023c 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;
+		} else if (PTR_ERR(reg) == -EPROBE_DEFER) {
+			return PTR_ERR(reg);
 		}
 	}
 

base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
-- 
2.25.1

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

[sashiko-bot]

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 <fuqingshuang@kylinos.cn>

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.

> +		} 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?

>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805020506.958722-1-fffsqian@163.com?part=1

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

[Guenter Roeck]

On 8/4/26 19:05, Qingshuang Fu wrote:
> From: Qingshuang Fu <fuqingshuang@kylinos.cn>
> 
> When devm_regulator_get_optional() returns -EPROBE_DEFER, the driver
> currently ignores the error and incorrectly falls back to internal VREF,
> even if an external vref is defined in device tree.
> 
> This could lead to conflicting voltage sources on the VREF pin and
> inaccurate ADC readings.
> 
> Check for -EPROBE_DEFER explicitly and return it to trigger probe deferral.
> Treat other errors (such as -ENODEV) as "no external vref" and use internal
> reference.
> 

Only -ENODEV means "no external vref". All other errors should trigger an error return.

Guenter


> Fixes: a8ddfea09566 ("hwmon: (ads7828) Accept optional parameters from device tree")
> Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
> ---
>   drivers/hwmon/ads7828.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index 149cfcec78dc..1c4a04cb023c 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;
> +		} else if (PTR_ERR(reg) == -EPROBE_DEFER) {
> +			return PTR_ERR(reg);
>   		}
>   	}
>   
> 
> base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86

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

[Guenter Roeck]

On 8/4/26 19:11, sashiko-bot@kernel.org 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 <fuqingshuang@kylinos.cn>
> 
> 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