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

[David Lechner <dlechner@baylibre.com>]

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

> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +static int ads1018_spi_read_exclusive(struct ads1018 *ads1018, __be16 *cnv,
> +				      bool hold_cs)
> +{
> +	int ret;
> +
> +	ads1018->xfer.cs_change = hold_cs;
> +
> +	ret = spi_sync_locked(ads1018->spi, &ads1018->msg_read);
> +	if (ret)
> +		return ret;
> +
> +	if (cnv)
> +		*cnv = ads1018->rx_buf[0];
> +
> +	return 0;
> +}
> +

...

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

> +};
> +
> +static irqreturn_t ads1018_irq_handler(int irq, void *dev_id)
> +{
> +	struct ads1018 *ads1018 = dev_id;
> +
> +	/*
> +	 * We need to check if the "drdy" pin is actually active or if it's a
> +	 * pending interrupt triggered by the SPI transfer.
> +	 */
> +	if (!gpiod_get_value(ads1018->drdy_gpiod))
> +		return IRQ_HANDLED;
> +
> +	iio_trigger_poll(ads1018->indio_trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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.)

> +
> +	if (iio_trigger_using_own(indio_dev)) {
> +		disable_irq(ads1018->drdy_irq);
> +		ret = ads1018_spi_read_exclusive(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];
> +	}
> +	if (ret)
> +		goto out_release_buffer;
> +
> +	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
> +
> +out_release_buffer:
> +	iio_device_release_buffer_mode(indio_dev);
> +out_notify_done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +