Message-ID: <aSlxSj7XEq7PUxsk@smile.fi.intel.com>
Date: Fri, 28 Nov 2025 11:54:18 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: 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

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?

> +	/* 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, ...);

...

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

...

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

> +};

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists