Message-Id: <DEKIF0XIN34S.97S95Z0Q5SL1@gmail.com>
Date: Fri, 28 Nov 2025 12:38:44 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Andy Shevchenko" <andriy.shevchenko@...el.com>, "Kurt Borja"
 <kuurtb@...il.com>
Cc: "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>,
 "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 v2 2/2] iio: adc: Add ti-ads1018 driver

Hi Andy,

On Fri Nov 28, 2025 at 4:54 AM -05, Andy Shevchenko wrote:
> On Thu, Nov 27, 2025 at 10:37:11PM -0500, 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.
>
> ...
>
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>
>> +#include <linux/bitmap.h>
>> +#include <linux/bitops.h>
>> +#include <linux/bits.h>
>
> bits.h and bitops.h are guaranteed by bitmap.h, so
>
> #include <linux/bitmap.h>
>
> is enough for all three.
>
>> +#include <linux/byteorder/generic.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/math.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/spi/spi.h>
>
> + types.h // E.g., u8 just a page below :-)
>
>> +#include <linux/units.h>
>
> ...
>
>> +static unsigned long ads1018_calc_delay(struct ads1018 *ads1018)
>> +{
>> +	const struct ads1018_chip_info *chip_info = ads1018->chip_info;
>> +	unsigned long mode = chip_info->num_data_rate_mode_to_hz - 1;
>> +	int hz = chip_info->data_rate_mode_to_hz[mode];
>
> Can frequency be negative?

I hope not 0.0

I return data_rate_mode_to_hz[] values directly in __ads1018_read_raw(),
which uses `int *`. That's why I chose int, but I can just make it
unsigned if that's preferred.

>
>> +	/* We subtract -10% data-rate error */
>> +	hz -= DIV_ROUND_UP(hz, 10);
>> +
>> +	/* Calculate time per sample in usecs */
>
> Spell it fully "microseconds" or use proper unit with a Greek latter "µs".
>
>> +	return DIV_ROUND_UP(MICROHZ_PER_HZ, hz);
>> +}
>
> ...
>
>> +static int
>> +__ads1018_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
>> +		   int *val, int *val2, long mask)
>
> Better naming is something like ads1018_read_raw_unlocked() if I got
> the context right.
>
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	const struct ads1018_chip_info *chip_info = ads1018->chip_info;
>> +	u8 drate_mode = ads1018_get_data_rate_mode(ads1018, chan->scan_index);
>> +	u8 pga_mode = ads1018_get_pga_mode(ads1018, chan->scan_index);
>> +	u8 max_drate_mode = chip_info->num_data_rate_mode_to_hz - 1;
>> +	u16 cnv, cfg = 0;
>
> Unneeded cfg assignment. See below.
>
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>
>> +		cfg |= ADS1018_CFG_VALID;
>> +		cfg |= ADS1018_CFG_OS_TRIG;
>
> 		cfg = ADS1018_CFG_VALID | ADS1018_CFG_OS_TRIG;
>
>> +		cfg |= FIELD_PREP(ADS1018_CFG_MUX_MASK, chan->scan_index);
>> +		cfg |= FIELD_PREP(ADS1018_CFG_PGA_MASK, pga_mode);
>> +		cfg |= FIELD_PREP(ADS1018_CFG_MODE_MASK, ADS1018_MODE_ONESHOT);
>> +		cfg |= FIELD_PREP(ADS1018_CFG_DRATE_MASK, max_drate_mode);
>
> + blank line.
>
>> +		if (chan->type == IIO_TEMP)
>> +			cfg |= ADS1018_CFG_TS_MODE_EN;
>> +
>> +		ret = ads1018_oneshot(ads1018, cfg, &cnv);
>> +		if (ret)
>> +			return ret;
>> +
>> +		cnv >>= chan->scan_type.shift;
>> +		*val = sign_extend32(cnv, chan->scan_type.realbits - 1);
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_VOLTAGE:
>> +			*val = chip_info->pga_mode_to_gain[pga_mode][0];
>> +			*val2 = chip_info->pga_mode_to_gain[pga_mode][1];
>> +			return IIO_VAL_INT_PLUS_NANO;
>> +
>> +		case IIO_TEMP:
>> +			*val = chip_info->temp_scale[0];
>> +			*val2 = chip_info->temp_scale[1];
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = chip_info->data_rate_mode_to_hz[drate_mode];
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>
> ...
>
>> +static int
>> +__ads1018_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
>> +		    int val, int val2, long mask)
>
> Naming...
>
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	const struct ads1018_chip_info *info = ads1018->chip_info;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		for (int i = 0; i < info->num_pga_mode_to_gain; i++) {
>
> Why is 'i' signed?
>
>> +			if (val != info->pga_mode_to_gain[i][0] ||
>> +			    val2 != info->pga_mode_to_gain[i][1])
>> +				continue;
>
>> +			ads1018_set_pga_mode(ads1018, chan->scan_index, i);
>> +			return 0;
>
> This is an invariant to the loop, perhaps
>
>> +		}
>
>> +		return -EINVAL;
>
> 		for (unsigned int i = 0; i < info->num_pga_mode_to_gain; i++) {
> 			if (val == info->pga_mode_to_gain[i][0] ||
> 			    val2 == info->pga_mode_to_gain[i][1])
> 				break;
> 		}
> 		if (i == info->num_pga_mode_to_gain)
> 			return -EINVAL;
>
> 		ads1018_set_pga_mode(ads1018, chan->scan_index, i);
> 		return 0;
>
> This also will follow the traditional pattern: Check for the error cases first.
>
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		for (int i = 0; i < info->num_data_rate_mode_to_hz; i++) {
>
> Ditto, same comments as per previous loop.
>
>> +			if (val != info->data_rate_mode_to_hz[i])
>> +				continue;
>> +
>> +			ads1018_set_data_rate_mode(ads1018, chan->scan_index, i);
>> +			return 0;
>> +		}
>> +
>> +		return -EINVAL;
>> +
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>
> ...
>
>> +static int ads1018_set_trigger_state(struct iio_trigger *trig, bool state)
>> +{
>> +	struct ads1018 *ads1018 = iio_trigger_get_drvdata(trig);
>> +	__be16 cnv;
>> +
>> +	/*
>> +	 * We need to lock the SPI bus and tie CS low (hold_cs) to catch
>> +	 * data-ready interrupts, otherwise the MISO line enters a Hi-Z state.
>> +	 */
>> +
>> +	if (state) {
>> +		spi_bus_lock(ads1018->spi->controller);
>> +		ads1018_read_locked(ads1018, &cnv, true);
>> +		enable_irq(ads1018->drdy_irq);
>> +	} else {
>> +		disable_irq(ads1018->drdy_irq);
>> +		ads1018_read_locked(ads1018, &cnv, false);
>> +		spi_bus_unlock(ads1018->spi->controller);
>> +	}
>> +
>> +	return 0;
>
> This function is basically
>
> 	if (foo)
> 		do this
> 	else
> 		do that
>
> please make just two distinct functions instead.
>
>> +}
>
> That's how it will look like:
>
> static void ads1018_set_trigger_enable(struct ads1018 *ads1018)
> {
> 	__be16 cnv;
>
> 	spi_bus_lock(ads1018->spi->controller);
> 	ads1018_read_locked(ads1018, &cnv, true);
> 	enable_irq(ads1018->drdy_irq);
> }
>
> static void ads1018_set_trigger_disable(struct ads1018 *ads1018)
> {
> 	__be16 cnv;
>
> 	disable_irq(ads1018->drdy_irq);
> 	ads1018_read_locked(ads1018, &cnv, false);
> 	spi_bus_unlock(ads1018->spi->controller);
> }
>
> static int ads1018_set_trigger_state(struct iio_trigger *trig, bool state)
> {
> 	struct ads1018 *ads1018 = iio_trigger_get_drvdata(trig);
>
> 	/*
> 	 * We need to lock the SPI bus and tie CS low (hold_cs) to catch
> 	 * data-ready interrupts, otherwise the MISO line enters a Hi-Z state.
> 	 */
> 	if (state)
> 		ads1018_set_trigger_enable(ads1018);
> 	else
> 		ads1018_set_trigger_disable(ads1018);
>
> 	return 0;
> }
>
> Now is the question, can we get rid of dummy variables by checking for
> the pointer in the _read_locked()? If so, the above become as simple as
>
> static void ads1018_set_trigger_enable(struct ads1018 *ads1018)
> {
> 	spi_bus_lock(ads1018->spi->controller);
> 	ads1018_read_locked(ads1018, NULL, true);
> 	enable_irq(ads1018->drdy_irq);
> }
>
> static void ads1018_set_trigger_disable(struct ads1018 *ads1018)
> {
> 	disable_irq(ads1018->drdy_irq);
> 	ads1018_read_locked(ads1018, NULL, false);
> 	spi_bus_unlock(ads1018->spi->controller);
> }
>
> Or if you make _read_locked() to return the data, just ignoring it as in
>
> 	ads1018_read_locked(ads1018, ...);

