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

[David Lechner <dlechner@baylibre.com>]

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



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);

	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.