Re: [PATCH 1/3] dt-bindings: iio: adc: Add GPADC for Allwinner A523

[Andre Przywara]

Hi Michal,

thanks for adding this!

On 5/10/26 14:57, Michal Piekos wrote:
> Add support for the GPADC for the Allwinner A523. It differs from the
> D1/T113s/R329/T507 by having two clocks.
>
> Signed-off-by: Michal Piekos <michal.piekos@xxxxxxxxxxxxx>
> ---
>   .../iio/adc/allwinner,sun20i-d1-gpadc.yaml         | 37 +++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml b/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> index da605a051b94..89da96cd705f 100644
> --- a/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> @@ -17,6 +17,7 @@ properties:
>         - items:
>             - enum:
>                 - allwinner,sun50i-h616-gpadc
> +              - allwinner,sun55i-a523-gpadc
>             - const: allwinner,sun20i-d1-gpadc

As Jernej already mentioned, the A523 GPADC is not fully compatible, 
since it adds another clock. The question to ask is: Can a driver only 
knowing about the fallback device handle this new device? For which the 
answer here is: No, it misses a clock.
So add just a single entry for the A523 (plus adding it to the driver).

So looking at this I wonder if we should add some property to describe 
the number of supported channels, since they are slightly different 
between the SoCs:
- The D1 manual mentions 2 channels.
- The T113s manual (same die as the D1?) describes 1 channel only.
- The T507 manual (same die as the H616) reports 4 channels.
- The A733 has 6 channels.
- The A133 has 1 channel, but it's channel 1, not 0.

So all of this is somewhat covered as channels are described as child 
nodes, and have a reg property. Ideally non-existing channels just 
wouldn't be listed, but I don't know if we want to rely on that.

So I am wondering if we should introduce a limit, or rather a mask (to 
cover the A133 oddity)?
Either a DT property (channel-mask, as a single sell representing the 
bit mask), or derived in the driver from the compatible string.
The former would avoid introducing different compatible strings just 
because of that, though I think this type of property is somewhat 
discouraged?

Any thoughts?

>   
>     "#io-channel-cells":
> @@ -29,7 +30,12 @@ properties:
>       const: 0
>   
>     clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 2
>   
>     interrupts:
>       maxItems: 1
> @@ -40,6 +46,35 @@ properties:
>     resets:
>       maxItems: 1
>   
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          items:
> +            - const: allwinner,sun55i-a523-gpadc
> +            - const: allwinner,sun20i-d1-gpadc
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 2
> +          maxItems: 2
> +          items:
> +            - description: Bus clock
> +            - description: Module clock

I am not a YAML expert, but I think you can drop the min and max 
properties, if you just enumerate the cases. Same for the names.

Cheers,
Andre

> +        clock-names:
> +          minItems: 2
> +          maxItems: 2
> +          items:
> +            - const: bus
> +            - const: mod
> +      required:
> +        - clock-names
> +    else:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +        clock-names: false
> +
>   patternProperties:
>     "^channel@[0-9a-f]+$":
>       $ref: adc.yaml