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

[Mike Looijmans <mike.looijmans@topic.nl>]

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
>>
>