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

[Kurt Borja]

On Mon Dec 15, 2025 at 10:55 AM -05, David Lechner wrote:
> On 12/11/25 10:25 PM, Kurt Borja wrote:
>> Add ti-ads1018 driver for Texas Instruments ADS1018 and ADS1118 SPI
>> analog-to-digital converters.
>> 
>
> ...
>
>> +/**
>> + * ads1018_spi_read_exclusive - Reads a conversion value from the device
>> + * @ads1018: Device data
>> + * @cnv: ADC Conversion value (optional)
>> + * @hold_cs: Keep CS line asserted after the SPI transfer
>> + *
>> + * Reads the most recent ADC conversion value, without updating the
>> + * device's configuration.
>> + *
>> + * Context: Expects iio_device_claim_buffer_mode() is held and SPI bus
>> + *	    *exclusive* use.
>
> I guess "exclusive" is supposed to mean that the SPI bus lock is held?
> Would have been more clear to just say "SPI bus lock is held".

I went with "exclusive" because we are not holding the lock on the same
thread, so that could be misleading.

...

>> +static int ads1018_buffer_postdisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	u16 cfg;
>> +
>> +	cfg = ADS1018_CFG_VALID;
>> +	cfg |= FIELD_PREP(ADS1018_CFG_MODE_MASK, ADS1018_MODE_ONESHOT);
>> +
>> +	ads1018->tx_buf[0] = cpu_to_be16(cfg);
>> +	ads1018->tx_buf[1] = 0;
>> +
>> +	return spi_write(ads1018->spi, ads1018->tx_buf, sizeof(ads1018->tx_buf));
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ads1018_buffer_ops = {
>> +	.preenable = ads1018_buffer_preenable,
>> +	.postdisable = ads1018_buffer_postdisable,
>> +	.validate_scan_mask = iio_validate_scan_mask_onehot,
>
> Why do we only allow one channel to be selected at a time?
> I guess we can remove that restriction later. I just didn't
> notice this in previous reviews.

I chose this restriction because of the timestamp discussion a few
versions ago.

I will remove it later and implement multichannel buffers as Jonathan
suggested.

...

>> +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;
>> +
>
>
>> +	if (iio_device_claim_buffer_mode(indio_dev))
>> +		goto out_notify_done;
>
> This should not be needed. It should not be possible to
> exit buffer mode without triggers being stopped first.
> (No other driver is doing this.)

Previously I had my own lock here because ads1018_spi_read_exclusive()
needs locking. I dropped it per your suggestion in v1 I believe, but
given the discussion in the cleanup.h series I was thinking in bringing
it back.

But yes, the scope can be adjusted too.

-- 
Thanks,
 ~ Kurt