| Message ID | 20240202105901.925875-1-mike.looijmans@topic.nl |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2,1/2] dt-bindings: iio: adc: ti-ads1298: Add bindings | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Fri, Feb 02, 2024 at 11:59:00AM +0100, Mike Looijmans wrote: > Bindings 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. > > The device has so many options for connecting stuff, at this > point the bindings aren't nearly complete but partial bindings > are better than no bindings at all. I'm inclined to agree, particularly given your comments on v1. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > > --- > > Changes in v2: > Remove "clk" name > Add datasheet and "incomplete" note > > .../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..bf5a43a81d59 > --- /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 > + > +description: | > + Datasheet at: https://www.ti.com/product/ADS1298 > + Bindings for this chip aren't complete. > + > +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, > + which is 2.4V when analog supply is below 4.4V, 4V otherwise. > + > + clocks: > + description: Optional 2.048 MHz external source clock on CLK pin > + maxItems: 1 > + > + 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 = <®_iso_5v_a>; > + 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 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, ®_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);
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 04-02-2024 16:54, Jonathan Cameron wrote: > 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. I think one of the key elements was the IRQF_ONESHOT usage. The DRDY signal on this chip isn't a "level" signal as most chips have, it will de-assert at any rising edge of the SPI clock, without regarding chip-select. There's no other indication of "data ready", so the only way is to keep the interrupt active on edge detect. Keeping things in hard IRQ handlers reduces the number of context switches, and the amount of work done is minimal. A worker thread would wake at every DRDY signal, and after the corresponding SPI transaction. This doesn't account for much overhead, but the interrupt rate is double the sampling frequency. Most importantly, the device doesn't have to compete with other threads in the system. If I have time and hardware available, I try to get some timing info with an oscilloscope... Assume "yes" to all other suggestions... >> + 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'd be happy to leave the IRQ direction to firmware (indeed, inverters happen...), but afaik that wasn't possible with interrupts. I'll dig into the docs to see if that has changed recently. > 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); (PS - sorry for sending HTML, consider me appropriately punisched for that)
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..bf5a43a81d59 --- /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 + +description: | + Datasheet at: https://www.ti.com/product/ADS1298 + Bindings for this chip aren't complete. + +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, + which is 2.4V when analog supply is below 4.4V, 4V otherwise. + + clocks: + description: Optional 2.048 MHz external source clock on CLK pin + maxItems: 1 + + 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 = <®_iso_5v_a>; + clocks = <&clk_ads1298>; + interrupt-parent = <&gpio0>; + interrupts = <78 IRQ_TYPE_EDGE_FALLING>; + spi-max-frequency = <20000000>; + spi-cpha; + }; + }; +...
Bindings 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. The device has so many options for connecting stuff, at this point the bindings aren't nearly complete but partial bindings are better than no bindings at all. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- Changes in v2: Remove "clk" name Add datasheet and "incomplete" note .../bindings/iio/adc/ti,ads1298.yaml | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml