[PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR

Marcelo Schmitt posted 4 patches 4 days, 13 hours ago
[PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
Posted by Marcelo Schmitt 4 days, 13 hours ago
Extend the AD4000 series device tree documentation to also describe
PulSAR devices.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
No changes from v2 -> v3.

 .../bindings/iio/adc/adi,ad4000.yaml          | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
index e413a9d8d2a2..4dbb3d2876f9 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -19,6 +19,20 @@ description: |
     https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf
 
 $ref: /schemas/spi/spi-peripheral-props.yaml#
 
@@ -63,6 +77,37 @@ properties:
 
       - const: adi,adaq4003
 
+      - const: adi,ad7946
+      - items:
+          - enum:
+              - adi,ad7942
+          - const: adi,ad7946
+
+      - const: adi,ad7983
+      - items:
+          - enum:
+              - adi,ad7980
+              - adi,ad7988-5
+              - adi,ad7686
+              - adi,ad7685
+              - adi,ad7988-1
+          - const: adi,ad7983
+
+      - const: adi,ad7688
+      - items:
+          - enum:
+              - adi,ad7693
+              - adi,ad7687
+          - const: adi,ad7688
+
+      - const: adi,ad7984
+      - items:
+          - enum:
+              - adi,ad7982
+              - adi,ad7690
+              - adi,ad7691
+          - const: adi,ad7984
+
   reg:
     maxItems: 1
 
@@ -133,6 +178,32 @@ required:
   - ref-supply
 
 allOf:
+  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7685
+              - adi,ad7686
+              - adi,ad7687
+              - adi,ad7688
+              - adi,ad7690
+              - adi,ad7691
+              - adi,ad7693
+              - adi,ad7942
+              - adi,ad7946
+              - adi,ad7980
+              - adi,ad7982
+              - adi,ad7983
+              - adi,ad7984
+              - adi,ad7988-1
+              - adi,ad7988-5
+    then:
+      properties:
+        adi,sdi-pin:
+          enum: [ high, low, cs ]
+          default: cs
   # The configuration register can only be accessed if SDI is connected to MOSI
   - if:
       required:
-- 
2.45.2
Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
Posted by Krzysztof Kozlowski 3 days, 18 hours ago
On Tue, Nov 19, 2024 at 09:53:40AM -0300, Marcelo Schmitt wrote:
> Extend the AD4000 series device tree documentation to also describe
> PulSAR devices.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> No changes from v2 -> v3.
> 
>  .../bindings/iio/adc/adi,ad4000.yaml          | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> index e413a9d8d2a2..4dbb3d2876f9 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -19,6 +19,20 @@ description: |
>      https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf
>  
>  $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
> @@ -63,6 +77,37 @@ properties:
>  
>        - const: adi,adaq4003
>  
> +      - const: adi,ad7946

All such cases are just one enum. That's the preferred syntax.


> +      - items:
> +          - enum:
> +              - adi,ad7942
> +          - const: adi,ad7946
> +
> +      - const: adi,ad7983
> +      - items:
> +          - enum:
> +              - adi,ad7980
> +              - adi,ad7988-5
> +              - adi,ad7686
> +              - adi,ad7685

Keep alphabetical order.

> +              - adi,ad7988-1
> +          - const: adi,ad7983
> +
> +      - const: adi,ad7688
> +      - items:
> +          - enum:
> +              - adi,ad7693
> +              - adi,ad7687
> +          - const: adi,ad7688
> +
> +      - const: adi,ad7984
> +      - items:
> +          - enum:
> +              - adi,ad7982
> +              - adi,ad7690
> +              - adi,ad7691
> +          - const: adi,ad7984
> +
>    reg:
>      maxItems: 1
>  
> @@ -133,6 +178,32 @@ required:
>    - ref-supply
>  
>  allOf:
> +  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad7685

Why do you need this? It's fallback is already here.

> +              - adi,ad7686
> +              - adi,ad7687
> +              - adi,ad7688
> +              - adi,ad7690
> +              - adi,ad7691
> +              - adi,ad7693
> +              - adi,ad7942
> +              - adi,ad7946
> +              - adi,ad7980
> +              - adi,ad7982
> +              - adi,ad7983
> +              - adi,ad7984
> +              - adi,ad7988-1
> +              - adi,ad7988-5

Best regards,
Krzysztof
Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
Posted by Marcelo Schmitt 1 day, 10 hours ago
On 11/20, Krzysztof Kozlowski wrote:
> On Tue, Nov 19, 2024 at 09:53:40AM -0300, Marcelo Schmitt wrote:
> > Extend the AD4000 series device tree documentation to also describe
> > PulSAR devices.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> > No changes from v2 -> v3.
> > 
> >  .../bindings/iio/adc/adi,ad4000.yaml          | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > index e413a9d8d2a2..4dbb3d2876f9 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > @@ -19,6 +19,20 @@ description: |
> >      https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> >      https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> >      https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
> > +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf
> >  
> >  $ref: /schemas/spi/spi-peripheral-props.yaml#
> >  
> > @@ -63,6 +77,37 @@ properties:
> >  
> >        - const: adi,adaq4003
> >  
> > +      - const: adi,ad7946
> 
> All such cases are just one enum. That's the preferred syntax.
> 
Ack

> 
> > +      - items:
> > +          - enum:
> > +              - adi,ad7942
> > +          - const: adi,ad7946
> > +
> > +      - const: adi,ad7983
> > +      - items:
> > +          - enum:
> > +              - adi,ad7980
> > +              - adi,ad7988-5
> > +              - adi,ad7686
> > +              - adi,ad7685
> 
> Keep alphabetical order.

Do the fallbacks declared here have any impact on the match try order or on how
the compatible list should be ordered?
The only significant difference between each group of devices is the sample rate.
A faster device can read at slower sample rates so if somebody knows to have
a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
could have a compatible like
      compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
                   "adi,ad7988-1", "adi,ad7983";

to try from fastest to slowest device.
The dt doc would indicate that order in the fallback list?
      - items:
          - enum:
              - adi,ad7980    # Fastest 16-bit pseudo-differential ADC
              - adi,ad7988-5  # 2nd fastest 16-bit pseudo-differential ADC
              - adi,ad7686    # 3rd fastest 16-bit pseudo-differential ADC
              - adi,ad7685    # 4th fastest 16-bit pseudo-differential ADC
              - adi,ad7988-1  # 5th fastest 16-bit pseudo-differential ADC
          - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC

https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
has a nice table with the different devices and sample rates.

writing-bindings.rst says "DO use fallback compatibles when devices are the same
as or a subset of prior implementations."
But, how can we use fallbacks properly?
From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
inferring only one fallback should be provided per group of devices.

> 
> > +              - adi,ad7988-1
> > +          - const: adi,ad7983
> > +
> > +      - const: adi,ad7688
> > +      - items:
> > +          - enum:
> > +              - adi,ad7693
> > +              - adi,ad7687
> > +          - const: adi,ad7688
> > +
> > +      - const: adi,ad7984
> > +      - items:
> > +          - enum:
> > +              - adi,ad7982
> > +              - adi,ad7690
> > +              - adi,ad7691
> > +          - const: adi,ad7984
> > +
> >    reg:
> >      maxItems: 1
> >  
> > @@ -133,6 +178,32 @@ required:
> >    - ref-supply
> >  
> >  allOf:
> > +  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,ad7685
> 
> Why do you need this? It's fallback is already here.

So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
and adi,sdi-pin = "sdi";

zynq-coraz7s-ad7687.dtb: adc@0: adi,sdi-pin:0: 'sdi' is not one of ['high', 'low', 'cs']
> 
> > +              - adi,ad7686
> > +              - adi,ad7687
> > +              - adi,ad7688
> > +              - adi,ad7690
> > +              - adi,ad7691
> > +              - adi,ad7693
> > +              - adi,ad7942
> > +              - adi,ad7946
> > +              - adi,ad7980
> > +              - adi,ad7982
> > +              - adi,ad7983
> > +              - adi,ad7984
> > +              - adi,ad7988-1
> > +              - adi,ad7988-5
> 
> Best regards,
> Krzysztof
>
Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
Posted by Krzysztof Kozlowski 1 day, 8 hours ago
On 22/11/2024 16:33, Marcelo Schmitt wrote:
>>
>>> +      - items:
>>> +          - enum:
>>> +              - adi,ad7942
>>> +          - const: adi,ad7946
>>> +
>>> +      - const: adi,ad7983
>>> +      - items:
>>> +          - enum:
>>> +              - adi,ad7980
>>> +              - adi,ad7988-5
>>> +              - adi,ad7686
>>> +              - adi,ad7685
>>
>> Keep alphabetical order.
> 
> Do the fallbacks declared here have any impact on the match try order or on how
> the compatible list should be ordered?

I don't understand, we do not talk about fallbacks. I also do not
understand at all how this relates to my comment.

> The only significant difference between each group of devices is the sample rate.
> A faster device can read at slower sample rates so if somebody knows to have
> a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
> could have a compatible like
>       compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
>                    "adi,ad7988-1", "adi,ad7983";

Can't you autodetect this?

> 
> to try from fastest to slowest device.
> The dt doc would indicate that order in the fallback list?
>       - items:
>           - enum:
>               - adi,ad7980    # Fastest 16-bit pseudo-differential ADC
>               - adi,ad7988-5  # 2nd fastest 16-bit pseudo-differential ADC
>               - adi,ad7686    # 3rd fastest 16-bit pseudo-differential ADC
>               - adi,ad7685    # 4th fastest 16-bit pseudo-differential ADC
>               - adi,ad7988-1  # 5th fastest 16-bit pseudo-differential ADC
>           - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC

Again, only one fallback here, not sure what are you asking about. BTW,
DT spec explains compatibles...

> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> has a nice table with the different devices and sample rates.
> 
> writing-bindings.rst says "DO use fallback compatibles when devices are the same
> as or a subset of prior implementations."
> But, how can we use fallbacks properly?

How DT spec and tutorials like elinux ask... What is exactly the problem
or question?

> From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm

How LVDS bridge is related to this one here?

> inferring only one fallback should be provided per group of devices.
> 
>>
>>> +              - adi,ad7988-1
>>> +          - const: adi,ad7983
>>> +
>>> +      - const: adi,ad7688
>>> +      - items:
>>> +          - enum:
>>> +              - adi,ad7693
>>> +              - adi,ad7687
>>> +          - const: adi,ad7688
>>> +
>>> +      - const: adi,ad7984
>>> +      - items:
>>> +          - enum:
>>> +              - adi,ad7982
>>> +              - adi,ad7690
>>> +              - adi,ad7691
>>> +          - const: adi,ad7984
>>> +
>>>    reg:
>>>      maxItems: 1
>>>  
>>> @@ -133,6 +178,32 @@ required:
>>>    - ref-supply
>>>  
>>>  allOf:
>>> +  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - adi,ad7685
>>
>> Why do you need this? It's fallback is already here.
> 
> So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
> and adi,sdi-pin = "sdi";


I mean this compatible, not if clause.




Best regards,
Krzysztof
Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
Posted by Marcelo Schmitt 1 day, 5 hours ago
On 11/22, Krzysztof Kozlowski wrote:
> On 22/11/2024 16:33, Marcelo Schmitt wrote:
> >>
> >>> +      - items:
> >>> +          - enum:
> >>> +              - adi,ad7942
> >>> +          - const: adi,ad7946
> >>> +
> >>> +      - const: adi,ad7983
> >>> +      - items:
> >>> +          - enum:
> >>> +              - adi,ad7980
> >>> +              - adi,ad7988-5
> >>> +              - adi,ad7686
> >>> +              - adi,ad7685
> >>
> >> Keep alphabetical order.
> > 
> > Do the fallbacks declared here have any impact on the match try order or on how
> > the compatible list should be ordered?
> 
> I don't understand, we do not talk about fallbacks. I also do not
> understand at all how this relates to my comment.

I was wondering if the arrangement in which compatible strings appear in dt doc
could be used to suggest the sequence to add them to the compatible property of a
device node in a dts. Apparently, the arrangement of compatible strings in dt doc
has nothing to do with how they can appear in a dts file. Will sort them in
alphabetical order.

> 
> > The only significant difference between each group of devices is the sample rate.
> > A faster device can read at slower sample rates so if somebody knows to have
> > a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
> > could have a compatible like
> >       compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
> >                    "adi,ad7988-1", "adi,ad7983";
> 
> Can't you autodetect this?

There is no way of detecting the maximum sample rate other than the compatible
string or, maybe, running a data capture.

> 
> > 
> > to try from fastest to slowest device.
> > The dt doc would indicate that order in the fallback list?
> >       - items:
> >           - enum:
> >               - adi,ad7980    # Fastest 16-bit pseudo-differential ADC
> >               - adi,ad7988-5  # 2nd fastest 16-bit pseudo-differential ADC
> >               - adi,ad7686    # 3rd fastest 16-bit pseudo-differential ADC
> >               - adi,ad7685    # 4th fastest 16-bit pseudo-differential ADC
> >               - adi,ad7988-1  # 5th fastest 16-bit pseudo-differential ADC
> >           - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC
> 
[...]
> > 
> > writing-bindings.rst says "DO use fallback compatibles when devices are the same
> > as or a subset of prior implementations."
> > But, how can we use fallbacks properly?
> 
> How DT spec and tutorials like elinux ask... What is exactly the problem
> or question?

Never mind. Do the bellow follow the preferred syntax?

      - items:
          - enum:
              - adi,ad7980
              - adi,ad7685
              - adi,ad7686
              - adi,ad7988-1
              - adi,ad7988-5
          - const: adi,ad7983

      - items:
          - enum:
              - adi,ad7688
              - adi,ad7693
          - const: adi,ad7687

      - items:
          - enum:
              - adi,ad7982
              - adi,ad7984
              - adi,ad7690
          - const: adi,ad7691

      - enum:
          - adi,ad7942
          - adi,ad7946
          - adi,ad7984

> 
> > From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
> 
> How LVDS bridge is related to this one here?

Aside from having compatible fallbacks, not related.

> 
> > inferring only one fallback should be provided per group of devices.
> > 
> >>
> >>> +              - adi,ad7988-1
> >>> +          - const: adi,ad7983
> >>> +
> >>> +      - const: adi,ad7688
> >>> +      - items:
> >>> +          - enum:
> >>> +              - adi,ad7693
> >>> +              - adi,ad7687
> >>> +          - const: adi,ad7688
> >>> +
> >>> +      - const: adi,ad7984
> >>> +      - items:
> >>> +          - enum:
> >>> +              - adi,ad7982
> >>> +              - adi,ad7690
> >>> +              - adi,ad7691
> >>> +          - const: adi,ad7984
> >>> +
> >>>    reg:
> >>>      maxItems: 1
> >>>  
> >>> @@ -133,6 +178,32 @@ required:
> >>>    - ref-supply
> >>>  
> >>>  allOf:
> >>> +  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - adi,ad7685
> >>
> >> Why do you need this? It's fallback is already here.
> > 
> > So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
> > and adi,sdi-pin = "sdi";
> 
> 
> I mean this compatible, not if clause.

dtbs_check don't show an error message if the allOf list only has the fallback
compatible for adi,ad7685 and a device node has both 
compatible = "adi,ad7685" and adi,sdi-pin = "sdi".

The new set of devices that will be supported by this binding don't have a
configuration register like the previous ones did. Because the PulSAR devices
don't have a config reg, they don't support all features of AD4000-like devices
and thus fewer IIO ABI interfaces are provided to user space. Though, AD4000
devices also can be wired in a way that no reg access is possible, in which
case they provide the same IIO interfaces that PulSAR devices do. The difference
is on what is connected to the peripheral SDI pin. When AD4000 SDI is connected
to SPI controller MOSI line, more interfaces are provided because the config
reg can be accessed to set additional features. But that is not an option for
PulSAR devices. Even if controller MOSI is connected to a PulSAR device, we
cannot provide the additional interfaces because every attempt to use them would
fail (the device has no register to configure). No datasheets mentions
connecting a PulSAR device SDI pin to a SPI MOSI line. All datasheets show
PulSAR SDI pin connected either to VIO (high), GND (low), or controller CS.

IMHO, it would be nice to have dtbs_check warn about invalid SDI pin
configuration otherwise it may only be noticed on driver probe.
Anyway, I'm also fine keeping only the fallback compatibles in the allOf list
if that makes dt maintainers happy.

> 
> 
> 
> 
> Best regards,
> Krzysztof
Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
Posted by Krzysztof Kozlowski 10 hours ago
On 22/11/2024 22:15, Marcelo Schmitt wrote:
> On 11/22, Krzysztof Kozlowski wrote:
>> On 22/11/2024 16:33, Marcelo Schmitt wrote:
>>>>
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - adi,ad7942
>>>>> +          - const: adi,ad7946
>>>>> +
>>>>> +      - const: adi,ad7983
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - adi,ad7980
>>>>> +              - adi,ad7988-5
>>>>> +              - adi,ad7686
>>>>> +              - adi,ad7685
>>>>
>>>> Keep alphabetical order.
>>>
>>> Do the fallbacks declared here have any impact on the match try order or on how
>>> the compatible list should be ordered?
>>
>> I don't understand, we do not talk about fallbacks. I also do not
>> understand at all how this relates to my comment.
> 
> I was wondering if the arrangement in which compatible strings appear in dt doc
> could be used to suggest the sequence to add them to the compatible property of a
> device node in a dts. Apparently, the arrangement of compatible strings in dt doc
> has nothing to do with how they can appear in a dts file. Will sort them in
> alphabetical order.

We talk here about enum. Enum enumerates, so obviously they cannot
appear one after another.

> 
>>
>>> The only significant difference between each group of devices is the sample rate.
>>> A faster device can read at slower sample rates so if somebody knows to have
>>> a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
>>> could have a compatible like
>>>       compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
>>>                    "adi,ad7988-1", "adi,ad7983";
>>
>> Can't you autodetect this?
> 
> There is no way of detecting the maximum sample rate other than the compatible
> string or, maybe, running a data capture.


Devices do not have version/revision/model register?

> 
>>
>>>
>>> to try from fastest to slowest device.
>>> The dt doc would indicate that order in the fallback list?
>>>       - items:
>>>           - enum:
>>>               - adi,ad7980    # Fastest 16-bit pseudo-differential ADC
>>>               - adi,ad7988-5  # 2nd fastest 16-bit pseudo-differential ADC
>>>               - adi,ad7686    # 3rd fastest 16-bit pseudo-differential ADC
>>>               - adi,ad7685    # 4th fastest 16-bit pseudo-differential ADC
>>>               - adi,ad7988-1  # 5th fastest 16-bit pseudo-differential ADC
>>>           - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC
>>
> [...]
>>>
>>> writing-bindings.rst says "DO use fallback compatibles when devices are the same
>>> as or a subset of prior implementations."
>>> But, how can we use fallbacks properly?
>>
>> How DT spec and tutorials like elinux ask... What is exactly the problem
>> or question?
> 
> Never mind. Do the bellow follow the preferred syntax?
> 
>       - items:
>           - enum:
>               - adi,ad7980
>               - adi,ad7685
>               - adi,ad7686
>               - adi,ad7988-1
>               - adi,ad7988-5
>           - const: adi,ad7983
> 
>       - items:
>           - enum:
>               - adi,ad7688
>               - adi,ad7693
>           - const: adi,ad7687
> 
>       - items:
>           - enum:
>               - adi,ad7982
>               - adi,ad7984
>               - adi,ad7690
>           - const: adi,ad7691
> 
>       - enum:
>           - adi,ad7942
>           - adi,ad7946
>           - adi,ad7984

Yes

> 
>>
>>> From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
>>
>> How LVDS bridge is related to this one here?
> 
> Aside from having compatible fallbacks, not related.
> 
>>
>>> inferring only one fallback should be provided per group of devices.
>>>
>>>>
>>>>> +              - adi,ad7988-1
>>>>> +          - const: adi,ad7983
>>>>> +
>>>>> +      - const: adi,ad7688
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - adi,ad7693
>>>>> +              - adi,ad7687
>>>>> +          - const: adi,ad7688
>>>>> +
>>>>> +      - const: adi,ad7984
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - adi,ad7982
>>>>> +              - adi,ad7690
>>>>> +              - adi,ad7691
>>>>> +          - const: adi,ad7984
>>>>> +
>>>>>    reg:
>>>>>      maxItems: 1
>>>>>  
>>>>> @@ -133,6 +178,32 @@ required:
>>>>>    - ref-supply
>>>>>  
>>>>>  allOf:
>>>>> +  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            enum:
>>>>> +              - adi,ad7685
>>>>
>>>> Why do you need this? It's fallback is already here.
>>>
>>> So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
>>> and adi,sdi-pin = "sdi";
>>
>>
>> I mean this compatible, not if clause.
> 
> dtbs_check don't show an error message if the allOf list only has the fallback
> compatible for adi,ad7685 and a device node has both 
> compatible = "adi,ad7685" and adi,sdi-pin = "sdi".

It must and your compatibles should not affect it. I don't know which
code you are testing, but I even tested the correct approach and it
correctly shows error.


> 
> The new set of devices that will be supported by this binding don't have a
> configuration register like the previous ones did. Because the PulSAR devices
> don't have a config reg, they don't support all features of AD4000-like devices
> and thus fewer IIO ABI interfaces are provided to user space. Though, AD4000
> devices also can be wired in a way that no reg access is possible, in which
> case they provide the same IIO interfaces that PulSAR devices do. The difference
> is on what is connected to the peripheral SDI pin. When AD4000 SDI is connected
> to SPI controller MOSI line, more interfaces are provided because the config
> reg can be accessed to set additional features. But that is not an option for
> PulSAR devices. Even if controller MOSI is connected to a PulSAR device, we
> cannot provide the additional interfaces because every attempt to use them would
> fail (the device has no register to configure). No datasheets mentions
> connecting a PulSAR device SDI pin to a SPI MOSI line. All datasheets show
> PulSAR SDI pin connected either to VIO (high), GND (low), or controller CS.
> 
> IMHO, it would be nice to have dtbs_check warn about invalid SDI pin
> configuration otherwise it may only be noticed on driver probe.
> Anyway, I'm also fine keeping only the fallback compatibles in the allOf list
> if that makes dt maintainers happy.

Only fallbacks go there,


Best regards,
Krzysztof