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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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@9elements.com>
>> ---
>>  .../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@ideasonboard.com>
>>  
>>  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...

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


Best regards,
Krzysztof