Re: [PATCH v2 2/2] iio: adc: ti-ads1298: Add driver

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 2 Feb 2024 11:59:01 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> Skeleton driver for the TI ADS1298 medical ADC. This device is
> typically used for ECG and similar measurements. Supports data
> acquisition at configurable scale and sampling frequency.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
Hi Mike,

A few minor things I'd missed before.

I'm still interested in why more standard interrupt handling isn't
good enough here (see reply to v1 thread) but if we can't get to the bottom
of that (or do figure it out and we can't fix it) then this doesn't look
too bad so I'll accept the complex handling.

J

> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> new file mode 100644
> index 000000000000..539598b9f3fa
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -0,0 +1,769 @@

> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>

I don't see any custom ABI, so shouldn't need this.

> +
> +#include <asm/unaligned.h>
> +



> +static int ads1298_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct ads1298_private *priv = context;
> +	struct spi_transfer reg_read_xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->cmd_buffer,
> +		.len = 3,
> +		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
> +		.delay = {
> +			.value = 2,
> +			.unit = SPI_DELAY_UNIT_USECS,
> +		},
> +	};
> +	int ret;
> +
> +	priv->cmd_buffer[0] = ADS1298_CMD_RREG | reg;
> +	priv->cmd_buffer[1] = 0x0;

Why mix of hex and decimal? Doesn't matter but looks odd


> +	priv->cmd_buffer[2] = 0;
> +
> +	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	*val = priv->cmd_buffer[2];
> +
> +	return 0;
> +}

> +
> +/* Called from SPI completion interrupt handler */
> +static void ads1298_rdata_complete(void *context)
> +{
> +	struct iio_dev *indio_dev = context;
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int scan_index;
> +	u32 *bounce = priv->bounce_buffer;
> +
> +	if (!iio_buffer_enabled(indio_dev)) {

Good to add a comment here on why this can't race as we are holding
the device in direct mode until after the completion.

iio_buffer_enabled() checks tend to expose races so I prefer people
to explicitly say why there isn't one.



> +		/* Happens when running in single transfer mode */
> +		ads1298_rdata_unmark_busy(priv);
> +		complete(&priv->completion);
> +		return;
> +	}
> +
> +	/* Demux the channel data into our bounce buffer */
> +	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		const struct iio_chan_spec *scan_chan =
> +					&indio_dev->channels[scan_index];
> +		const u8 *data = priv->rx_buffer + scan_chan->address;
> +
> +		*bounce++ = get_unaligned_be24(data);
> +	}
> +
> +	/* rx_buffer can be overwritten from this point on */
> +	ads1298_rdata_release_busy_or_restart(priv);
> +
> +	iio_push_to_buffers(indio_dev, priv->bounce_buffer);
> +}

> +
> +static const char *ads1298_family_name(unsigned int id)
> +{
> +	switch (id & ADS1298_MASK_ID_FAMILY) {
> +	case ADS1298_ID_FAMILY_ADS129X:
> +		return "ADS129x";
> +	case ADS1298_ID_FAMILY_ADS129XR:
> +		return "ADS129xR";
> +	default:
> +		return "(unknown)";
> +	}
> +}
> +
> +static int ads1298_init(struct ads1298_private *priv)
> +{
> +	struct device *dev = &priv->spi->dev;
> +	int ret;
> +	unsigned int val;
> +
> +	/* Device initializes into RDATAC mode, which we don't want. */
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_SDATAC);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(priv->regmap, ADS1298_REG_ID, &val);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "Found %s, %u channels\n", ads1298_family_name(val),
> +		 4 + 2 * (val & ADS1298_MASK_ID_CHANNELS));
Noise for the log that is easy to figure out anyway (assuming successful
probe) Make that dev_dbg()

> +
> +	/* Enable internal test signal, double amplitude, double frequency */
> +	ret = regmap_write(priv->regmap, ADS1298_REG_CONFIG2,
> +		ADS1298_MASK_CONFIG2_RESERVED |
> +		ADS1298_MASK_CONFIG2_INT_TEST |
> +		ADS1298_MASK_CONFIG2_TEST_AMP |
> +		ADS1298_MASK_CONFIG2_TEST_FREQ_FAST);

Unless lines are very long, parameters should align just after (


> +	if (ret)
> +		return ret;
> +
> +	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
> +	if (!priv->reg_vref) {
> +		/* Enable internal reference */
> +		val |= ADS1298_MASK_CONFIG3_PWR_REFBUF;
> +		/* Use 4V VREF when power supply is at least 4.4V */
> +		if (regulator_get_voltage(priv->reg_avdd) >= 4400000)
> +			val |= ADS1298_MASK_CONFIG3_VREF_4V;
> +	}
> +	return regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
> +}
> +
> +static int ads1298_probe(struct spi_device *spi)
> +{

...

> +	ret = devm_request_irq(dev, spi->irq, &ads1298_interrupt,
> +			       IRQF_TRIGGER_FALLING, indio_dev->name,
I missed this before (and we've gotten it wrong a bunch of times in the past
so plenty of bad examples to copy that we can't fix without possible
regressions) but we generally now leave irq direction to the firmware description.
People have an annoying habit of putting not gates and similar in the path
to interrupt pins.  Fine to have the binding state the expected form though
(as you do).  So basically not flags here.

I'm still curious to understand more of where the delays that lead to
needing to do this complex handling came from, but I guess it's not too bad
if we can't get to the bottom of that so I'll take the driver anyway
(after a little more time on list for others to review!)

> +			       indio_dev);