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

[Nicolae-Daniel Deaconescu <Nicolae-daniel.Deaconescu@analog.com>]

Hi Jonathan,

On Sat, 7 Nov 2026 01:59:01 +0000, Jonathan Cameron wrote:
>
> > +	 * 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.

Agreed it is ugly, but I'll not duplicate the check per branch, because the
error can't happen

A mode is only chosen when its flag is set here:

	if (st->can_wide_bandwidth)
		ret = ad7960_set_mode(st, AD7960_MODE_WIDE_BANDWIDTH, ...);
	else if (st->can_narrow_bandwidth)
		ret = ad7960_set_mode(st, AD7960_MODE_NARROW_BANDWIDTH, ...);

ad7960_set_mode() fails for such a mode only if that same flag is false:

	case AD7960_MODE_WIDE_BANDWIDTH:
		if (!st->can_wide_bandwidth)
			return -EINVAL;
		...
		return 0;

   (AD7960_MODE_NARROW_BANDWIDTH is the same with can_narrow_bandwidth.)

Both flags are set before this point and not changed afterwards.

In v4 I'll pick the mode first and call ad7960_set_mode() once,
returning its error instead of -EINVAL:

	if (st->can_wide_bandwidth)
		mode = AD7960_MODE_WIDE_BANDWIDTH;
	else if (st->can_narrow_bandwidth)
		mode = AD7960_MODE_NARROW_BANDWIDTH;
	else
		return dev_err_probe(dev, -EINVAL,
			"couldn't set device to wide or narrow bandwidth modes\n");

	ret = ad7960_set_mode(st, mode, st->have_refin, st->vref_mv);
	if (ret)
		return dev_err_probe(dev, ret, "failed to set EN pins\n");

Thanks,
Daniel