Message-Id: <DES3ZMFGDP6A.2LQ1WHH17P7JS@gmail.com>
Date: Sun, 07 Dec 2025 11:01:41 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Jonathan Cameron" <jic23@...nel.org>, "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 Sat Dec 6, 2025 at 2:27 PM -05, Jonathan Cameron wrote:
> 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.

Makes sense.

Isn't there a "defer" proposal to the language spec? I think I came
across something like that in the past. I really hope this is the case.

>
>> >> 
>> >>   
>> >
>> > 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.

I agree.

>
>> 
>> >
>> >
>> >
>> > 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.

That's good to know!

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

Glad I can help :)

>
> Jonathan
>

-- 
 ~ Kurt

Powered by blists - more mailing lists