On 09/10/2022 14:03, Serge Semin wrote: > 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... It's just a convention, although quite logical because "ifs" can grow significantly, so putting it before properties is outside of context. Reader does not know yet to what this if applies. > >> >>> >>>> + 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. Yes, this will work, but defining properties inside "if" is usually not readable. Best regards, Krzysztof
