[1/2] dt-bindings: iio: adc: ti-ads1298: Add driver

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>

---

 .../bindings/iio/adc/ti,ads1298.yaml          | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml

On Wed, Dec 13, 2023 at 10:47:22AM +0100, Mike Looijmans 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.

...

> +config TI_ADS1298
> +	tristate "Texas Instruments ADS1298"
> +	depends on SPI
> +	select IIO_BUFFER

> +	default y

Huh?!

> +	help
> +	  If you say yes here you get support for Texas Instruments ADS1298
> +	  medical ADC chips
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called ti-ads1298.

...

> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>

> +#include <linux/kernel.h>

Is it used as a proxy? (At least for array_size.h)
Please use real headers in such case.

> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>

> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

This is interesting grouping, but okay, I understand the point.

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

...

> +#define ADS1298_CLK_RATE	2048000

Units? _HZ ?

...

> +/* Outputs status word and 8 samples of 24 bits each, plus the command byte */

/* Outputs status word and 8 24-bit samples, plus the command byte */

a bit shorter.

> +#define ADS1298_SPI_RDATA_BUFFER_SIZE	((ADS1298_MAX_CHANNELS + 1) * 3 + 1)

...

> +#define ADS1298_CHAN(index)					\
> +{								\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = index,					\
> +	.address = 3 * index + 4,				\

Hmm... does this 3 have a distinct definition?

> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),		\

Can be written as below

	.info_mask_separate =					\
		BIT(IIO_CHAN_INFO_RAW) |			\
		BIT(IIO_CHAN_INFO_SCALE),			\

> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \

	.info_mask_shared_by_all =				\
		BIT(IIO_CHAN_INFO_SAMP_FREQ) |			\
		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\

> +	.scan_index = index,					\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 24,					\
> +		.storagebits = 32,				\
> +		.endianness = IIO_BE,				\
> +	},							\
> +}

...

> +static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
> +{
> +	struct spi_transfer cmd_xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->cmd_buffer,
> +		.len = 1,

		sizeof(command) ?

> +		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,

> +		.delay.value = 2,
> +		.delay.unit = SPI_DELAY_UNIT_USECS,

		.delay = {
			.value = ...
			.unit = ...
		},

> +	};

> +	priv->cmd_buffer[0] = command;
> +
> +	return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
> +}

...

> +	/* Cannot take longer than 40ms (250Hz) */
> +	ret = wait_for_completion_timeout(&priv->completion,
> +					  msecs_to_jiffies(50));

One line?

> +	if (!ret)
> +		return -ETIMEDOUT;

...

> +	if (cfg & ADS1298_MASK_CONFIG1_HR)
> +		rate >>= 6; /* HR mode */
> +	else
> +		rate >>= 7; /* LP mode */

Are those magic numbers defined?

...

> +	factor = (rate >> 6) / val;

Is it the same 6 semantically as above?

...

> +	return IIO_VAL_FRACTIONAL_LOG2;
> +}
> +
> +

One blank line is enough.

> +		*val = sign_extend32(get_unaligned_be24(
> +					priv->rx_buffer + chan->address), 23);

Strange indentation.

		*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
				     23);

...

> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
> +		if (!ret) {

Why not using standard pattern?

		if (ret)
			return ret;

(see below)

> +			ret = IIO_VAL_INT;
> +			*val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));

Outer parentheses are redundant.

> +		}
> +		break;

		return IIO_VAL_INT;


> +	default:
> +		ret = -EINVAL;
> +		break;

return directly.

> +	}

> +	return ret;

It will gone.

...

> +static int ads1298_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct ads1298_private *priv = iio_priv(indio_dev);

> +	int ret;

No need, just return directly.

> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = ads1298_set_samp_freq(priv, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}

...

> +static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			      unsigned int writeval, unsigned int *readval)
> +{
> +	struct ads1298_private *priv = iio_priv(indio_dev);

> +	if (!readval)

Perhaps positive conditional?

	if (readval)
		return readval;
	return writeval;

> +		return regmap_write(priv->regmap, reg, writeval);
> +
> +	return regmap_read(priv->regmap, reg, readval);
> +}

...

> +	/* Power down channels that aren't in use */
> +	for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
> +		regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
> +				   ADS1298_MASK_CH_PD,
> +				   test_bit(i, scan_mask) ?
> +							0 : ADS1298_MASK_CH_PD);

Broken indentation.

> +	}

...

> +static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
> +{
> +	unsigned long flags;
> +
> +	/* Notify we're no longer waiting for the SPI transfer to complete */
> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
> +	priv->rdata_xfer_busy = false;
> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);

Perhaps switch to use guard()?
(And scoped_guard() where it makes sense.)

> +}

> +		/* Transfer 24-bit value into 32-bit array */
> +		memcpy(bounce + 1, data, 3);

Hmm... Wouldn't get_unaligned_..24() work here better?

> +		bounce += 4;

If so, you can iterate over u32 members directly without this += 4.

...

> +static const char *ads1298_family_name(unsigned int id)
> +{
> +	switch (id & 0xe0) {

GENMASK() ?

> +	case 0x80:
> +		return "ADS129x";
> +	case 0xc0:
> +		return "ADS129xR";

Can we have these all magics be defined?

> +	default:
> +		return "(unknown)";
> +	}
> +}

...

> +	if ((val & 0x18) == 0x10) {

Ditto.

> +		dev_info(dev, "Found %s, %d channels\n",
> +			 ads1298_family_name(val),
> +			 4 + 2 * (val & 0x07));

Ditto for 0x07.

> +	} else {
> +		dev_err(dev, "Unknown ID: 0x%x\n", val);
> +		return -ENODEV;
> +	}

...

> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> +	if (indio_dev == NULL)

We do !indio_dev in this case.

> +		return -ENOMEM;

...

> +		ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> +					       priv->reg_vref);

One line?


> +		if (ret)
> +			return ret;

...

> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Failed to get clk\n");

Ditto.

...

> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable avdd regulator\n");

Ditto.

...

> +	ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> +				       priv->reg_avdd);

Ditto.

...

> +	priv->regmap = devm_regmap_init(dev, NULL, priv,
> +					&ads1298_regmap_config);

Ditto.

...

> +	/* Avoid giving all the same name, iio scope doesn't handle that well */

IIO

> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
> +					 spi_get_device_id(spi)->name,
> +					 dev_name(dev));

No error check?

> +	if (reset_gpio) {
> +		udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */

How do you know it's 2MHz? Is it always the same on all platforms / setups?

> +		gpiod_set_value(reset_gpio, 0);
> +	} else {
> +		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "RESET failed\n");
> +	}
> +	/* Wait 18 clock cycles for reset command to complete */

Ditto.

> +	udelay(9);

...

> +	ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,

&spi->dev is different to dev?

> +			       IRQF_TRIGGER_FALLING, indio_dev->name,
> +			       indio_dev);

...

> +	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
> +					  &ads1298_setup_ops);

Ditto.

...

> +static const struct spi_device_id ads1298_id[] = {
> +	{ "ads1298",  },

Inner comma is not needed.

> +	{ }
> +};

...

> +static const struct of_device_id ads1298_of_table[] = {
> +	{ .compatible = "ti,ads1298" },
> +	{ },

Drop the comma in terminator entry.

> +};

On Wed, Dec 13, 2023 at 10:47:21AM +0100, Mike Looijmans 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.

I think the commit subject and body here were accidentally copy-pasted
from the driver patch. Patches for bindings should avoid talking about
drivers and focus on the harware (unless we are talking about LEDs or
motors etc)

> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
>  .../bindings/iio/adc/ti,ads1298.yaml          | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> new file mode 100644
> index 000000000000..7a160ba721eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments' ads1298 medical ADC chips
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads1298
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cpha: true
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description:
> +      Analog power supply, voltage between AVDD and AVSS. When providing a
> +      symmetric +/- 2.5V, the regulator should report 5V.
> +
> +  vref-supply:
> +    description:
> +      Optional reference voltage. If omitted, internal reference is used,
> +      depending on analog supply this is 2.4 or 4V.

It may be worth mentioning here what the conditions for the internal
reference being 2.4 or 4 volts actually are.

