Re: [PATCH v3] hwmon: (ads7828) Fix external VREF regulator handling

[Guenter Roeck]

On 8/4/26 21:21, Qingshuang Fu wrote:
> From: Qingshuang Fu <fuqingshuang@xxxxxxxxxx>
>
> The driver currently has two issues with the external VREF regulator
> handling in ads7828_probe():
>
> 1. All errors from devm_regulator_get_optional() are ignored, causing the
>     driver to incorrectly fall back to internal VREF even for transient
>     errors like -EPROBE_DEFER or genuine failures like -ENOMEM.
>
> 2. The external regulator is never enabled. The driver calls
>     regulator_get_voltage() without first calling regulator_enable(),
>     so the VREF pin may remain unpowered if the regulator is not
>     configured as always-on.
>
> Fix both issues by switching to devm_regulator_get_enable_read_voltage(),
> which handles regulator get, enable, and voltage read in one call.
> Only -ENODEV (no regulator specified in device tree) should trigger the
> fallback to internal VREF. All other errors are propagated to the caller.
>
> Fixes: a8ddfea09566 ("hwmon: (ads7828) Accept optional parameters from device tree")
> Signed-off-by: Qingshuang Fu <fuqingshuang@xxxxxxxxxx>
> ---
> Changes in v3:
> - Switch to devm_regulator_get_enable_read_voltage() to also enable the
>    external regulator, as suggested by Guenter Roeck.
> Changes in v2:
> - Broaden the error check to handle all errors except -ENODEV, instead of
>    only checking for -EPROBE_DEFER. This addresses the Sashiko AI review
>    concern about masking genuine errors like -ENOMEM and -EINVAL.
>
>   drivers/hwmon/ads7828.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index 149cfcec78dc..172da6f6549d 100644
> --- a/drivers/hwmon/ads7828.c
> +++ b/drivers/hwmon/ads7828.c
> @@ -106,12 +106,11 @@ static int ads7828_probe(struct i2c_client *client)
>   	struct ads7828_data *data;
>   	struct device *hwmon_dev;
>   	unsigned int vref_mv = ADS7828_INT_VREF_MV;
> -	unsigned int vref_uv;
> +	int vref_uv;
>   	bool diff_input = false;
>   	bool ext_vref = false;
>   	unsigned int regval;
>   	enum ads7828_chips chip;
> -	struct regulator *reg;
>   
>   	data = devm_kzalloc(dev, sizeof(struct ads7828_data), GFP_KERNEL);
>   	if (!data)
> @@ -125,14 +124,15 @@ static int ads7828_probe(struct i2c_client *client)
>   	} 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);
> +		vref_uv = devm_regulator_get_enable_read_voltage(dev, "vref");
> +		if (vref_uv >= 0) {

Please use
		if (IS_ERR(vref_uv)) {
			if (vref_uv != -ENODEV)
				return vref_uv;
		} else {
			...

Thanks,
Guenter

>   			vref_mv = DIV_ROUND_CLOSEST(vref_uv, 1000);
>   			if (vref_mv < ADS7828_EXT_VREF_MV_MIN ||
>   			    vref_mv > ADS7828_EXT_VREF_MV_MAX)
>   				return -EINVAL;
>   			ext_vref = true;
> +		} else if (vref_uv != -ENODEV) {
> +			return vref_uv;
>   		}
>   	}
>