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

[Krzysztof Kozlowski <krzk@kernel.org>]

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