Re: [v9 1/4] dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants

[Serge Semin]

On Sun, Oct 09, 2022 at 05:25:22PM +0200, Krzysztof Kozlowski wrote:
> On 08/10/2022 13:50, Serge Semin wrote:
> > On Fri, Oct 07, 2022 at 09:53:50AM +0200, Patrick Rudolph wrote:
> >> Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x
> >> chips. The functionality will be provided by the exisintg pca954x driver.
> >>
> >> While on it make the interrupts support conditionally as not all of the
> >> existing chips have interrupts.
> >>
> >> For chips that are powered off by default add an optional regulator
> >> called vdd-supply.
> >>
> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >> ---
> >>  .../bindings/i2c/i2c-mux-pca954x.yaml         | 39 ++++++++++++++++---
> >>  1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> >> index 9f1726d0356b..efad0a95806f 100644
> >> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> >> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> >> @@ -4,21 +4,25 @@
> >>  $id: http://devicetree.org/schemas/i2c/i2c-mux-pca954x.yaml#
> >>  $schema: http://devicetree.org/meta-schemas/core.yaml#
> >>  
> >> -title: NXP PCA954x I2C bus switch
> >> +title: NXP PCA954x I2C and compatible bus switches
> >>  
> >>  maintainers:
> >>    - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>  
> >>  description:
> >> -  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices.
> >> -
> > 
> >> -allOf:
> >> -  - $ref: /schemas/i2c/i2c-mux.yaml#
> > 
> > Why do you move the allOf statement to the bottom of the schema?
> 

> Because it goes with 'ifs' at the bottom of the schema...

Is there a requirement to move the allOf array to the bottom of the
schema if it contains the 'if' statement? If only there were some
kernel doc with all such implicit conventions...

> 
> > 
> >> +  The binding supports NXP PCA954x and PCA984x I2C mux/switch devices,
> >> +  and the Maxim MAX735x and MAX736x I2C mux/switch devices.
> > 
> > What about combining the sentence: "The binding supports NXP
> > PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ?
> > Currently it does look a bit bulky.
> 
> Drop "The binding supports". Instead describe the hardware.
> 
> > 
> >>  
> >>  properties:
> >>    compatible:
> >>      oneOf:
> >>        - enum:
> >> +          - maxim,max7356
> >> +          - maxim,max7357
> >> +          - maxim,max7358
> >> +          - maxim,max7367
> >> +          - maxim,max7368
> >> +          - maxim,max7369
> >>            - nxp,pca9540
> >>            - nxp,pca9542
> >>            - nxp,pca9543
> >> @@ -59,10 +63,33 @@ properties:
> >>      description: if present, overrides i2c-mux-idle-disconnect
> >>      $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state
> >>  
> >> +  vdd-supply:
> >> +    description: A voltage regulator supplying power to the chip.
> >> +
> >>  required:
> >>    - compatible
> >>    - reg
> >>  
> >> +allOf:
> >> +  - $ref: /schemas/i2c/i2c-mux.yaml#
> >> +  - if:
> >> +      not:
> >> +        properties:
> >> +          compatible:
> >> +            contains:
> >> +              enum:
> >> +                - maxim,max7367
> >> +                - maxim,max7369
> >> +                - nxp,pca9542
> >> +                - nxp,pca9543
> >> +                - nxp,pca9544
> >> +                - nxp,pca9545
> >> +    then:
> > 
> >> +      properties:
> >> +        interrupts: false
> >> +        "#interrupt-cells": false
> >> +        interrupt-controller: false
> > 
> > I'd suggest to add an opposite definition. Evaluate the properties for
> > the devices which expect them being evaluated instead of falsing their
> > existence for the devices which don't support the interrupts.
> 

> The properties rather should be defined in top-level than in "if", so I
> am not sure how would you want to achieve opposite way.

With one more implicit convention like "preferably define the
properties in the top-level than in if" of course I can't. Otherwise I
thought something like this would work:
+allOf:
+  - ...
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum: [...]
+    then:
+      properties:
+        interrupts: ...
+        "#interrupt-cells": ...
+        interrupt-controller: ...
...
-  interrupts:
-  "#interrupt-cells":
-  interrupt-controller: ...

With unevaluatedProperties set to false and evaluation performed for
the particular compatibles such schema shall work with the same
semantic.

-Sergey

> 
> 
> Best regards,
> Krzysztof
>