diff mbox series

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

Message ID 20240214063736.88283-1-mike.looijmans@topic.nl
State Not Applicable, archived
Headers show
Series [v4,1/2] dt-bindings: iio: adc: ti-ads1298: Add bindings | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Mike Looijmans Feb. 14, 2024, 6:37 a.m. UTC 
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>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

---

(no changes since v2)

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

Comments

Jonathan Cameron Feb. 16, 2024, 1:38 p.m. UTC | #1 
On Wed, 14 Feb 2024 07:37:36 +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,

One final thing noticed on a (hopefully) last read through.

/sys/bus/iio:device0/name is going to read ads1298 whichever
chip is detected.

Would be more useful to users if it identified the actual
part given that is easily read from the ID register.

Jonathan

> ---
> 
> Changes in v4:
> Explain rdata_xfer_busy better and remove post-decrement
> Reset assert explanation and add cansleep
> Additional style changes
> 
> Changes in v3:
> Indentation fixups
> Remove unused headers
> Remove #define leftovers
> Use devm_get_clk_optional_enabled
> Use ilog2 instead of fls()-1
> Magic "23" replaced
> Explain the extra "0" in read/write register
> use guard() from cleanup.h
> use REGCACHE_MAPLE
> 
> Changes in v2:
> Remove accidental "default y" in Kconfig
> Indentation and similar cosmetic fixes
> Magic numbers into constants
> Short return paths in read_raw and write_raw
> DMA buffer alignment
> Bounce buffer is u32 instead of u8
> Avoid races using claim_direct_mode
> Check errors on all register accesses
> Immediate SPI restart to reduce underruns
> "name" is chip name, not unique

I missed this until having a final read through but it's not the chip name
in the driver currently - the name is always ads1298 despite there being a handy
ID register that tells us what we actually have.


> 
>  drivers/iio/adc/Kconfig      |  11 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ti-ads1298.c | 766 +++++++++++++++++++++++++++++++++++
>  3 files changed, 778 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads1298.c

> +
> +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_probe(struct spi_device *spi)
> +{

...

> +
> +	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;
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;

I was going to just tweak this whilst applying.  Using the spi device id often
ends up being fragile in the long term because of the split between the different
ID tables and the mess that happens if fallback compatibles are in use and
the spi IDs are missing (you will get a warning about this at runtime
but it'll carry on anyway).

Easier to just hard code the name for now and when you have multiple
devices supported, add this to a chip_info type structure.
Or we could make it support the more specific name given the detection above.
Is there a reason to not do that given a more accurate name is
easy to work out and may be useful to a user?

Jonathan
Mike Looijmans Feb. 16, 2024, 2:01 p.m. UTC | #2 
On 16-02-2024 14:38, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 07:37:36 +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,
>
> One final thing noticed on a (hopefully) last read through.
>
> /sys/bus/iio:device0/name is going to read ads1298 whichever
> chip is detected.
>
> Would be more useful to users if it identified the actual
> part given that is easily read from the ID register.

Makes sense. So It would say "ads1296" or "ads1298r" for example. I 
guess we prefer all lower-case here.


>
> Jonathan
>
>> ---
>>
>> Changes in v4:
>> Explain rdata_xfer_busy better and remove post-decrement
>> Reset assert explanation and add cansleep
>> Additional style changes
>>
>> Changes in v3:
>> Indentation fixups
>> Remove unused headers
>> Remove #define leftovers
>> Use devm_get_clk_optional_enabled
>> Use ilog2 instead of fls()-1
>> Magic "23" replaced
>> Explain the extra "0" in read/write register
>> use guard() from cleanup.h
>> use REGCACHE_MAPLE
>>
>> Changes in v2:
>> Remove accidental "default y" in Kconfig
>> Indentation and similar cosmetic fixes
>> Magic numbers into constants
>> Short return paths in read_raw and write_raw
>> DMA buffer alignment
>> Bounce buffer is u32 instead of u8
>> Avoid races using claim_direct_mode
>> Check errors on all register accesses
>> Immediate SPI restart to reduce underruns
>> "name" is chip name, not unique
> I missed this until having a final read through but it's not the chip name
> in the driver currently - the name is always ads1298 despite there being a handy
> ID register that tells us what we actually have.
>
>
>>   drivers/iio/adc/Kconfig      |  11 +
>>   drivers/iio/adc/Makefile     |   1 +
>>   drivers/iio/adc/ti-ads1298.c | 766 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 778 insertions(+)
>>   create mode 100644 drivers/iio/adc/ti-ads1298.c
>> +
>> +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_probe(struct spi_device *spi)
>> +{
> ...
>
>> +
>> +	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;
>> +
>> +	indio_dev->name = spi_get_device_id(spi)->name;
> I was going to just tweak this whilst applying.  Using the spi device id often
> ends up being fragile in the long term because of the split between the different
> ID tables and the mess that happens if fallback compatibles are in use and
> the spi IDs are missing (you will get a warning about this at runtime
> but it'll carry on anyway).
>
> Easier to just hard code the name for now and when you have multiple
> devices supported, add this to a chip_info type structure.
> Or we could make it support the more specific name given the detection above.
> Is there a reason to not do that given a more accurate name is
> easy to work out and may be useful to a user?
>
The only reason I tried to make the "name" unique is that IIO 
oscilloscope (ab)uses "name" as a unique identifier. That's something 
that ought to be fixed in IIO oscilloscope.

Giving the chip name would reveal the actual detected hardware, there's 
currently no other way for the user to find that out.
Mike Looijmans Feb. 16, 2024, 3:04 p.m. UTC | #3 
On 16-02-2024 14:38, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 07:37:36 +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,
>
> One final thing noticed on a (hopefully) last read through.
>
> /sys/bus/iio:device0/name is going to read ads1298 whichever
> chip is detected.
>
> Would be more useful to users if it identified the actual
> part given that is easily read from the ID register.

Implemented it like that, the "name" now reflects the chip's information.

While at it, I also made the probe fail on invalid ID (tested by probing 
an empty SPI bus)

And also pass the channel count into the "num_channels" member, and 
adapted the SPI transaction, which should support the devices with 4 or 
6 channels better.

I'll post a v5 soon, bit of testing first...
diff mbox series

Patch

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 = <&reg_iso_5v_a>;
+          clocks = <&clk_ads1298>;
+          interrupt-parent = <&gpio0>;
+          interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
+          spi-max-frequency = <20000000>;
+          spi-cpha;
+        };
+    };
+...