> +
> +  clocks:
> +    description: Optional 2.048 MHz external source clock on CLK pin
> +    maxItems: 1
> +
> +  clock-names:
> +    const: clk

Since you have only one clock, having clock-names (especially with a
name like "clk") is pointless IMO.

Generally though, this patch looks good to me.

Cheers,
Conor.

> +  interrupts:
> +    description: Interrupt on DRDY pin, triggers on falling edge
> +    maxItems: 1
> +
> +  label: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - avdd-supply
> +  - interrupts
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@1 {
> +          reg = <1>;
> +          compatible = "ti,ads1298";
> +          label = "ads1298-1-ecg";
> +          avdd-supply = <&reg_iso_5v_a>;
> +          clock-names = "clk";
> +          clocks = <&clk_ads1298>;
> +          interrupt-parent = <&gpio0>;
> +          interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
> +          spi-max-frequency = <20000000>;
> +          spi-cpha;
> +        };
> +    };
> +...
> -- 
> 2.34.1
> 
> 
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topic.nl
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail

On Wed, 13 Dec 2023 16:23:43 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Wed, Dec 13, 2023 at 10:47:21AM +0100, Mike Looijmans 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.  
> 
> I think the commit subject and body here were accidentally copy-pasted
> from the driver patch. Patches for bindings should avoid talking about
> drivers and focus on the harware (unless we are talking about LEDs or
> motors etc)
> 
> > 
> > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> > 
> > ---
> > 
> >  .../bindings/iio/adc/ti,ads1298.yaml          | 80 +++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> > new file mode 100644
> > index 000000000000..7a160ba721eb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Texas Instruments' ads1298 medical ADC chips
> > +
> > +maintainers:
> > +  - Mike Looijmans <mike.looijmans@topic.nl>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ti,ads1298
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cpha: true
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  avdd-supply:
> > +    description:
> > +      Analog power supply, voltage between AVDD and AVSS. When providing a
> > +      symmetric +/- 2.5V, the regulator should report 5V.

Any precedence in tree for doing this?  I thought we had bindings that required negative
supplies to be specified separately if present - so this would need to be 2 
supplies. e.g.
https://elixir.bootlin.com/linux/v6.7-rc5/source/Documentation/devicetree/bindings/iio/adc/ti,adc12138.yaml#L37


> > +
> > +  vref-supply:
> > +    description:
> > +      Optional reference voltage. If omitted, internal reference is used,
> > +      depending on analog supply this is 2.4 or 4V.  
> 
> It may be worth mentioning here what the conditions for the internal
> reference being 2.4 or 4 volts actually are.
> 
> > +
> > +  clocks:
> > +    description: Optional 2.048 MHz external source clock on CLK pin
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: clk  
> 
> Since you have only one clock, having clock-names (especially with a
> name like "clk") is pointless IMO.
> 
> Generally though, this patch looks good to me.
> 
> Cheers,
> Conor.
> 
> > +  interrupts:
> > +    description: Interrupt on DRDY pin, triggers on falling edge
> > +    maxItems: 1
> > +
> > +  label: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - avdd-supply
> > +  - interrupts
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@1 {
> > +          reg = <1>;
> > +          compatible = "ti,ads1298";
> > +          label = "ads1298-1-ecg";
> > +          avdd-supply = <&reg_iso_5v_a>;
> > +          clock-names = "clk";
> > +          clocks = <&clk_ads1298>;
> > +          interrupt-parent = <&gpio0>;
> > +          interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
> > +          spi-max-frequency = <20000000>;
> > +          spi-cpha;
> > +        };
> > +    };
> > +...
> > -- 
> > 2.34.1
> > 
> > 
> > Met vriendelijke groet / kind regards,
> > 
> > Mike Looijmans
> > System Expert
> > 
> > 
> > TOPIC Embedded Products B.V.
> > Materiaalweg 4, 5681 RJ Best
> > The Netherlands
> > 
> > T: +31 (0) 499 33 69 69
> > E: mike.looijmans@topic.nl
> > W: www.topic.nl
> > 
> > Please consider the environment before printing this e-mail  
>

On Wed, 13 Dec 2023 10:47:22 +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,

Various small things inline - some of which probably overlap with Andy's
comments.

The larger one is I don't follow why this does manual protection against
concurrent handling of the result of an IRQ.  Much easier to just use
an IRQ threaded handler and IRQF_ONESHOT to mask the irq entirely until
after the initial handling is done.  If that doesn't work for some reason
then more comments needed to explain why!

Jonathan

> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> new file mode 100644
> index 000000000000..54466285063f
> --- /dev/null

> + */
> +#define ADS1298_SPI_BUS_SPEED_SLOW	ADS1298_CLK_RATE
> +/* For reading and writing registers, we need a 3-byte buffer */
> +#define ADS1298_SPI_CMD_BUFFER_SIZE	3
> +/* Outputs status word and 8 samples of 24 bits each, plus the command byte */
> +#define ADS1298_SPI_RDATA_BUFFER_SIZE	((ADS1298_MAX_CHANNELS + 1) * 3 + 1)
> +
> +struct ads1298_private {
> +	const struct ads1298_chip_info	*chip_info;

Looks like a tab whereas other cases are spaces.

> +	struct spi_device *spi;
> +	struct regulator *reg_avdd;
> +	struct regulator *reg_vref;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +	struct completion completion;
> +	struct iio_trigger *trig;
> +	struct spi_transfer rdata_xfer;
> +	struct spi_message rdata_msg;
> +	spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
> +	bool rdata_xfer_busy;
> +
> +	/* Temporary storage for demuxing data after SPI transfer */
> +	u8 bounce_buffer[ADS1298_MAX_CHANNELS * 4];
> +
> +	/* For synchronous SPI exchanges (read/write registers) */
> +	u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] ____cacheline_aligned;

Not sufficient on alignment - see discussion on patch series for IIO_DMA_MINALIGN
and use that instead. Boils down to there being a few systems with cacheline lengths
are different across caches (that's common) AND this value is the smallest one whereas
coherency is out past a cache with a bigger cache line length.

> +
> +	/* Buffer used for incoming SPI data */
> +	u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE] ____cacheline_aligned;
Generally drivers don't allow DMA transfers that will trample on data in another
DMA transfer - as such it's only normally necessary to force the whole set of buffers
to sit in their own cacheline.  Hence we mark only first one as needing alignment.
> +	/* Contains the RDATA command and zeroes to clock out */
> +	u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE] ____cacheline_aligned;
> +};


...

> +static const u8 ads1298_pga_settings[] = { 6, 1, 2, 3, 4, 8, 12 };
> +
> +static int ads1298_get_scale(struct ads1298_private *priv,
> +			     int channel, int *val, int *val2)
> +{
> +	int ret;
> +	unsigned int regval;
> +	u8 gain;
> +
> +	if (priv->reg_vref) {
> +		ret = regulator_get_voltage(priv->reg_vref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000; /* Convert to millivolts */
> +	} else {
> +		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG3, &regval);
> +		if (ret)
> +			return ret;
> +
> +		/* Refererence in millivolts */
> +		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ? 4000 : 2400;
> +	}
> +
> +	ret = regmap_read(priv->regmap, ADS1298_REG_CHnSET(channel), &regval);
> +	if (ret)
> +		return ret;
> +
> +	gain = ads1298_pga_settings[FIELD_GET(ADS1298_MASK_CH_PGA, regval)];
> +	*val /= gain; /* Full scale is VREF / gain */
> +
> +	*val2 = 23; /* Signed 24-bit, max amplitude is 2^23 */
> +
> +	return IIO_VAL_FRACTIONAL_LOG2;
> +}
> +
> +

Trivial: Single blank line only.

> +static int ads1298_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		/* When busy, just peek in the buffer */
> +		if (!iio_buffer_enabled(indio_dev)) {
Races - you need to claim direct mode to prevent that.

> +			ret = ads1298_read_one(priv, chan->scan_index);
> +			if (ret)
> +				return ret;
> +		}
> +		*val = sign_extend32(get_unaligned_be24(
> +					priv->rx_buffer + chan->address), 23);
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = ads1298_get_scale(priv, chan->channel, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = ads1298_get_samp_freq(priv, val);
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
> +		if (!ret) {
> +			ret = IIO_VAL_INT;
> +			*val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
> +		}
Takes a line or 2 more but better to keep consistent style on error handling
		if (ret)
			return ret;
		*val = 16 << (*val & ADS1298_MASK_CONFIG1_DR); /* Note fewer () */
		return IIO_VAL_INT;


> +		break;
> +	default:
> +		ret = -EINVAL;
Always return early if there is nothing to do.  Makes review easier as no
need to sanity check what happens for each path if it's already returned.

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int ads1298_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = ads1298_set_samp_freq(priv, val);

return ads1298_set_samp_freq()

> +		break;
> +	default:
> +		ret = -EINVAL;

return -EINVAL;

> +		break;
> +	}
> +
> +	return ret;
> +}

..
> +static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			      unsigned int writeval, unsigned int *readval)
> +{
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +
> +	if (!readval)
> +		return regmap_write(priv->regmap, reg, writeval);
> +
> +	return regmap_read(priv->regmap, reg, readval);
> +}
> +
> +static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
> +				    const unsigned long *scan_mask)
> +{
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int i;
> +
> +	/* Power down channels that aren't in use */
> +	for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
> +		regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),

Always check regmap accesses.  There's a horrible bus under this ;)

> +				   ADS1298_MASK_CH_PD,
> +				   test_bit(i, scan_mask) ?
> +							0 : ADS1298_MASK_CH_PD);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info ads1298_info = {
> +	.read_raw = &ads1298_read_raw,
> +	.write_raw = &ads1298_write_raw,
> +	.update_scan_mode = &ads1298_update_scan_mode,
> +	.debugfs_reg_access = &ads1298_reg_access,
> +};
> +
> +static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
> +{
> +	unsigned long flags;
> +
> +	/* Notify we're no longer waiting for the SPI transfer to complete */
> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
> +	priv->rdata_xfer_busy = false;
> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
> +}
> +
> +static void ads1298_rdata_complete(void *context)
> +{
> +	struct iio_dev *indio_dev = context;
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int scan_index;
> +	u8 *bounce = priv->bounce_buffer;
> +
> +	if (!iio_buffer_enabled(indio_dev)) {

This can race I think with a transition into buffered mode.
You should be able to use iio_device_claim_direct_mode
here().  That will fail if you are in buffered mode but if it succeeds
it will hold us in non buffered mode until it is released.
 
> +		/* 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;
> +
> +		/* Transfer 24-bit value into 32-bit array */
> +		memcpy(bounce + 1, data, 3);
> +		bounce += 4;
> +	}
> +
> +	/* rx_buffer can be overwritten from this point on */
> +	ads1298_rdata_unmark_busy(priv);
> +
> +	iio_push_to_buffers(indio_dev, priv->bounce_buffer);
> +}
> +
> +static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +	struct ads1298_private *priv = iio_priv(indio_dev);
> +	unsigned long flags;
> +	bool wasbusy;
> +
> +	/* Prevent that we submit a message that was still in progress */
> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
> +	wasbusy = priv->rdata_xfer_busy;
> +	priv->rdata_xfer_busy = true;
> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
> +
> +	if (!wasbusy)
> +		spi_async(priv->spi, &priv->rdata_msg);
> +
> +	return IRQ_HANDLED;

Can you not use the IRQF_ONESHOT and an IRQ thread to deal with what
you are using wasbusy for here?  I'm not keen on a reply saying we've
handled the interrupt when we really haven't finished with it.

This looks like a classic data ready trigger as well. Why handle
it without the trigger infrastructure?

> +};



> +
> +static const struct iio_buffer_setup_ops ads1298_setup_ops = {
> +	.postenable = &ads1298_buffer_postenable,
> +	.predisable = &ads1298_buffer_predisable,
> +};

...


> +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;
> +
> +	if ((val & 0x18) == 0x10) {

FIELD_GET() and appropriate mask defines.

> +		dev_info(dev, "Found %s, %d channels\n",
> +			 ads1298_family_name(val),
> +			 4 + 2 * (val & 0x07));
> +	} else {
> +		dev_err(dev, "Unknown ID: 0x%x\n", val);

In order to support fall back DT compatibles etc, we generally only
print a message on a missmatched ID.  It might be for a new fully
compatible device with a DT binding deliberately specifying that
the older device is fine for matching against.
This is something we are slowly fixing up in older drives after
some long discussions with the DT maintainers a few years back.

> +		return -ENODEV;
> +	}
> +
> +	/* Enable internal test signal, double amplitude, double frequency */
> +	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);
> +

Always check return values unless you expect them to fail (in which case document
that).

> +	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
> +	if (!priv->reg_vref) {
> +		/* Enable internal reference */
> +		val |= ADS1298_MASK_CONFIG3_PWR_REFBUF;

#FIELD_PREP() and masks preferred for consistency with multi bit fields.

> +		/* 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;
> +	}
> +	regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
> +
> +	for (val = 0; val < ADS1298_MAX_CHANNELS; val++) {
> +		/* Set mux to analog input, PGA = 6x */

Use appropriate mask defines and FIELD_PREP() so that 0x00 doesn't
need a comment here as the define makes it clear what is going on.

> +		regmap_write(priv->regmap, ADS1298_REG_CHnSET(val), 0x00);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ads1298_probe(struct spi_device *spi)
> +{
> +	struct ads1298_private *priv;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	struct gpio_desc *reset_gpio;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(indio_dev);
> +
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, ret, "Cannot get reset GPIO\n");
> +
> +	priv->reg_avdd = devm_regulator_get(dev, "avdd");
> +	if (IS_ERR(priv->reg_avdd))
> +		return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
> +				     "Failed to get avdd regulator\n");
> +
> +	/* VREF can be supplied externally. Otherwise use internal reference */
> +	priv->reg_vref = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(priv->reg_vref)) {
> +		if (PTR_ERR(priv->reg_vref) == -ENODEV)
> +			priv->reg_vref = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
> +					     "Failed to get vref regulator\n");
> +	} else {
> +		ret = regulator_enable(priv->reg_vref);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> +					       priv->reg_vref);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	priv->clk = devm_clk_get_enabled(dev, "clk");
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Failed to get clk\n");
> +
> +	ret = regulator_enable(priv->reg_avdd);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable avdd regulator\n");
> +
> +	ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> +				       priv->reg_avdd);
> +	if (ret)
> +		return ret;
I'd group the getting of this regulator with it's enabling.  That will bring it
inline with how you handle vref.


> +
> +	priv->spi = spi;
> +	init_completion(&priv->completion);
> +	spin_lock_init(&priv->irq_busy_lock);
> +	priv->regmap = devm_regmap_init(dev, NULL, priv,
> +					&ads1298_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	priv->tx_buffer[0] = ADS1298_CMD_RDATA;
> +	priv->rdata_xfer.tx_buf = priv->tx_buffer;
> +	priv->rdata_xfer.rx_buf = priv->rx_buffer;
> +	priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE;
> +	/* Must keep CS low for 4 clocks */
> +	priv->rdata_xfer.delay.value = 2;
> +	priv->rdata_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
> +	spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1);
> +	priv->rdata_msg.complete = &ads1298_rdata_complete;
> +	priv->rdata_msg.context = indio_dev;
> +
> +	/* Avoid giving all the same name, iio scope doesn't handle that well */

That's intentional - name is the type of chip, not a unique name.
If you need a way to identify the device then use the path of the parent
which will associate this with the underlying bus device and hence provide
a stable reference.  We deliberately don't attempt to provide a different way
to identify the device. readlink is your friend in userspace.
Some drivers got through review with naming coming from elsewhere but this
is the part number of the device.  With hindsight the ABI should have reflected
that to avoid confusion but we are stuck with 'name'.

> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
> +					 spi_get_device_id(spi)->name,
> +					 dev_name(dev));
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	indio_dev->channels = ads1298_channels;
> +	indio_dev->num_channels = ADS1298_MAX_CHANNELS;
> +	indio_dev->info = &ads1298_info;
> +
> +	if (reset_gpio) {
> +		udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */
> +		gpiod_set_value(reset_gpio, 0);
> +	} else {
> +		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "RESET failed\n");
> +	}
> +	/* Wait 18 clock cycles for reset command to complete */
> +	udelay(9);
> +
> +	ret = ads1298_init(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Init failed\n");
> +
> +	ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,
> +			       IRQF_TRIGGER_FALLING, indio_dev->name,
> +			       indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
> +					  &ads1298_setup_ops);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct spi_device_id ads1298_id[] = {
> +	{ "ads1298",  },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ads1298_id);
> +
> +static const struct of_device_id ads1298_of_table[] = {
> +	{ .compatible = "ti,ads1298" },
> +	{ },
No point in the trailing comma.
> +};
> +MODULE_DEVICE_TABLE(of, ads1298_of_table);

Thanks,

Jonathan

On Wed, 13 Dec 2023 10:47:21 +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>
> 
> ---
> 
>  .../bindings/iio/adc/ti,ads1298.yaml          | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> new file mode 100644
> index 000000000000..7a160ba721eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments' ads1298 medical ADC chips
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads1298
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cpha: true
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description:
> +      Analog power supply, voltage between AVDD and AVSS. When providing a
> +      symmetric +/- 2.5V, the regulator should report 5V.
Commented on in other thread.
> +
> +  vref-supply:
> +    description:
> +      Optional reference voltage. If omitted, internal reference is used,
> +      depending on analog supply this is 2.4 or 4V.
> +
There is a dvdd-supply as well.  Might be others. Makes sure to document them all.

Should probably also document the gpios as the binding should attempt to be as complete
as possible independent of what the driver currently supports.

Lots of complex options for this device and the use of those pins, so maybe something
that can be left for now - but the patch description should then mention that is intentional.

Jonathan

> +  clocks:
> +    description: Optional 2.048 MHz external source clock on CLK pin
> +    maxItems: 1
> +
> +  clock-names:
> +    const: clk
> +
> +  interrupts:
> +    description: Interrupt on DRDY pin, triggers on falling edge
> +    maxItems: 1
> +
> +  label: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - avdd-supply
> +  - interrupts
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@1 {
> +          reg = <1>;
> +          compatible = "ti,ads1298";
> +          label = "ads1298-1-ecg";
> +          avdd-supply = <&reg_iso_5v_a>;
> +          clock-names = "clk";
> +          clocks = <&clk_ads1298>;
> +          interrupt-parent = <&gpio0>;
> +          interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
> +          spi-max-frequency = <20000000>;
> +          spi-cpha;
> +        };
> +    };
> +...

See below.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail
On 14-12-2023 11:22, Jonathan Cameron wrote:
> On Wed, 13 Dec 2023 16:23:43 +0000
> Conor Dooley <conor@kernel.org> wrote:
> 
>> On Wed, Dec 13, 2023 at 10:47:21AM +0100, Mike Looijmans 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.
>>
>> I think the commit subject and body here were accidentally copy-pasted
>> from the driver patch. Patches for bindings should avoid talking about
>> drivers and focus on the harware (unless we are talking about LEDs or
>> motors etc)

Yeah, that's what happened. Will fix in next version.

>>
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>
>>> ---
>>>
>>>   .../bindings/iio/adc/ti,ads1298.yaml          | 80 +++++++++++++++++++
>>>   1 file changed, 80 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
>>> new file mode 100644
>>> index 000000000000..7a160ba721eb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Texas Instruments' ads1298 medical ADC chips
>>> +
>>> +maintainers:
>>> +  - Mike Looijmans <mike.looijmans@topic.nl>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ti,ads1298
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  spi-cpha: true
>>> +
>>> +  reset-gpios:
>>> +    maxItems: 1
>>> +
>>> +  avdd-supply:
>>> +    description:
>>> +      Analog power supply, voltage between AVDD and AVSS. When providing a
>>> +      symmetric +/- 2.5V, the regulator should report 5V.
> 
> Any precedence in tree for doing this?  I thought we had bindings that required negative
> supplies to be specified separately if present - so this would need to be 2
> supplies. e.g.
> https://elixir.bootlin.com/linux/v6.7-rc5/source/Documentation/devicetree/bindings/iio/adc/ti,adc12138.yaml#L37
> 

Given that some serious thought...

Splitting into positive and negative supplies would make sense if the chip 
would have terminals for positive, negative and ground. Which it does not 
have, there's only positive and negative (which the datasheet misleadingly 
calls "analog ground").

The analog voltage supplied to the chip has no effect on its outputs, the 
analog supply must be connected between the AVDD and AVSS pins. Its relation 
to analog ground is not relevant, so whether the voltages are +5/0 or 
+2.5/-2.5 or +4/-1 or whatever does not affect the output of the ADC, which 
only reports the difference between its "p" and "n" input signals. It only 
affects the range of the inputs, as it cannot measure (p or n) outside the 
analog supply.


> 
>>> +
>>> +  vref-supply:
>>> +    description:
>>> +      Optional reference voltage. If omitted, internal reference is used,
>>> +      depending on analog supply this is 2.4 or 4V.
>>
>> It may be worth mentioning here what the conditions for the internal
>> reference being 2.4 or 4 volts actually are.

Will do (there's a 4.4 V treshold).


>>
>>> +
>>> +  clocks:
>>> +    description: Optional 2.048 MHz external source clock on CLK pin
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    const: clk
>>
>> Since you have only one clock, having clock-names (especially with a
>> name like "clk") is pointless IMO.

True.

>>
>> Generally though, this patch looks good to me.
>>
>> Cheers,
>> Conor.
>>
>>> +  interrupts:
>>> +    description: Interrupt on DRDY pin, triggers on falling edge
>>> +    maxItems: 1
>>> +
>>> +  label: true
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - avdd-supply
>>> +  - interrupts
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    spi {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        adc@1 {
>>> +          reg = <1>;
>>> +          compatible = "ti,ads1298";
>>> +          label = "ads1298-1-ecg";
>>> +          avdd-supply = <&reg_iso_5v_a>;
>>> +          clock-names = "clk";
>>> +          clocks = <&clk_ads1298>;
>>> +          interrupt-parent = <&gpio0>;
>>> +          interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
>>> +          spi-max-frequency = <20000000>;
>>> +          spi-cpha;
>>> +        };
>>> +    };
>>> +...
>>> -- 
>>> 2.34.1
>>>
>>>
>>> Met vriendelijke groet / kind regards,
>>>
>>> Mike Looijmans
>>> System Expert
>>>
>>>
>>> TOPIC Embedded Products B.V.
>>> Materiaalweg 4, 5681 RJ Best
>>> The Netherlands
>>>
>>> T: +31 (0) 499 33 69 69
>>> E: mike.looijmans@topic.nl
>>> W: www.topic.nl
>>>
>>> Please consider the environment before printing this e-mail
>>
>

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail
On 14-12-2023 12:11, Jonathan Cameron wrote:
> On Wed, 13 Dec 2023 10:47:21 +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>
>>
>> ---
>>
>>   .../bindings/iio/adc/ti,ads1298.yaml          | 80 +++++++++++++++++++
>>   1 file changed, 80 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
>> new file mode 100644
>> index 000000000000..7a160ba721eb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments' ads1298 medical ADC chips
>> +
>> +maintainers:
>> +  - Mike Looijmans <mike.looijmans@topic.nl>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,ads1298
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  spi-cpha: true
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  avdd-supply:
>> +    description:
>> +      Analog power supply, voltage between AVDD and AVSS. When providing a
>> +      symmetric +/- 2.5V, the regulator should report 5V.
> Commented on in other thread.
>> +
>> +  vref-supply:
>> +    description:
>> +      Optional reference voltage. If omitted, internal reference is used,
>> +      depending on analog supply this is 2.4 or 4V.
>> +
> There is a dvdd-supply as well.  Might be others. Makes sure to document them all.

Extra supplies make sense, I'll add them.

> 
> Should probably also document the gpios as the binding should attempt to be as complete
> as possible independent of what the driver currently supports.
> 
> Lots of complex options for this device and the use of those pins, so maybe something
> that can be left for now - but the patch description should then mention that is intentional.

The device has so many options for connecting stuff... It's indeed possible to 
(also) use it as a GPIO expander and as a clock source and more...

I'll put it in the patch description that the definition is incomplete by design.

The main reason I'm submitting is that this is about the third time I've 
written a driver for this chip, and I'm sure that other companies are writing 
their own as well. I'm hoping this will result in some joint effort to 
properly support it...


> 
> Jonathan
> 
>> +  clocks:
>> +    description: Optional 2.048 MHz external source clock on CLK pin
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: clk
>> +
>> +  interrupts:
>> +    description: Interrupt on DRDY pin, triggers on falling edge
>> +    maxItems: 1
>> +
>> +  label: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - avdd-supply
>> +  - interrupts
>> +
>> +allOf:
>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    spi {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        adc@1 {
>> +          reg = <1>;
>> +          compatible = "ti,ads1298";
>> +          label = "ads1298-1-ecg";
>> +          avdd-supply = <&reg_iso_5v_a>;
>> +          clock-names = "clk";
>> +          clocks = <&clk_ads1298>;
>> +          interrupt-parent = <&gpio0>;
>> +          interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
>> +          spi-max-frequency = <20000000>;
>> +          spi-cpha;
>> +        };
>> +    };
>> +...
>

> >>> +  avdd-supply:
> >>> +    description:
> >>> +      Analog power supply, voltage between AVDD and AVSS. When providing a
> >>> +      symmetric +/- 2.5V, the regulator should report 5V.  
> > 
> > Any precedence in tree for doing this?  I thought we had bindings that required negative
> > supplies to be specified separately if present - so this would need to be 2
> > supplies. e.g.
> > https://elixir.bootlin.com/linux/v6.7-rc5/source/Documentation/devicetree/bindings/iio/adc/ti,adc12138.yaml#L37
> >   
> 
> Given that some serious thought...
> 
> Splitting into positive and negative supplies would make sense if the chip 
> would have terminals for positive, negative and ground. Which it does not 
> have, there's only positive and negative (which the datasheet misleadingly 
> calls "analog ground").
> 
> The analog voltage supplied to the chip has no effect on its outputs, the 
> analog supply must be connected between the AVDD and AVSS pins. Its relation 
> to analog ground is not relevant, so whether the voltages are +5/0 or 
> +2.5/-2.5 or +4/-1 or whatever does not affect the output of the ADC, which 
> only reports the difference between its "p" and "n" input signals. It only 
> affects the range of the inputs, as it cannot measure (p or n) outside the 
> analog supply.
> 
Ah.  Not having seen any isolators, I assumed this device was running
against a common ground with the digital side of things.
AVSS to DGND is between -3 and 0.2 so I think there is a ground even if it's
not directly connected.

Given this only operates in differential mode the fact both signals are
referenced to +-vref which is from VASS doesn't matter in the end and any
offset goes away.

So yes I think I agree.  This should be a single supply.

Jonathan

> > 
> > Should probably also document the gpios as the binding should attempt to be as complete
> > as possible independent of what the driver currently supports.
> > 
> > Lots of complex options for this device and the use of those pins, so maybe something
> > that can be left for now - but the patch description should then mention that is intentional.  
> 
> The device has so many options for connecting stuff... It's indeed possible to 
> (also) use it as a GPIO expander and as a clock source and more...
> 
> I'll put it in the patch description that the definition is incomplete by design.
> 
> The main reason I'm submitting is that this is about the third time I've 
> written a driver for this chip, and I'm sure that other companies are writing 
> their own as well. I'm hoping this will result in some joint effort to 
> properly support it...
> 

Fair enough. Incomplete bindings aren't ideal, but definitely better than no bindings
in my view.

Jonathan

Hi Andy,

Took me a while to get back to this. Incorporated all of your comments, 
except that I seem to be unable to count characters. Most places, I have 
more than 80 characters in a line it I don't indent as I did. I'll try 
to improve my indents...


On 13-12-2023 15:55, Andy Shevchenko wrote:
> On Wed, Dec 13, 2023 at 10:47:22AM +0100, Mike Looijmans 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.
> ...
>
>> +config TI_ADS1298
>> +	tristate "Texas Instruments ADS1298"
>> +	depends on SPI
>> +	select IIO_BUFFER
>> +	default y
> Huh?!
>
>> +	help
>> +	  If you say yes here you get support for Texas Instruments ADS1298
>> +	  medical ADC chips
>> +
>> +	  This driver can also be built as a module. If so, the module will be
>> +	  called ti-ads1298.
> ...
>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
> Is it used as a proxy? (At least for array_size.h)
> Please use real headers in such case.
>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
> This is interesting grouping, but okay, I understand the point.
>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +#include <linux/iio/sysfs.h>
> ...
>
>> +#define ADS1298_CLK_RATE	2048000
> Units? _HZ ?
>
> ...
>
>> +/* Outputs status word and 8 samples of 24 bits each, plus the command byte */
> /* Outputs status word and 8 24-bit samples, plus the command byte */
>
> a bit shorter.
>
>> +#define ADS1298_SPI_RDATA_BUFFER_SIZE	((ADS1298_MAX_CHANNELS + 1) * 3 + 1)
> ...
>
>> +#define ADS1298_CHAN(index)					\
>> +{								\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.channel = index,					\
>> +	.address = 3 * index + 4,				\
> Hmm... does this 3 have a distinct definition?
>
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>> +			      BIT(IIO_CHAN_INFO_SCALE),		\
> Can be written as below
>
> 	.info_mask_separate =					\
> 		BIT(IIO_CHAN_INFO_RAW) |			\
> 		BIT(IIO_CHAN_INFO_SCALE),			\
>
>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> 	.info_mask_shared_by_all =				\
> 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |			\
> 		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
>
>> +	.scan_index = index,					\
>> +	.scan_type = {						\
>> +		.sign = 's',					\
>> +		.realbits = 24,					\
>> +		.storagebits = 32,				\
>> +		.endianness = IIO_BE,				\
>> +	},							\
>> +}
> ...
>
>> +static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
>> +{
>> +	struct spi_transfer cmd_xfer = {
>> +		.tx_buf = priv->cmd_buffer,
>> +		.rx_buf = priv->cmd_buffer,
>> +		.len = 1,
> 		sizeof(command) ?
>
>> +		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
>> +		.delay.value = 2,
>> +		.delay.unit = SPI_DELAY_UNIT_USECS,
> 		.delay = {
> 			.value = ...
> 			.unit = ...
> 		},
>
>> +	};
>> +	priv->cmd_buffer[0] = command;
>> +
>> +	return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
>> +}
> ...
>
>> +	/* Cannot take longer than 40ms (250Hz) */
>> +	ret = wait_for_completion_timeout(&priv->completion,
>> +					  msecs_to_jiffies(50));
> One line?
>
>> +	if (!ret)
>> +		return -ETIMEDOUT;
> ...
>
>> +	if (cfg & ADS1298_MASK_CONFIG1_HR)
>> +		rate >>= 6; /* HR mode */
>> +	else
>> +		rate >>= 7; /* LP mode */
> Are those magic numbers defined?
>
> ...
>
>> +	factor = (rate >> 6) / val;
> Is it the same 6 semantically as above?
>
> ...
>
>> +	return IIO_VAL_FRACTIONAL_LOG2;
>> +}
>> +
>> +
> One blank line is enough.
>
>> +		*val = sign_extend32(get_unaligned_be24(
>> +					priv->rx_buffer + chan->address), 23);
> Strange indentation.
>
> 		*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
> 				     23);

Doesn't fit, first line is 83 characters by my count...


> ...
>
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
>> +		if (!ret) {
> Why not using standard pattern?
>
> 		if (ret)
> 			return ret;
>
> (see below)
>
>> +			ret = IIO_VAL_INT;
>> +			*val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
> Outer parentheses are redundant.
>
>> +		}
>> +		break;
> 		return IIO_VAL_INT;
>
>
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
> return directly.
>
>> +	}
>> +	return ret;
> It will gone.
>
> ...
>
>> +static int ads1298_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan, int val,
>> +			     int val2, long mask)
>> +{
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	int ret;
> No need, just return directly.
>
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		ret = ads1298_set_samp_freq(priv, val);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
> ...
>
>> +static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>> +			      unsigned int writeval, unsigned int *readval)
>> +{
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	if (!readval)
> Perhaps positive conditional?
>
> 	if (readval)
> 		return readval;
> 	return writeval;
>
>> +		return regmap_write(priv->regmap, reg, writeval);
>> +
>> +	return regmap_read(priv->regmap, reg, readval);
>> +}
> ...
>
>> +	/* Power down channels that aren't in use */
>> +	for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
>> +		regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
>> +				   ADS1298_MASK_CH_PD,
>> +				   test_bit(i, scan_mask) ?
>> +							0 : ADS1298_MASK_CH_PD);
> Broken indentation.
>
>> +	}
> ...
>
>> +static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
>> +{
>> +	unsigned long flags;
>> +
>> +	/* Notify we're no longer waiting for the SPI transfer to complete */
>> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
>> +	priv->rdata_xfer_busy = false;
>> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
> Perhaps switch to use guard()?
> (And scoped_guard() where it makes sense.)
>
>> +}
>> +		/* Transfer 24-bit value into 32-bit array */
>> +		memcpy(bounce + 1, data, 3);
> Hmm... Wouldn't get_unaligned_..24() work here better?
>
>> +		bounce += 4;
> If so, you can iterate over u32 members directly without this += 4.
>
> ...
>
>> +static const char *ads1298_family_name(unsigned int id)
>> +{
>> +	switch (id & 0xe0) {
> GENMASK() ?
>
>> +	case 0x80:
>> +		return "ADS129x";
>> +	case 0xc0:
>> +		return "ADS129xR";
> Can we have these all magics be defined?
>
>> +	default:
>> +		return "(unknown)";
>> +	}
>> +}
> ...
>
>> +	if ((val & 0x18) == 0x10) {
> Ditto.
>
>> +		dev_info(dev, "Found %s, %d channels\n",
>> +			 ads1298_family_name(val),
>> +			 4 + 2 * (val & 0x07));
> Ditto for 0x07.
>
>> +	} else {
>> +		dev_err(dev, "Unknown ID: 0x%x\n", val);
>> +		return -ENODEV;
>> +	}
> ...
>
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
>> +	if (indio_dev == NULL)
> We do !indio_dev in this case.
>
>> +		return -ENOMEM;
> ...
>
>> +		ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
>> +					       priv->reg_vref);
> One line?
>
>
>> +		if (ret)
>> +			return ret;
> ...
>
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
>> +				     "Failed to get clk\n");
> Ditto.
>
> ...
>
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to enable avdd regulator\n");
> Ditto.
>
> ...
>
>> +	ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
>> +				       priv->reg_avdd);
> Ditto.
>
> ...
>
>> +	priv->regmap = devm_regmap_init(dev, NULL, priv,
>> +					&ads1298_regmap_config);
> Ditto.
>
> ...
>
>> +	/* Avoid giving all the same name, iio scope doesn't handle that well */
> IIO
>
>> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
>> +					 spi_get_device_id(spi)->name,
>> +					 dev_name(dev));
> No error check?
>
>> +	if (reset_gpio) {
>> +		udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */
> How do you know it's 2MHz? Is it always the same on all platforms / setups?
>
>> +		gpiod_set_value(reset_gpio, 0);
>> +	} else {
>> +		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret, "RESET failed\n");
>> +	}
>> +	/* Wait 18 clock cycles for reset command to complete */
> Ditto.
>
>> +	udelay(9);
> ...
>
>> +	ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,
> &spi->dev is different to dev?
>
>> +			       IRQF_TRIGGER_FALLING, indio_dev->name,
>> +			       indio_dev);
> ...
>
>> +	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
>> +					  &ads1298_setup_ops);
> Ditto.
>
> ...
>
>> +static const struct spi_device_id ads1298_id[] = {
>> +	{ "ads1298",  },
> Inner comma is not needed.
>
>> +	{ }
>> +};
> ...
>
>> +static const struct of_device_id ads1298_of_table[] = {
>> +	{ .compatible = "ti,ads1298" },
>> +	{ },
> Drop the comma in terminator entry.
>
>> +};

On 14-12-2023 12:06, Jonathan Cameron wrote:
> On Wed, 13 Dec 2023 10:47:22 +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,
>
> Various small things inline - some of which probably overlap with Andy's
> comments.


Working on it - Assume "yes to all" as my response on all suggestions, 
except for the IRQ handling...

>
> The larger one is I don't follow why this does manual protection against
> concurrent handling of the result of an IRQ.  Much easier to just use
> an IRQ threaded handler and IRQF_ONESHOT to mask the irq entirely until
> after the initial handling is done.  If that doesn't work for some reason
> then more comments needed to explain why!

Yeah, definitely needs comments, and a bit of code too...

This chip doesn't have a buffer, but it does "latch" the sample data 
when it receives a RDATA command (hence I use that in favor of RDATAC, 
which does not latch and might return corrupted data).

To keep the latency as low as possible, I want to immediately start the 
SPI transfer when the DRDY interrupt arrives. My experiments have shown 
a big difference there, when using a ONESHOT trigger, it failed to meet 
the deadline at even the lowest frequencies.

The next SPI transfer can start immediately after the data has been 
copied into the intermediate buffer for IIO, the handler need not wait 
for IIO to process the data.

When the DRDY interrupt arrives, and there's an SPI transfer still in 
progress, it's not too late yet, the "latch" may save us still. Once the 
SPI transfer completes and the data is in the intermediate buffer, we 
should immediately start a new SPI transaction to latch and fetch the 
next set. (This code is still missing in the current version)

Only when DRDY arrives a second time during an SPI transaction we missed 
the deadline and sample data was lost.

No further comments below from me, just kept the history for reference.


> Jonathan
>
>> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
>> new file mode 100644
>> index 000000000000..54466285063f
>> --- /dev/null
>> + */
>> +#define ADS1298_SPI_BUS_SPEED_SLOW	ADS1298_CLK_RATE
>> +/* For reading and writing registers, we need a 3-byte buffer */
>> +#define ADS1298_SPI_CMD_BUFFER_SIZE	3
>> +/* Outputs status word and 8 samples of 24 bits each, plus the command byte */
>> +#define ADS1298_SPI_RDATA_BUFFER_SIZE	((ADS1298_MAX_CHANNELS + 1) * 3 + 1)
>> +
>> +struct ads1298_private {
>> +	const struct ads1298_chip_info	*chip_info;
> Looks like a tab whereas other cases are spaces.
>
>> +	struct spi_device *spi;
>> +	struct regulator *reg_avdd;
>> +	struct regulator *reg_vref;
>> +	struct clk *clk;
>> +	struct regmap *regmap;
>> +	struct completion completion;
>> +	struct iio_trigger *trig;
>> +	struct spi_transfer rdata_xfer;
>> +	struct spi_message rdata_msg;
>> +	spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
>> +	bool rdata_xfer_busy;
>> +
>> +	/* Temporary storage for demuxing data after SPI transfer */
>> +	u8 bounce_buffer[ADS1298_MAX_CHANNELS * 4];
>> +
>> +	/* For synchronous SPI exchanges (read/write registers) */
>> +	u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] ____cacheline_aligned;
> Not sufficient on alignment - see discussion on patch series for IIO_DMA_MINALIGN
> and use that instead. Boils down to there being a few systems with cacheline lengths
> are different across caches (that's common) AND this value is the smallest one whereas
> coherency is out past a cache with a bigger cache line length.
>
>> +
>> +	/* Buffer used for incoming SPI data */
>> +	u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE] ____cacheline_aligned;
> Generally drivers don't allow DMA transfers that will trample on data in another
> DMA transfer - as such it's only normally necessary to force the whole set of buffers
> to sit in their own cacheline.  Hence we mark only first one as needing alignment.
>> +	/* Contains the RDATA command and zeroes to clock out */
>> +	u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE] ____cacheline_aligned;
>> +};
>
> ...
>
>> +static const u8 ads1298_pga_settings[] = { 6, 1, 2, 3, 4, 8, 12 };
>> +
>> +static int ads1298_get_scale(struct ads1298_private *priv,
>> +			     int channel, int *val, int *val2)
>> +{
>> +	int ret;
>> +	unsigned int regval;
>> +	u8 gain;
>> +
>> +	if (priv->reg_vref) {
>> +		ret = regulator_get_voltage(priv->reg_vref);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*val = ret / 1000; /* Convert to millivolts */
>> +	} else {
>> +		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG3, &regval);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Refererence in millivolts */
>> +		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ? 4000 : 2400;
>> +	}
>> +
>> +	ret = regmap_read(priv->regmap, ADS1298_REG_CHnSET(channel), &regval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gain = ads1298_pga_settings[FIELD_GET(ADS1298_MASK_CH_PGA, regval)];
>> +	*val /= gain; /* Full scale is VREF / gain */
>> +
>> +	*val2 = 23; /* Signed 24-bit, max amplitude is 2^23 */
>> +
>> +	return IIO_VAL_FRACTIONAL_LOG2;
>> +}
>> +
>> +
> Trivial: Single blank line only.
>
>> +static int ads1298_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		/* When busy, just peek in the buffer */
>> +		if (!iio_buffer_enabled(indio_dev)) {
> Races - you need to claim direct mode to prevent that.
>
>> +			ret = ads1298_read_one(priv, chan->scan_index);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +		*val = sign_extend32(get_unaligned_be24(
>> +					priv->rx_buffer + chan->address), 23);
>> +		ret = IIO_VAL_INT;
>> +		break;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		ret = ads1298_get_scale(priv, chan->channel, val, val2);
>> +		break;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		ret = ads1298_get_samp_freq(priv, val);
>> +		break;
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
>> +		if (!ret) {
>> +			ret = IIO_VAL_INT;
>> +			*val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
>> +		}
> Takes a line or 2 more but better to keep consistent style on error handling
> 		if (ret)
> 			return ret;
> 		*val = 16 << (*val & ADS1298_MASK_CONFIG1_DR); /* Note fewer () */
> 		return IIO_VAL_INT;
>
>
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
> Always return early if there is nothing to do.  Makes review easier as no
> need to sanity check what happens for each path if it's already returned.
>
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int ads1298_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan, int val,
>> +			     int val2, long mask)
>> +{
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		ret = ads1298_set_samp_freq(priv, val);
> return ads1298_set_samp_freq()
>
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
> return -EINVAL;
>
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
> ..
>> +static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>> +			      unsigned int writeval, unsigned int *readval)
>> +{
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +
>> +	if (!readval)
>> +		return regmap_write(priv->regmap, reg, writeval);
>> +
>> +	return regmap_read(priv->regmap, reg, readval);
>> +}
>> +
>> +static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
>> +				    const unsigned long *scan_mask)
>> +{
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	/* Power down channels that aren't in use */
>> +	for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
>> +		regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
> Always check regmap accesses.  There's a horrible bus under this ;)
>
>> +				   ADS1298_MASK_CH_PD,
>> +				   test_bit(i, scan_mask) ?
>> +							0 : ADS1298_MASK_CH_PD);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct iio_info ads1298_info = {
>> +	.read_raw = &ads1298_read_raw,
>> +	.write_raw = &ads1298_write_raw,
>> +	.update_scan_mode = &ads1298_update_scan_mode,
>> +	.debugfs_reg_access = &ads1298_reg_access,
>> +};
>> +
>> +static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
>> +{
>> +	unsigned long flags;
>> +
>> +	/* Notify we're no longer waiting for the SPI transfer to complete */
>> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
>> +	priv->rdata_xfer_busy = false;
>> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
>> +}
>> +
>> +static void ads1298_rdata_complete(void *context)
>> +{
>> +	struct iio_dev *indio_dev = context;
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	int scan_index;
>> +	u8 *bounce = priv->bounce_buffer;
>> +
>> +	if (!iio_buffer_enabled(indio_dev)) {
> This can race I think with a transition into buffered mode.
> You should be able to use iio_device_claim_direct_mode
> here().  That will fail if you are in buffered mode but if it succeeds
> it will hold us in non buffered mode until it is released.
>   
>> +		/* 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;
>> +
>> +		/* Transfer 24-bit value into 32-bit array */
>> +		memcpy(bounce + 1, data, 3);
>> +		bounce += 4;
>> +	}
>> +
>> +	/* rx_buffer can be overwritten from this point on */
>> +	ads1298_rdata_unmark_busy(priv);
>> +
>> +	iio_push_to_buffers(indio_dev, priv->bounce_buffer);
>> +}
>> +
>> +static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
>> +{
>> +	struct iio_dev *indio_dev = dev_id;
>> +	struct ads1298_private *priv = iio_priv(indio_dev);
>> +	unsigned long flags;
>> +	bool wasbusy;
>> +
>> +	/* Prevent that we submit a message that was still in progress */
>> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
>> +	wasbusy = priv->rdata_xfer_busy;
>> +	priv->rdata_xfer_busy = true;
>> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
>> +
>> +	if (!wasbusy)
>> +		spi_async(priv->spi, &priv->rdata_msg);
>> +
>> +	return IRQ_HANDLED;
> Can you not use the IRQF_ONESHOT and an IRQ thread to deal with what
> you are using wasbusy for here?  I'm not keen on a reply saying we've
> handled the interrupt when we really haven't finished with it.
>
> This looks like a classic data ready trigger as well. Why handle
> it without the trigger infrastructure?
>
>> +};
>
>
>> +
>> +static const struct iio_buffer_setup_ops ads1298_setup_ops = {
>> +	.postenable = &ads1298_buffer_postenable,
>> +	.predisable = &ads1298_buffer_predisable,
>> +};
> ...
>
>
>> +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;
>> +
>> +	if ((val & 0x18) == 0x10) {
> FIELD_GET() and appropriate mask defines.
>
>> +		dev_info(dev, "Found %s, %d channels\n",
>> +			 ads1298_family_name(val),
>> +			 4 + 2 * (val & 0x07));
>> +	} else {
>> +		dev_err(dev, "Unknown ID: 0x%x\n", val);
> In order to support fall back DT compatibles etc, we generally only
> print a message on a missmatched ID.  It might be for a new fully
> compatible device with a DT binding deliberately specifying that
> the older device is fine for matching against.
> This is something we are slowly fixing up in older drives after
> some long discussions with the DT maintainers a few years back.
>
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Enable internal test signal, double amplitude, double frequency */
>> +	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);
>> +
> Always check return values unless you expect them to fail (in which case document
> that).
>
>> +	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
>> +	if (!priv->reg_vref) {
>> +		/* Enable internal reference */
>> +		val |= ADS1298_MASK_CONFIG3_PWR_REFBUF;
> #FIELD_PREP() and masks preferred for consistency with multi bit fields.
>
>> +		/* 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;
>> +	}
>> +	regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
>> +
>> +	for (val = 0; val < ADS1298_MAX_CHANNELS; val++) {
>> +		/* Set mux to analog input, PGA = 6x */
> Use appropriate mask defines and FIELD_PREP() so that 0x00 doesn't
> need a comment here as the define makes it clear what is going on.
>
>> +		regmap_write(priv->regmap, ADS1298_REG_CHnSET(val), 0x00);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1298_probe(struct spi_device *spi)
>> +{
>> +	struct ads1298_private *priv;
>> +	struct iio_dev *indio_dev;
>> +	struct device *dev = &spi->dev;
>> +	struct gpio_desc *reset_gpio;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
>> +	if (indio_dev == NULL)
>> +		return -ENOMEM;
>> +
>> +	priv = iio_priv(indio_dev);
>> +
>> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(reset_gpio))
>> +		return dev_err_probe(dev, ret, "Cannot get reset GPIO\n");
>> +
>> +	priv->reg_avdd = devm_regulator_get(dev, "avdd");
>> +	if (IS_ERR(priv->reg_avdd))
>> +		return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
>> +				     "Failed to get avdd regulator\n");
>> +
>> +	/* VREF can be supplied externally. Otherwise use internal reference */
>> +	priv->reg_vref = devm_regulator_get_optional(dev, "vref");
>> +	if (IS_ERR(priv->reg_vref)) {
>> +		if (PTR_ERR(priv->reg_vref) == -ENODEV)
>> +			priv->reg_vref = NULL;
>> +		else
>> +			return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
>> +					     "Failed to get vref regulator\n");
>> +	} else {
>> +		ret = regulator_enable(priv->reg_vref);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
>> +					       priv->reg_vref);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	priv->clk = devm_clk_get_enabled(dev, "clk");
>> +	if (IS_ERR(priv->clk))
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
>> +				     "Failed to get clk\n");
>> +
>> +	ret = regulator_enable(priv->reg_avdd);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to enable avdd regulator\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
>> +				       priv->reg_avdd);
>> +	if (ret)
>> +		return ret;
> I'd group the getting of this regulator with it's enabling.  That will bring it
> inline with how you handle vref.
>
>
>> +
>> +	priv->spi = spi;
>> +	init_completion(&priv->completion);
>> +	spin_lock_init(&priv->irq_busy_lock);
>> +	priv->regmap = devm_regmap_init(dev, NULL, priv,
>> +					&ads1298_regmap_config);
>> +	if (IS_ERR(priv->regmap))
>> +		return PTR_ERR(priv->regmap);
>> +
>> +	priv->tx_buffer[0] = ADS1298_CMD_RDATA;
>> +	priv->rdata_xfer.tx_buf = priv->tx_buffer;
>> +	priv->rdata_xfer.rx_buf = priv->rx_buffer;
>> +	priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE;
>> +	/* Must keep CS low for 4 clocks */
>> +	priv->rdata_xfer.delay.value = 2;
>> +	priv->rdata_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
>> +	spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1);
>> +	priv->rdata_msg.complete = &ads1298_rdata_complete;
>> +	priv->rdata_msg.context = indio_dev;
>> +
>> +	/* Avoid giving all the same name, iio scope doesn't handle that well */
> That's intentional - name is the type of chip, not a unique name.
> If you need a way to identify the device then use the path of the parent
> which will associate this with the underlying bus device and hence provide
> a stable reference.  We deliberately don't attempt to provide a different way
> to identify the device. readlink is your friend in userspace.
> Some drivers got through review with naming coming from elsewhere but this
> is the part number of the device.  With hindsight the ABI should have reflected
> that to avoid confusion but we are stuck with 'name'.
>
>> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
>> +					 spi_get_device_id(spi)->name,
>> +					 dev_name(dev));
>> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> +	indio_dev->channels = ads1298_channels;
>> +	indio_dev->num_channels = ADS1298_MAX_CHANNELS;
>> +	indio_dev->info = &ads1298_info;
>> +
>> +	if (reset_gpio) {
>> +		udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */
>> +		gpiod_set_value(reset_gpio, 0);
>> +	} else {
>> +		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret, "RESET failed\n");
>> +	}
>> +	/* Wait 18 clock cycles for reset command to complete */
>> +	udelay(9);
>> +
>> +	ret = ads1298_init(priv);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Init failed\n");
>> +
>> +	ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,
>> +			       IRQF_TRIGGER_FALLING, indio_dev->name,
>> +			       indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
>> +					  &ads1298_setup_ops);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +static const struct spi_device_id ads1298_id[] = {
>> +	{ "ads1298",  },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, ads1298_id);
>> +
>> +static const struct of_device_id ads1298_of_table[] = {
>> +	{ .compatible = "ti,ads1298" },
>> +	{ },
> No point in the trailing comma.
>> +};
>> +MODULE_DEVICE_TABLE(of, ads1298_of_table);
> Thanks,
>
> Jonathan
>
>

On Wed, Jan 31, 2024 at 05:10:08PM +0100, Mike Looijmans wrote:
> On 13-12-2023 15:55, Andy Shevchenko wrote:
> > On Wed, Dec 13, 2023 at 10:47:22AM +0100, Mike Looijmans wrote:

First of all, please remove unneeded context, don't make me waste time on doing
that for you!

...

> > 		*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
> > 				     23);
> 
> Doesn't fit, first line is 83 characters by my count...

Is it a problem?

On Thu, 1 Feb 2024 13:38:16 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Jan 31, 2024 at 05:10:08PM +0100, Mike Looijmans wrote:
> > On 13-12-2023 15:55, Andy Shevchenko wrote:  
> > > On Wed, Dec 13, 2023 at 10:47:22AM +0100, Mike Looijmans wrote:  
> 
> First of all, please remove unneeded context, don't make me waste time on doing
> that for you!
> 
> ...
> 
> > > 		*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
> > > 				     23);  
> > 
> > Doesn't fit, first line is 83 characters by my count...  
> 
> Is it a problem?

To add a bit more info here. For IIO at least (and most of the rest of the kernel)
it's fine to go over 80 chars if there is a significant cost in readability to break
the line.  Here that is the case + you are only going 3 chars over so it's fine
to do so.

Jonathan

> 
>

On Wed, 31 Jan 2024 17:24:18 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> On 14-12-2023 12:06, Jonathan Cameron wrote:
> > On Wed, 13 Dec 2023 10:47:22 +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,
> >
> > Various small things inline - some of which probably overlap with Andy's
> > comments.  
> 
> 
> Working on it - Assume "yes to all" as my response on all suggestions, 
> except for the IRQ handling...
> 
> >
> > The larger one is I don't follow why this does manual protection against
> > concurrent handling of the result of an IRQ.  Much easier to just use
> > an IRQ threaded handler and IRQF_ONESHOT to mask the irq entirely until
> > after the initial handling is done.  If that doesn't work for some reason
> > then more comments needed to explain why!  
> 
> Yeah, definitely needs comments, and a bit of code too...
> 
> This chip doesn't have a buffer, but it does "latch" the sample data 
> when it receives a RDATA command (hence I use that in favor of RDATAC, 
> which does not latch and might return corrupted data).
> 
> To keep the latency as low as possible, I want to immediately start the 
> SPI transfer when the DRDY interrupt arrives. My experiments have shown 
> a big difference there, when using a ONESHOT trigger, it failed to meet 
> the deadline at even the lowest frequencies.

That's interesting to hear.  I wonder why - the overhead should be small.

Potentially it kicks the interrupt thread which then might kick an spi
controller thread rather than kicking the spi controller directly.
I think that depends on the SPI controller driver implementation choices
assuming things aren't contended - the defaults in the spi core
will take a fast path in current context if no contention - so it'll
happen in the interrupt thread.  So in general case there should be
very little difference in the timing needed to kick off the actual
SPI transfer starting via the two paths. The bit you mention below
on overlapping is a gain.  One trick we do to try and grab that back
if it's occasional is to poll the device in the trigger reenable
callback (only works if there is a register to say there is new data).

Maybe something odd going on in the interrupt controller driver...
It might not be useable for the higher frequencies, but should work
at reasonably low ones.



> 
> The next SPI transfer can start immediately after the data has been 
> copied into the intermediate buffer for IIO, the handler need not wait 
> for IIO to process the data.

I'd be surprised if the processing time was significant (compared to the SPI
transfer times) - so this 'feels' like a bit of over the top micro
optimization.

> 
> When the DRDY interrupt arrives, and there's an SPI transfer still in 
> progress, it's not too late yet, the "latch" may save us still. Once the 
> SPI transfer completes and the data is in the intermediate buffer, we 
> should immediately start a new SPI transaction to latch and fetch the 
> next set. (This code is still missing in the current version)
> 
> Only when DRDY arrives a second time during an SPI transaction we missed 
> the deadline and sample data was lost.

If you are running anywhere near speeds where this happens, things aren't
going to be reliable anyway. Whether that is a problem depends on your usecase.

So in conclusion, I'm surprised you are seeing such a difference in the
rates you can capture at.  Might be worth trying to grab some debug info
to understand this a bit more given you are proposing an unusual structure
with maintenance costs etc.

> 
> No further comments below from me, just kept the history for reference
Don't bother :)  Better to crop as much as possible. We have archives for
history.

Jonathan
>