Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

["Kurt Borja" <kuurtb@gmail.com>]

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

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

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


-- 
 ~ Kurt