Message-ID: <20251206192750.03469a87@jic23-huawei>
Date: Sat, 6 Dec 2025 19:27:50 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: "Kurt Borja" <kuurtb@...il.com>
Cc: "David Lechner" <dlechner@...libre.com>, "Andy Shevchenko"
 <andriy.shevchenko@...el.com>, "Rob Herring" <robh@...nel.org>, "Krzysztof
 Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
 "Tobias Sperling" <tobias.sperling@...ting.com>, Nuno Sá
 <nuno.sa@...log.com>, "Andy Shevchenko" <andy@...nel.org>,
 <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "Jonathan Cameron"
 <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

On Tue, 02 Dec 2025 09:46:37 -0500
"Kurt Borja" <kuurtb@...il.com> wrote:

> On Mon Dec 1, 2025 at 4:53 PM -05, David Lechner wrote:
> > On 12/1/25 1:47 PM, Kurt Borja wrote:  
> >> On Mon Dec 1, 2025 at 11:07 AM -05, David Lechner wrote:
> >> 
> >> ...
> >>   
> >>>>>> +	if (iio_device_claim_buffer_mode(indio_dev))
> >>>>>> +		goto out_notify_done;
> >>>>>> +
> >>>>>> +	if (iio_trigger_using_own(indio_dev)) {
> >>>>>> +		disable_irq(ads1018->drdy_irq);
> >>>>>> +		ret = ads1018_read_unlocked(ads1018, &scan.conv, true);
> >>>>>> +		enable_irq(ads1018->drdy_irq);
> >>>>>> +	} else {
> >>>>>> +		ret = spi_read(ads1018->spi, ads1018->rx_buf, sizeof(ads1018->rx_buf));
> >>>>>> +		scan.conv = ads1018->rx_buf[0];
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	iio_device_release_buffer_mode(indio_dev);
> >>>>>> +
> >>>>>> +	if (ret)
> >>>>>> +		goto out_notify_done;
> >>>>>> +
> >>>>>> +	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
> >>>>>> +
> >>>>>> +out_notify_done:
> >>>>>> +	iio_trigger_notify_done(ads1018->indio_trig);  
> >>>>>
> >>>>> Jonathan et al., maybe we need an ACQUIRE() class for this? It will solve
> >>>>> the conditional scoped guard case, no?  
> >>>
> >>> No, ACQUIRE() is not scoped, just conditional. I don't think it
> >>> will improve anything here.  
> >> 
> >> Maybe I'm not understanding the problem fully?
> >> 
> >> I interpreted "ACQUIRE() class" as a general GUARD class, i.e.
> >> 	
> >> 	guard(iio_trigger_notify)(indio_dev->trig);
> >> 
> >> This way drivers may use other cleanup.h helpers cleaner, because of the
> >> goto problem?
> >> 
> >> I do think it's a good idea, like a `defer` keyword. But it is a bit
> >> unorthodox using guard for non locks.

Agreed. This one is weird if called guard().

I'd not be against a defer() if it existed, but my guess is Linus Torvalds
will just say this is too weird and helper function for everything before
the unconditional cleanup is the way to go.

People did mess around with __free() for cases like this but that is very
ugly given no 'constructor' occurred so mostly those got rejected I think.

> >> 
> >>   
> >
> > To take a simple example first:
> >
> > static int
> > ads1018_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > 		 int *val, int *val2, long mask)
> > {
> > 	int ret;
> >
> > 	if (!iio_device_claim_direct(indio_dev))
> > 		return -EBUSY;
> >
> > 	ret = ads1018_read_raw_unlocked(indio_dev, chan, val, val2, mask);
> >
> > 	iio_device_release_direct(indio_dev);
> >
> > 	return ret;
> > }
> >
> > using ACQUIRE would look like:
> >
> > static int
> > ads1018_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > 		 int *val, int *val2, long mask)
> > {
> > 	int ret;
> >
> > 	ACQUIRE(iio_device_claim_direct_mode, claim)(indio_dev);
> > 	if ((ret = ACQUIRE_ERR(iio_device_claim_direct_mode, &claim)))
> > 		return ret;
> >
> > 	return ads1018_read_raw_unlocked(indio_dev, chan, val, val2, mask);
> > }
> >
> > It makes it quite more verbose IMHO with little benefit (the direct
> > return is nice, but comes at at an expense of the rest being less
> > readable).  
> 
> This is verbose yes, but we could avoid having two functions in the
> first place and implement everything inside ads1018_read_raw() with
> ACQUIRE(...) on top.

Agreed - there are places where this makes sense but I'm not keen
on lots of churn to inject it in places where we already have
the two function approach.

> 
> >
> >
> >
> > And when we need it to be scoped, it adds indent and we have to do
> > some unusual things still to avoid using goto.
> >
> > static irqreturn_t ads1018_trigger_handler(int irq, void *p)
> > {
> > 	struct iio_poll_func *pf = p;
> > 	struct iio_dev *indio_dev = pf->indio_dev;
> > 	struct ads1018 *ads1018 = iio_priv(indio_dev);
> > 	struct {
> > 		__be16 conv;
> > 		aligned_s64 ts;
> > 	} scan = {};
> > 	int ret;
> >
> > 	do {
> > 		ACQUIRE(iio_device_claim_direct_mode, claim)(indio_dev);
> > 		if ((ret = ACQUIRE_ERR(iio_device_claim_direct_mode, &claim)))
> > 			break;
> >
> > 		if (iio_trigger_using_own(indio_dev)) {
> > 			disable_irq(ads1018->drdy_irq);
> > 			ret = ads1018_read_unlocked(ads1018, &scan.conv, true);
> > 			enable_irq(ads1018->drdy_irq);
> > 		} else {
> > 			ret = spi_read(ads1018->spi, ads1018->rx_buf, sizeof(ads1018->rx_buf));
> > 			scan.conv = ads1018->rx_buf[0];
> > 		}
> > 	} while (0);  
> 
> Here we could use scoped_cond_guard() instead, no?
Just in case this comes back. Please no!  

scoped_cond_guard() manages to thoroughly confuse compilers.
It got so bad when we tried that originally I went back and reverted
all use of that in IIO.

> 
> >
> > 	if (!ret)
> > 		iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
> >
> > 	iio_trigger_notify_done(ads1018->indio_trig);
> >
> > 	return IRQ_HANDLED;
> > }
> >
> > So unless Jonathan says this is what he wants, I would avoid it.  
> 
> I will submit this as a separate RFC patch. We can continue the
> discussion there to avoid delaying this series.

Thanks and very wise to not yet use it in here as that discussion
may take some time given there is naming involved ;)

Jonathan

> 
> 

Powered by blists - more mailing lists