Re: [PATCH v3 5/7] iio: adc: ad7625: move initial mode setup

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 6 Nov 2026 18:05:17 +0200
Nicolae-Daniel Deaconescu <Nicolae-daniel.Deaconescu@analog.com> wrote:

> Move the EN pin mode setup from ad7625_probe() into
> ad7625_set_initial_mode(). No behaviour change. A following patch adds
> parts without EN pins that skip it.
> 
> Signed-off-by: Nicolae-Daniel Deaconescu <Nicolae-daniel.Deaconescu@analog.com>
> ---
>  drivers/iio/adc/ad7625.c | 57 ++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7625.c b/drivers/iio/adc/ad7625.c
> index e699a7023b02..133c83df8a52 100644
> --- a/drivers/iio/adc/ad7625.c
> +++ b/drivers/iio/adc/ad7625.c
> @@ -561,6 +561,37 @@ static int devm_ad7625_regulator_setup(struct device *dev,
>  	return 0;
>  }
>  
> +static int ad7625_set_initial_mode(struct device *dev, struct ad7625_state *st)
> +{
> +	int ret;
> +
> +	/* Set the device mode based on detected EN configuration. */
> +	if (!st->info->has_bandwidth_control) {
> +		ad7625_set_en_gpios_for_vref(st, st->have_refin, st->vref_mV);
> +		return 0;
> +	}
> +
> +	/*
> +	 * If neither sampling mode is available, then report an error,
> +	 * since the other modes are not useful defaults.
> +	 */
> +	if (st->can_wide_bandwidth) {
> +		ret = ad7960_set_mode(st, AD7960_MODE_WIDE_BANDWIDTH,
> +				      st->have_refin, st->vref_mV);
> +	} else if (st->can_narrow_bandwidth) {
> +		ret = ad7960_set_mode(st, AD7960_MODE_NARROW_BANDWIDTH,
> +				      st->have_refin, st->vref_mV);
> +	} else {
> +		return dev_err_probe(dev, -EINVAL,
> +			"couldn't set device to wide or narrow bandwidth modes\n");
> +	}
> +
> +	if (ret)
> +		return dev_err_probe(dev, -EINVAL, "failed to set EN pins\n");

That's ugly given we are some distance from where ret was set and it
is eating the value.  I'd just duplicate it in the if / else if and
perhaps give a more focused error message in each of those (I haven't
thought about what it would be so maybe it doesn't!)

I know this came from original code, but seems worth tidying up whilst
you are here.

> +
> +	return 0;
> +}
> +
>  static int ad7625_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -595,29 +626,9 @@ static int ad7625_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* Set the device mode based on detected EN configuration. */
> -	if (!st->info->has_bandwidth_control) {
> -		ad7625_set_en_gpios_for_vref(st, st->have_refin, st->vref_mV);
> -	} else {
> -		/*
> -		 * If neither sampling mode is available, then report an error,
> -		 * since the other modes are not useful defaults.
> -		 */
> -		if (st->can_wide_bandwidth) {
> -			ret = ad7960_set_mode(st, AD7960_MODE_WIDE_BANDWIDTH,
> -					      st->have_refin, st->vref_mV);
> -		} else if (st->can_narrow_bandwidth) {
> -			ret = ad7960_set_mode(st, AD7960_MODE_NARROW_BANDWIDTH,
> -					      st->have_refin, st->vref_mV);
> -		} else {
> -			return dev_err_probe(dev, -EINVAL,
> -				"couldn't set device to wide or narrow bandwidth modes\n");
> -		}
> -
> -		if (ret)
> -			return dev_err_probe(dev, -EINVAL,
> -					     "failed to set EN pins\n");
> -	}
> +	ret = ad7625_set_initial_mode(dev, st);
> +	if (ret)
> +		return ret;
>  
>  	ret = devm_ad7625_pwm_get(dev, st);
>  	if (ret)