Re: [PATCH v2 11/27] iio: adc: ad4695: Stop using iio_device_claim_direct_scoped()

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2025-02-16 at 13:00 -0600, David Lechner wrote:
> On 2/16/25 12:19 PM, Jonathan Cameron wrote:
> > On Sun,  9 Feb 2025 18:06:08 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> > 
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > This complex cleanup.h use case of conditional guards has proved
> > > to be more trouble that it is worth in terms of false positive compiler
> > > warnings and hard to read code.
> > > 
> > > Move directly to the new claim/release_direct() that allow sparse
> > > to check for unbalanced context.  In some cases code is factored
> > > out to utility functions that can do a direct return with the
> > > claim and release around the call.
> > > 
> > > Reviewed-by: David Lechner <dlechner@baylibre.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > v2: Typo in commit description (David).
> > > Note there are several sets current in flight that touch this driver.
> > > I'll rebase as necessary depending on what order the dependencies resolve.
> > I've done this rebase and applied on the testing branch of iio.git.
> > 
> > Would appreciate a sanity check if anyone has time though!
> > 
> > New code is as follows.  The one corner I was not sure on was
> > that for calibbias reading the direct mode claim was held for a long
> > time.  That seems to be unnecessary as we have a copy of osr anyway
> > in that function used for other purposes.
> > 
> 
> ...
> 
> >  	case IIO_CHAN_INFO_CALIBBIAS:
> > -		switch (chan->type) {
> > -		case IIO_VOLTAGE:
> > -			iio_device_claim_direct_scoped(return -EBUSY,
> > indio_dev) {
> > -				ret = regmap_read(st->regmap16,
> > -					AD4695_REG_OFFSET_IN(chan-
> > >scan_index),
> > -					&reg_val);
> > -				if (ret)
> > -					return ret;
> > -
> > -				tmp = sign_extend32(reg_val, 15);
> > -
> > -				switch (cfg->oversampling_ratio) {
> > -				case 1:
> > -					*val = tmp / 4;
> > -					*val2 = abs(tmp) % 4 * MICRO / 4;
> > -					break;
> > -				case 4:
> > -					*val = tmp / 2;
> > -					*val2 = abs(tmp) % 2 * MICRO / 2;
> > -					break;
> > -				case 16:
> > -					*val = tmp;
> > -					*val2 = 0;
> > -					break;
> > -				case 64:
> > -					*val = tmp * 2;
> > -					*val2 = 0;
> > -					break;
> > -				default:
> > -					return -EINVAL;
> > -				}
> > -
> > -				if (tmp < 0 && *val2) {
> > -					*val *= -1;
> > -					*val2 *= -1;
> > -				}
> > -
> > -				return IIO_VAL_INT_PLUS_MICRO;
> > +		switch (chan->type)
> > +		case IIO_VOLTAGE: {
> > +			if (!iio_device_claim_direct(indio_dev))
> > +				return -EBUSY;
> > +			ret = regmap_read(st->regmap16,
> > +					  AD4695_REG_OFFSET_IN(chan-
> > >scan_index),
> > +					  &reg_val);
> > +			iio_device_release_direct(indio_dev);
> > +			if (ret)
> > +				return ret;
> > ////THIS IS THE BIT I WOuLD LIKE EYES on.
> 
> Looks fine to me.

+1

- Nuno Sá
> 
> > +
> > +			tmp = sign_extend32(reg_val, 15);
> > +
> > +			switch (osr) {
> > +			case 1:
> > +				*val = tmp / 4;
> > +				*val2 = abs(tmp) % 4 * MICRO / 4;
> > +				break;
> > +			case 4:
> > +				*val = tmp / 2;
> > +				*val2 = abs(tmp) % 2 * MICRO / 2;
> > +				break;
> > +			case 16:
> > +				*val = tmp;
> > +				*val2 = 0;
> > +				break;
> > +			case 64:
> > +				*val = tmp * 2;
> > +				*val2 = 0;
> > +				break;
> > +			default:
> > +				return -EINVAL;
> > +			}
> > +
> > +			if (tmp < 0 && *val2) {
> > +				*val *= -1;
> > +				*val2 *= -1;
> >  			}
> > -			unreachable();
> > +
> > +			return IIO_VAL_INT_PLUS_MICRO;
> >  		default:
> >  			return -EINVAL;
> >  		}