Message-ID: <a01f95ba-23c0-4c4b-a6bc-31b316bb04ef@baylibre.com>
Date: Mon, 1 Dec 2025 17:09:52 -0600
From: David Lechner <dlechner@...libre.com>
To: Kurt Borja <kuurtb@...il.com>, Jonathan Cameron <jic23@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Tobias Sperling <tobias.sperling@...ting.com>
Cc: 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 v3 2/2] iio: adc: Add ti-ads1018 driver

On 11/28/25 9:47 PM, Kurt Borja 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>
> ---

...

> +#define ADS1018_CFG_DEFAULT		0x058b

Would be nice to use the macros below to define this value so that we
know what it is actually doing.

> +
> +#define ADS1018_CFG_OS_TRIG		BIT(15)
> +#define ADS1018_CFG_TS_MODE_EN		BIT(4)
> +#define ADS1018_CFG_PULL_UP		BIT(3)
> +#define ADS1018_CFG_NOP			BIT(1)
> +#define ADS1018_CFG_VALID		(ADS1018_CFG_PULL_UP | ADS1018_CFG_NOP)
> +
> +#define ADS1018_CFG_MUX_MASK		GENMASK(14, 12)
> +
> +#define ADS1018_CFG_PGA_MASK		GENMASK(11, 9)
> +#define ADS1018_PGA_DEFAULT		2
> +
> +#define ADS1018_CFG_MODE_MASK		GENMASK(8, 8)

This is just BIT(8)

> +#define ADS1018_MODE_CONTINUOUS		0
> +#define ADS1018_MODE_ONESHOT		1
> +
> +#define ADS1018_CFG_DRATE_MASK		GENMASK(7, 5)
> +#define ADS1018_DRATE_DEFAULT		4
> +
> +#define ADS1018_CHANNELS_MAX		10
> +

...

> +#define ADS1018_VOLT_CHAN(_addr, _chan, _realbits) {				\
> +	.type = IIO_VOLTAGE,							\
> +	.channel = _chan,							\
> +	.scan_index = _addr,							\
> +	.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),			\
> +	.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,							\
> +}
> +
> +#define ADS1018_TEMP_CHAN(_addr, _realbits) {					\
> +	.type = IIO_TEMP,							\
> +	.channel = 0,								\

channel doesn't matter if it isn't indexed. So we can omit that.

> +	.scan_index = _addr,							\

I would just say _index instead of _addr. For temperature, there is just a
bit flag separate from the mux values.

> +	.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),			\
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +}
> +

...

> +/**
> + * ads1018_calc_delay - Calculates an appropriate delay for a single-shot
> + *			reading

Could leave out a few words to make this fit on one line since we have
the long explanation below.

> + * @ad1018: 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.
> + *
> + * Return: Delay in microseconds.
> + */
> +static unsigned long ads1018_calc_delay(struct ads1018 *ads1018)

Using unsigned long is odd since this doesn't depend on pointer size.
Better to use e.g. u32 so we don't have to think about different sizes
on different architectures.

