diff mbox series

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

Message ID 20250313-dev-adp5589-fw-v1-5-20e80d4bd4ea@analog.com (mailing list archive)
State New
Headers show
Series mfd: adp5585: support keymap events and drop legacy Input driver | expand

Commit Message

Nuno Sá via B4 Relay March 13, 2025, 2:19 p.m. UTC 
From: Nuno Sá <nuno.sa@analog.com>

The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder,
programmable logic, reset generator, and PWM generator.

We can't really have adp5589 devices fallback to adp5585 (which have
less pins) because there are some significant differences in the register
map.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../devicetree/bindings/mfd/adi,adp5585.yaml       | 48 +++++++++++++++-------
 .../devicetree/bindings/trivial-devices.yaml       |  2 -
 2 files changed, 34 insertions(+), 16 deletions(-)

Comments

Krzysztof Kozlowski March 14, 2025, 8:49 a.m. UTC | #1 
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.




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

Best regards,
Krzysztof
Nuno Sá March 14, 2025, 9:38 a.m. UTC | #2 
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:
were already in the binding):

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


Thx!
- Nuno Sá
Krzysztof Kozlowski March 17, 2025, 7:41 a.m. UTC | #3 
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.

Best regards,
Krzysztof
Nuno Sá March 17, 2025, 9:30 a.m. UTC | #4 
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á
Krzysztof Kozlowski March 17, 2025, 10:41 a.m. UTC | #5 
On Mon, Mar 17, 2025 at 09:30:37AM +0000, Nuno Sá wrote:
> 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

Yes, you are right. The diff context confused me, so I thought these are
different.

It's fine.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
index e30e22f964f78519b2ec207e9415e4897db5c702..87256a37b5f4b6a019f581b164c276d8805d2e52 100644
--- a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
+++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
@@ -15,14 +15,21 @@  description:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - adi,adp5585-00  # Default
-          - adi,adp5585-01  # 11 GPIOs
-          - adi,adp5585-02  # No pull-up resistors by default on special pins
-          - adi,adp5585-03  # Alternate I2C address
-          - adi,adp5585-04  # Pull-down resistors on all pins by default
-      - const: adi,adp5585
+    oneOf:
+      - items:
+          - enum:
+              - adi,adp5585-00  # Default
+              - adi,adp5585-01  # 11 GPIOs
+              - adi,adp5585-02  # No pull-up resistors by default on special pins
+              - adi,adp5585-03  # Alternate I2C address
+              - adi,adp5585-04  # Pull-down resistors on all pins by default
+          - const: adi,adp5585
+      - items:
+          - enum:
+              - adi,adp5589-00  # Default
+              - adi,adp5589-01  # R4 defaulted to RESET1 output
+              - adi,adp5589-02  # Pull-down resistors by default on special pins
+          - const: adi,adp5589
 
   reg:
     maxItems: 1
@@ -63,13 +70,26 @@  allOf:
       properties:
         gpio-reserved-ranges: false
     else:
-      properties:
-        gpio-reserved-ranges:
-          maxItems: 1
-          items:
+      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:
-              - const: 5
-              - const: 1
+              items:
+                - const: 5
+                - const: 1
+      else:
+        properties:
+          gpio-reserved-ranges: false
 
 additionalProperties: false
 
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index fadbd3c041c8c39faedfe62874d4eba25a0bf30e..bdf0afb9301181b3f95fb7125302304834bdee94 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -39,8 +39,6 @@  properties:
           - ad,adm9240
             # AD5110 - Nonvolatile Digital Potentiometer
           - adi,ad5110
-            # Analog Devices ADP5589 Keypad Decoder and I/O Expansion
-          - adi,adp5589
             # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
           - adi,lt7182s
             # AMS iAQ-Core VOC Sensor