Message-ID: <20251206200721.5e683a83@jic23-huawei>
Date: Sat, 6 Dec 2025 20:07:21 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Kurt Borja <kuurtb@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Tobias Sperling
 <tobias.sperling@...ting.com>, David Lechner <dlechner@...libre.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 v6 2/2] iio: adc: Add ti-ads1018 driver

On Thu, 04 Dec 2025 13:01:28 -0500
Kurt Borja <kuurtb@...il.com> wrote:

> Add ti-ads1018 driver for Texas Instruments ADS1018 and ADS1118 SPI
> analog-to-digital converters.
> 
> These chips' MOSI pin is shared with a data-ready interrupt. Defining
> this interrupt in devicetree is optional, therefore we only create an
> IIO trigger if one is found.
> 
> Handling this interrupt requires some considerations. When enabling the
> trigger the CS line is tied low (active), thus we need to hold
> spi_bus_lock() too, to avoid state corruption. This is done inside the
> set_trigger_state() callback, to let users use other triggers without
> wasting a bus lock.
> 
> Signed-off-by: Kurt Borja <kuurtb@...il.com>
Hi Kurt

Generally in a good state.  A few minor things inline.
I didn't understand the delay calculation comments.

Jonathan


> diff --git a/drivers/iio/adc/ti-ads1018.c b/drivers/iio/adc/ti-ads1018.c
> new file mode 100644
> index 000000000000..419e789bd0eb
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1018.c
> @@ -0,0 +1,826 @@

> +struct ads1018_chip_info {
> +	const char *name;
> +
> +	const struct iio_chan_spec *channels;
> +	unsigned long num_channels;
> +
> +	/* IIO_VAL_INT */
> +	const unsigned int *data_rate_mode_to_hz;
> +	unsigned long num_data_rate_mode_to_hz;
(unlike pga mode this one is fine as there are variable numbers
 of these)
> +
> +	/* IIO_VAL_INT_PLUS_NANO */
> +	const unsigned int (*pga_mode_to_gain)[2];
> +	unsigned long num_pga_mode_to_gain;

Maybe this is a case of premature addition of flexibility given
this is always 6 so far and you could just embed the data in here
rather than access via a pointer.

> +
> +	/* IIO_VAL_INT_PLUS_MICRO */
> +	const int temp_scale[2];
> +};


> +#define ADS1018_VOLT_CHAN(_index, _chan, _realbits) {				\
> +	.type = IIO_VOLTAGE,							\
> +	.channel = _chan,							\
> +	.scan_index = _index,							\
> +	.scan_type = {								\
> +		.sign = 's',							\
> +		.realbits = _realbits,						\
> +		.storagebits = 16,						\
> +		.shift = 16 - _realbits,					\
> +		.endianness = IIO_BE,						\
> +	},									\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> +			      BIT(IIO_CHAN_INFO_SCALE) |			\
> +			      BIT(IIO_CHAN_INFO_SAMP_FREQ),			\

What motivates per channel sampling frequency?

Given you have to write it each time you configure I guess it doesn't matter much
either way.

> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +	.indexed = true,							\
> +}