> +{
> +	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];
> +
> +	/* We subtract 10% data-rate error */
> +	hz -= DIV_ROUND_UP(hz, 10);
> +
> +	/* Calculate time per sample in microseconds */
> +	return DIV_ROUND_UP(MICROHZ_PER_HZ, hz);
> +}
> +
> +/**
> + * ads1018_read_unlocked - Reads a conversion value from the device
> + * @ad1018: Device data
> + * @cnv: ADC Conversion value

Would be nice to mention that this one is 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 spi_bus_lock() is held.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +static int ads1018_read_unlocked(struct ads1018 *ads1018, __be16 *cnv, bool hold_cs)

It is a bit odd to me to call this "unlocked" and then call
"spi_synced_locked()". It sounds like we are using opposite words
to mean the same thing.

> +{
> +	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;
> +}
> +
> +/**
> + * ads1018_oneshot - Performs a one-shot reading sequence
> + * @ad1018: 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
> + * conversion.
> + *
> + * Context: Expects iio_device_claim_direct() is held.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +static int ads1018_oneshot(struct ads1018 *ads1018, u16 cfg, u16 *cnv)
> +{
> +	struct spi_transfer xfer[2] = {
> +		{
> +			.tx_buf = ads1018->tx_buf,
> +			.len = sizeof(ads1018->tx_buf),
> +			.delay = {
> +				.value = ads1018_calc_delay(ads1018),
> +				.unit = SPI_DELAY_UNIT_USECS,
> +			},
> +		},
> +		{
> +			.rx_buf = ads1018->rx_buf,
> +			.len = sizeof(ads1018->rx_buf),
> +		},
> +	};
> +	int ret;
> +
> +	ads1018->tx_buf[0] = cpu_to_be16(cfg);
> +	ads1018->tx_buf[1] = 0;

Do we actually need to transmit two words to trigger a conversion?

It looks like there is a "16-Bit Data Transmission Cycle" for when
we don't need to read the config register back.

> +
> +	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, struct iio_chan_spec const *chan,

Saying "ulocked" here is a bit confusing since the previous "unlocked" had
to do with SPI bus lock rather than iio_device_claim_direct().

> +			  int *val, int *val2, long mask)
> +{

...

> +static int ads1018_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
> +	const struct ads1018_chip_info *chip_info = ads1018->chip_info;
> +	unsigned int pga, drate, addr;
> +	u16 cfg;
> +
> +	addr = find_first_bit(indio_dev->active_scan_mask, iio_get_masklength(indio_dev));
> +	pga = ads1018_get_pga_mode(ads1018, addr);
> +	drate = ads1018_get_data_rate_mode(ads1018, addr);
> +
> +	cfg = ADS1018_CFG_VALID;
> +	cfg |= FIELD_PREP(ADS1018_CFG_MUX_MASK, addr);
> +	cfg |= FIELD_PREP(ADS1018_CFG_PGA_MASK, pga);
> +	cfg |= FIELD_PREP(ADS1018_CFG_MODE_MASK, ADS1018_MODE_CONTINUOUS);
> +	cfg |= FIELD_PREP(ADS1018_CFG_DRATE_MASK, drate);
> +
> +	if (chip_info->channels[addr].type == IIO_TEMP)
> +		cfg |= ADS1018_CFG_TS_MODE_EN;
> +
> +	ads1018->tx_buf[0] = cpu_to_be16(cfg);
> +	ads1018->tx_buf[1] = 0;

Seems like we could use 16-bit cycles here too?

> +
> +	return spi_write(ads1018->spi, ads1018->tx_buf, sizeof(ads1018->tx_buf));

Hmmm... In the case where the trigger is not the DRDY signal (i.e. hritmer or
sysfs), it seems like we would want to defer this until we actually receive
a trigger. Otherwise, if the trigger is not already enabled and it is a while
before the trigger is enabled, then the first data value will be quite stale
compared to the others since the conversion was done when the buffer was enabled
rather than when the trigger fired.

> +}
> +
> +static int ads1018_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
> +
> +	ads1018->tx_buf[0] = cpu_to_be16(ADS1018_CFG_DEFAULT);

Changing DEFAULT to a more descritive name (e.g. SINGLE_SHOT_MODE) would make
this more clear that the purpose of doing this is to take it out of conversion
mode. Otherwise, a comment would be helpful here.

> +	ads1018->tx_buf[1] = 0;
> +
> +	return spi_write(ads1018->spi, ads1018->tx_buf, sizeof(ads1018->tx_buf));
> +}
> +

...

> +static int ads1018_spi_probe(struct spi_device *spi)
> +{
> +	const struct ads1018_chip_info *info = spi_get_device_match_data(spi);
> +	struct iio_dev *indio_dev;
> +	struct ads1018 *ads1018;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*ads1018));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	ads1018 = iio_priv(indio_dev);
> +	ads1018->spi = spi;
> +	ads1018->chip_info = info;
> +	spi_set_drvdata(spi, ads1018);

Looks like this isn't needed any more.

> +
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->name = info->name;
> +	indio_dev->info = &ads1018_iio_info;
> +	indio_dev->channels = info->channels;
> +	indio_dev->num_channels = info->num_channels;
> +
> +	for (unsigned int i = 0; i < ADS1018_CHANNELS_MAX; i++) {
> +		ads1018->chan_data[i].data_rate_mode = ADS1018_DRATE_DEFAULT;
> +		ads1018->chan_data[i].pga_mode = ADS1018_PGA_DEFAULT;
> +	}
> +
> +	ads1018->xfer.rx_buf = ads1018->rx_buf;
> +	ads1018->xfer.len = sizeof(ads1018->rx_buf);
> +	spi_message_init_with_transfers(&ads1018->msg_read, &ads1018->xfer, 1);
> +
> +	ret = ads1018_trigger_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, iio_pollfunc_store_time,
> +					      ads1018_trigger_handler, &ads1018_buffer_ops);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +/**
> + * ADS1018_FSR_TO_SCALE - Converts FSR into scale
> + * @_fsr: Full-scale range in millivolts
> + * @_res: ADC resolution

This doesn't match the implementaion, it requires resolution - 1.
I would do the - 1 in the macro so that we can use the advertised
12 or 16-bit resolution in the tables.

> + *
> + * Return: Scale in IIO_VAL_INT_PLUS_NANO format
> + */
> +#define ADS1018_FSR_TO_SCALE(_fsr, _res) \
> +	{ 0, ((_fsr) * (MICRO >> 6)) / BIT((_res) - 6) }
> +
> +static const unsigned int ads1018_gain_table[][2] = {
> +	ADS1018_FSR_TO_SCALE(6144, 11),
> +	ADS1018_FSR_TO_SCALE(4096, 11),
> +	ADS1018_FSR_TO_SCALE(2048, 11),
> +	ADS1018_FSR_TO_SCALE(1024, 11),
> +	ADS1018_FSR_TO_SCALE(512, 11),
> +	ADS1018_FSR_TO_SCALE(256, 11),
> +};
> +
> +static const unsigned int ads1118_gain_table[][2] = {
> +	ADS1018_FSR_TO_SCALE(6144, 15),
> +	ADS1018_FSR_TO_SCALE(4096, 15),
> +	ADS1018_FSR_TO_SCALE(2048, 15),
> +	ADS1018_FSR_TO_SCALE(1024, 15),
> +	ADS1018_FSR_TO_SCALE(512, 15),
> +	ADS1018_FSR_TO_SCALE(256, 15),
> +};
> +

Powered by blists - more mailing lists