Sure, I like this.

>
> ...
>
>> +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 = 0;
>
> Unneeded assignment.
>
>> +	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 = 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);
>
> + blank line.
>
>> +	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;
>> +
>> +	return spi_write(ads1018->spi, ads1018->tx_buf, sizeof(ads1018->tx_buf));
>> +}
>
> ...
>
>> +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;
>> +
>> +	if (iio_trigger_using_own(indio_dev)) {
>> +		disable_irq(ads1018->drdy_irq);
>> +		ret = ads1018_read_locked(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)
>> +		iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
>> +					    pf->timestamp);
>
> 	if (ret)
> 		goto out_notify_done;
>
> // see a comment about traditional pattern somewhere above.
>
> 	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
>
> (and yes, leave the above on a single 82 characters long line).
>
>> +out_notify_done:
>> +	iio_trigger_notify_done(ads1018->indio_trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>
> ...
>
>> +static int ads1018_trigger_setup(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	struct spi_device *spi = ads1018->spi;
>
> 	struct device *dev = &spi->dev;
>
>> +	int ret;
>
> 	const char con_id = "drdy";
>
>> +	ads1018->drdy_gpiod = devm_gpiod_get_optional(&spi->dev, "drdy",
>> +						      GPIOD_IN);
>
> 	ads1018->drdy_gpiod = devm_gpiod_get_optional(dev, con_id, GPIOD_IN);
>
> and so on...
>
> // here we already make the diff 0 LoCs
>
>> +	if (IS_ERR(ads1018->drdy_gpiod))
>> +		return dev_err_probe(&spi->dev, PTR_ERR(ads1018->drdy_gpiod),
>> +				     "Failed to get 'drdy' GPIO.\n");
>
> 		return dev_err_probe(dev, PTR_ERR(ads1018->drdy_gpiod),
> 				     "Failed to get '%s' GPIO.\n", con_id);
>
>> +	if (spi->irq > 0) {
>> +		ads1018->drdy_irq = spi->irq;
>> +	} else if (ads1018->drdy_gpiod) {
>> +		ads1018->drdy_irq = gpiod_to_irq(ads1018->drdy_gpiod);
>> +		if (ads1018->drdy_irq < 0)
>> +			return dev_err_probe(&spi->dev, ads1018->drdy_irq,
>> +					     "Failed to get IRQ from 'drdy' GPIO.\n");
>
> 			return dev_err_probe(dev, ads1018->drdy_irq,
> 					     "Failed to get IRQ from '%s' GPIO.\n",
> 					     con_id);
>
>> +	} else {
>> +		return 0;
>> +	}
>
> Can be refactored to have less indentation, though
>
> 	/* Try to retrieve an IRQ from SPI core then from GPIO */
> 	if (spi->irq > 0)
> 		ads1018->drdy_irq = spi->irq;
> 	else if (ads1018->drdy_gpiod)
> 		ads1018->drdy_irq = gpiod_to_irq(ads1018->drdy_gpiod);
> 	if (ads1018->drdy_irq < 0)
> 		return dev_err_probe(dev, ads1018->drdy_irq,
> 				     "Failed to get IRQ from '%s' GPIO.\n", con_id);
> 	/* No IRQ is fine, avoid supporting triggers */
> 	if (ads1018->drdy_irq == 0)
> 		return 0;
>
>> +	ads1018->indio_trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d-drdy",
>> +						     indio_dev->name,
>> +						     iio_device_id(indio_dev));
>
> // and here I believe we make it 1 LoC less.
>
>> +	if (!ads1018->indio_trig)
>> +		return -ENOMEM;
>> +
>> +	iio_trigger_set_drvdata(ads1018->indio_trig, ads1018);
>> +	ads1018->indio_trig->ops = &ads1018_trigger_ops;
>> +
>> +	ret = devm_iio_trigger_register(&spi->dev, ads1018->indio_trig);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * The "data-ready" IRQ line is shared with the MOSI pin, thus we need
>> +	 * to keep it disabled until we actually request data.
>> +	 */
>> +	return devm_request_irq(&spi->dev, ads1018->drdy_irq, ads1018_irq_handler,
>> +			       IRQF_NO_AUTOEN, ads1018->chip_info->name, ads1018);
>> +}
>
> ...
>
>> +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);
>> +
>> +	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 (int i = 0; i < ADS1018_CHANNELS_MAX; i++) {
>
> unsigned int
>
>> +		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);
>> +}
>
> ...
>
>> +#define ADS1018_FSR_TO_SCALE(_fsr, _res) \
>> +	{ 0, ((_fsr) * MICRO) / BIT(_res) }
>> +
>> +static const int ads1018_gain_table[][2] = {
>> +	ADS1018_FSR_TO_SCALE(6144, 11),
>
> This won't (hmm... might not? see below for the details) work on 32-bit.
>
>> +	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 int ads1118_gain_table[][2] = {
>> +	ADS1018_FSR_TO_SCALE(6144, 15),
>
> Ditto.
>
>> +	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),
>> +};
>
> To address that you need to divide MICRO at least by 2
> (note, by 2⁶ is also possible without loosing in precision).
>
> // Assuming that you want to keep the initialisers as is now:
> #define ADS1018_FSR_TO_SCALE(_fsr, _res) \
> 	{ 0, ((_fsr) * (MICRO >> 6)) / BIT((_res) - 6) }
>
> OTOH, these all are constants, so the clever compiler (even 32-bit compiler)
> may do it for you and won't complain as there is no overflow in the result.
>
> TL;DR: Try to compile this with 32-bit compiler on 32-bit machine before
> changing as suggested.

I didn't consider 32-bit here... I'll test it for v3!

>
> ...
>
>> +static const unsigned int ads1018_data_rate_table[] = {
>> +	128, 250, 490, 920, 1600, 2400, 3300
>
> Leave trailing comma.
>
>> +};
>> +
>> +static const unsigned int ads1118_data_rate_table[] = {
>> +	8, 16, 32, 64, 128, 250, 475, 860
>
> Ditto.
>
>> +};

Ack, for everything else. Thank you, Andy!


-- 
 ~ Kurt

Powered by blists - more mailing lists