> +/**
> + * ads1018_get_data_rate_mode - Get current data-rate mode for a channel.
> + * @ads1018: Device data
> + * @address: Channel address
> + *
> + * Context: Expects iio_device_claim_direct() is held.
> + *
> + * Return: Current data-rate mode for the channel at @address.
> + */
> +static u8 ads1018_get_data_rate_mode(struct ads1018 *ads1018, unsigned int address)
> +{
> +	return ads1018->chan_data[address].data_rate_mode;
> +}
> +
> +/**
> + * ads1018_get_pga_mode - Get current PGA mode for a channel.
> + * @ads1018: Device data
> + * @address: Channel address
> + *
> + * Context: Expects iio_device_claim_direct() is held.
> + *
> + * Return: Current PGA mode for the channel at @address.
> + */
> +static u8 ads1018_get_pga_mode(struct ads1018 *ads1018, unsigned int address)
> +{
> +	return ads1018->chan_data[address].pga_mode;
> +}
> +
> +/**
> + * ads1018_set_data_rate_mode - Set a data-rate mode for a channel.
> + * @ads1018: Device data
> + * @address: Channel address
> + * @val: New data-rate mode for channel at @address.
> + *
> + * Context: Expects iio_device_claim_direct() is held.
> + *
> + * Lazily set a new data-rate mode for a channel.

I'm not sure these tiny access helpers add anything much. Maybe
just get/set the value directly inline?  The field name in the
structure makes it fairly clear what is going on.

> + */
> +static void ads1018_set_data_rate_mode(struct ads1018 *ads1018,
> +				       unsigned int address, u8 val)
> +{
> +	ads1018->chan_data[address].data_rate_mode = val;
> +}
> +
> +/**
> + * ads1018_set_pga_mode - Set a PGA mode for a channel.
> + * @ads1018: Device data
> + * @address: Channel address
> + * @val: New PGA mode for channel at @address.
> + *
> + * Context: Expects iio_device_claim_direct() is held.
> + *
> + * Lazily set a new PGA mode for a channel.
> + */
> +static void ads1018_set_pga_mode(struct ads1018 *ads1018,
> +				 unsigned int address, u8 val)
> +{
> +	ads1018->chan_data[address].pga_mode = val;
> +}
> +
> +/**
> + * ads1018_calc_delay - Calculates a suitable delay for a single-shot reading
> + * @ads1018: Device data
> + *
> + * Calculates an appropriate delay for a single shot reading, assuming the
> + * device's maximum data-rate is used.
> + *
> + * Context: Expects iio_device_claim_direct() is held.

What in here changes if we are in buffered mode?
We have no reason to call it but why does that matter?

> + *
> + * Return: Delay in microseconds (Always greater than 0).
> + */
> +static u32 ads1018_calc_delay(struct ads1018 *ads1018)
> +{
> +	const struct ads1018_chip_info *chip_info = ads1018->chip_info;
> +	unsigned long max_drate_mode = chip_info->num_data_rate_mode_to_hz - 1;
> +	unsigned int hz = chip_info->data_rate_mode_to_hz[max_drate_mode];
> +
> +	/*
> +	 * Calculate the worst-case sampling rate on the maximum data-rate
> +	 * mode by subtracting 10% error specified in the datasheet.
> +	 */
> +	hz -= DIV_ROUND_UP(hz, 10);
> +
> +	/*
> +	 * Then calculate time per sample in microseconds.
> +	 */

Single line comment syntax appropriate here.

> +	return DIV_ROUND_UP(MICROHZ_PER_HZ, hz);
> +}

> +/**
> + * ads1018_single_shot - Performs a one-shot reading sequence
> + * @ads1018: Device data
> + * @cfg: New configuration for the device
> + * @cnv: Conversion value
> + *
> + * Writes a new configuration, waits an appropriate delay (assuming the new
> + * configuration uses the maximum data-rate) and then reads the most recent

I'm lost on this.  Normally the longest delay is governed by the minimum data rate.
I.e. Samples take longer when running few per second, so we wait longer.

I think this is meant to mean the delay needed for a sample at the minimum expected
rate for this configuration.

> + * conversion.
> + *
> + * Context: Expects iio_device_claim_direct() is held.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +static int ads1018_single_shot(struct ads1018 *ads1018, u16 cfg, u16 *cnv)
> +{
> +	struct spi_transfer xfer[2] = {
> +		{
> +			.tx_buf = ads1018->tx_buf,
> +			.len = sizeof(ads1018->tx_buf[0]),
> +			.delay = {
> +				.value = ads1018_calc_delay(ads1018),
> +				.unit = SPI_DELAY_UNIT_USECS,
> +			},
> +			.cs_change = 1, /* 16-bit mode requires CS de-assert */
> +		},
> +		{
> +			.rx_buf = ads1018->rx_buf,
> +			.len = sizeof(ads1018->rx_buf[0]),
> +		},
> +	};
> +	int ret;
> +
> +	ads1018->tx_buf[0] = cpu_to_be16(cfg);
> +
> +	ret = spi_sync_transfer(ads1018->spi, xfer, ARRAY_SIZE(xfer));
> +	if (ret)
> +		return ret;
> +
> +	*cnv = be16_to_cpu(ads1018->rx_buf[0]);
> +
> +	return 0;
> +}
> +
> +static int
> +ads1018_read_raw_unlocked(struct iio_dev *indio_dev,

Similar comment to below.  I think unlocked is emphasising the wrong property.

> +			  struct iio_chan_spec const *chan, int *val, int *val2,
> +			  long mask)
> +{

> +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;
> +}

> +
> +static int
> +ads1018_write_raw_unlocked(struct iio_dev *indio_dev,

Similar to the naming discussion on the ACQUIRE RFC I'm not sure
using locked here is really descriptive of more than an internal
detail of how we prevent mode switching. I'd prefer something like
ads1018_write_raw_direct_claimed() or ads1018_write_raw_direct_mode()
(the absence of any other write_raw_*** would indicate this is the only
valid one perhaps).

Also this isn't the unlocked version, it's the one that doesn't take
the lock.


> +			   struct iio_chan_spec const *chan, int val, int val2,
> +			   long mask)
> +{
...

> +
> +static int
> +ads1018_write_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_write_raw_unlocked(indio_dev, chan, val, val2, mask);
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
> +}

Powered by blists - more mailing lists