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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx
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@xxxxxxxxxx> 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@xxxxxxxx>

---

  .../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@xxxxxxxx>
+
+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@xxxxxxxx
W: www.topic.nl

Please consider the environment before printing this e-mail








[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux