Re: [PATCH 05/18] dt-bindings: mfd: adp5585: document adp5589 I/O expander

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Mon, 2025-03-17 at 08:41 +0100, Krzysztof Kozlowski wrote:
> On 14/03/2025 10:38, Nuno Sá wrote:
> > On Fri, 2025-03-14 at 09:49 +0100, Krzysztof Kozlowski wrote:
> > > On Thu, Mar 13, 2025 at 02:19:22PM +0000, Nuno Sá wrote:
> > > >    reg:
> > > >      maxItems: 1
> > > > @@ -63,13 +70,26 @@ allOf:
> > > >        properties:
> > > >          gpio-reserved-ranges: false
> > > >      else:
> > > > -      properties:
> > > > -        gpio-reserved-ranges:
> > > > -          maxItems: 1
> > > > -          items:
> > > > +      if:
> > > 
> > > Do not nest if:then:else:if:then, it leads to code impossible to read.
> > > Just provide if-then cases for each of your variant.
> > > 
> > 
> > Alright...
> > 
> > > 
> > > 
> > > 
> > > > +        properties:
> > > > +          compatible:
> > > > +            contains:
> > > > +              enum:
> > > > +                - adi,adp5585-00
> > > > +                - adi,adp5585-02
> > > > +                - adi,adp5585-03
> > > > +                - adi,adp5585-04
> > > > +      then:
> > > > +        properties:
> > > > +          gpio-reserved-ranges:
> > > > +            maxItems: 1
> > > 
> > > one tem?
> > > 
> > > >              items:
> > > > -              - const: 5
> > > > -              - const: 1
> > > 
> > > But here two...
> > > 
> > > > +              items:
> > > > +                - const: 5
> > > > +                - const: 1
> > > 
> > > and this is confusing. I don't get what you want to express.
> > > 
> > 
> > I just kept it as before (maybe I messed up in some other way but the 2
> > items:
> 
> No, your code is very different.
> 
> > were already in the binding):
> 
> I see only one GPIO range.
> 
> > 
> > https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml#L70
> > 
> > If this is not needed I can simplifying during this patch. Is this
> > sufficient?
> > 
> > ...
> > 
> >         gpio-reserved-ranges:
> >           maxItems: 1
> >           items:
> >             - const: 5
> >             - const: 1
> 
> Again, different code and not correct as you have now two ranges. Open
> original code - it is clear not the same. So two tries - your patch and
> code above - are different but I don't get why you claim your code is
> identical.
> 

I'm really missing something obvious but how is it different? The original code
was:

- if:
      properties:
        compatible:
          contains:
            const: adi,adp5585-01
    then:
      properties:
        gpio-reserved-ranges: false
    else:
      properties:
        gpio-reserved-ranges:
          maxItems: 1
          items:
            items:
              - const: 5
              - const: 1

Now, I have:

- if:
      properties:
        compatible:
          contains:
            const: adi,adp5585-01
    then:
      properties:
        gpio-reserved-ranges: false
    else:
      if:
        properties:
          compatible:
            contains:
              enum:
                - adi,adp5585-00
                - adi,adp5585-02
                - adi,adp5585-03
                - adi,adp5585-04
      then:
        properties:
          gpio-reserved-ranges:
            maxItems: 1
            items:
              items:
                - const: 5
                - const: 1
      else:
        properties:
          gpio-reserved-ranges: false


So the only thing I have is the nested 'if else' (that you already complained
about) and I need to have 'gpio-reserved-ranges: false' for the adp5589 family
of devices since there is no such constrain. But this part:

properties:
  gpio-reserved-ranges:
    maxItems: 1
    items:
      items:
        - const: 5
        - const: 1

is very much what we have today.

So, sorry if I'm missing something obvious but I'm really not getting what you
mean...

- Nuno Sá