| Message ID | 20191203134816.5319-2-jagan@amarulasolutions.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm/sun4i: Allwinner A64 MIPI-DSI support | expand |
On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > The MIPI DSI controller in Allwinner A64 is similar to A33. > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > to have separate compatible for A64 on the same driver. > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > is not required for A64. > > On that note > - A64 require minimum of 1 clock like the bus clock > - A33 require minimum of 2 clocks like both bus, mod clocks > > So, update dt-bindings so-that it can document both A33, > A64 bindings requirements. > > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes for v12: > - Use 'enum' instead of oneOf+const > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > index dafc0980c4fa..b91446475f35 100644 > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > @@ -15,7 +15,9 @@ properties: > "#size-cells": true > > compatible: > - const: allwinner,sun6i-a31-mipi-dsi > + enum: > + - allwinner,sun6i-a31-mipi-dsi > + - allwinner,sun50i-a64-mipi-dsi > > reg: > maxItems: 1 > @@ -24,6 +26,8 @@ properties: > maxItems: 1 > > clocks: > + minItems: 1 > + maxItems: 2 > items: > - description: Bus Clock > - description: Module Clock > @@ -63,13 +67,25 @@ required: > - reg > - interrupts > - clocks > - - clock-names > - phys > - phy-names > - resets > - vcc-dsi-supply > - port > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: allwinner,sun6i-a31-mipi-dsi > + then: > + properties: > + clocks: > + minItems: 2 > + required: > + - clock-names > + Your else condition should check that the number of clocks items is 1 on the A64 Maxime
On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard <mripard@kernel.org> wrote: > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > to have separate compatible for A64 on the same driver. > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > is not required for A64. > > > > On that note > > - A64 require minimum of 1 clock like the bus clock > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > So, update dt-bindings so-that it can document both A33, > > A64 bindings requirements. > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > Changes for v12: > > - Use 'enum' instead of oneOf+const > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > index dafc0980c4fa..b91446475f35 100644 > > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > @@ -15,7 +15,9 @@ properties: > > "#size-cells": true > > > > compatible: > > - const: allwinner,sun6i-a31-mipi-dsi > > + enum: > > + - allwinner,sun6i-a31-mipi-dsi > > + - allwinner,sun50i-a64-mipi-dsi > > > > reg: > > maxItems: 1 > > @@ -24,6 +26,8 @@ properties: > > maxItems: 1 > > > > clocks: > > + minItems: 1 > > + maxItems: 2 > > items: > > - description: Bus Clock > > - description: Module Clock > > @@ -63,13 +67,25 @@ required: > > - reg > > - interrupts > > - clocks > > - - clock-names > > - phys > > - phy-names > > - resets > > - vcc-dsi-supply > > - port > > > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: allwinner,sun6i-a31-mipi-dsi > > + then: > > + properties: > > + clocks: > > + minItems: 2 > > + required: > > + - clock-names > > + > > Your else condition should check that the number of clocks items is 1 > on the A64 But the minItems mentioned as 1 in clocks, which is unchanged number by default. doesn't it sufficient?
On Mon, Dec 16, 2019 at 04:37:20PM +0530, Jagan Teki wrote: > On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > > to have separate compatible for A64 on the same driver. > > > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > > is not required for A64. > > > > > > On that note > > > - A64 require minimum of 1 clock like the bus clock > > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > > > So, update dt-bindings so-that it can document both A33, > > > A64 bindings requirements. > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > --- > > > Changes for v12: > > > - Use 'enum' instead of oneOf+const > > > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++-- > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > index dafc0980c4fa..b91446475f35 100644 > > > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > @@ -15,7 +15,9 @@ properties: > > > "#size-cells": true > > > > > > compatible: > > > - const: allwinner,sun6i-a31-mipi-dsi > > > + enum: > > > + - allwinner,sun6i-a31-mipi-dsi > > > + - allwinner,sun50i-a64-mipi-dsi > > > > > > reg: > > > maxItems: 1 > > > @@ -24,6 +26,8 @@ properties: > > > maxItems: 1 > > > > > > clocks: > > > + minItems: 1 > > > + maxItems: 2 > > > items: > > > - description: Bus Clock > > > - description: Module Clock > > > @@ -63,13 +67,25 @@ required: > > > - reg > > > - interrupts > > > - clocks > > > - - clock-names > > > - phys > > > - phy-names > > > - resets > > > - vcc-dsi-supply > > > - port > > > > > > +allOf: > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: allwinner,sun6i-a31-mipi-dsi > > > + then: > > > + properties: > > > + clocks: > > > + minItems: 2 > > > + required: > > > + - clock-names > > > + > > > > Your else condition should check that the number of clocks items is 1 > > on the A64 > > But the minItems mentioned as 1 in clocks, which is unchanged number > by default. doesn't it sufficient? In the main schema, it's said that the clocks property can have one or two elements (to cover the A31 case that has one, and the A64 case that has 2). This is fine. Later on, you enforce that the A64 has two elements, and this is fine too. However, you never check that on the A31 you only have one clock, and you could very well have two and no one would notice. Maxime
On Mon, Dec 16, 2019 at 4:50 PM Maxime Ripard <mripard@kernel.org> wrote: > > On Mon, Dec 16, 2019 at 04:37:20PM +0530, Jagan Teki wrote: > > On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > > > to have separate compatible for A64 on the same driver. > > > > > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > > > is not required for A64. > > > > > > > > On that note > > > > - A64 require minimum of 1 clock like the bus clock > > > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > > > > > So, update dt-bindings so-that it can document both A33, > > > > A64 bindings requirements. > > > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > --- > > > > Changes for v12: > > > > - Use 'enum' instead of oneOf+const > > > > > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++-- > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > index dafc0980c4fa..b91446475f35 100644 > > > > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > @@ -15,7 +15,9 @@ properties: > > > > "#size-cells": true > > > > > > > > compatible: > > > > - const: allwinner,sun6i-a31-mipi-dsi > > > > + enum: > > > > + - allwinner,sun6i-a31-mipi-dsi > > > > + - allwinner,sun50i-a64-mipi-dsi > > > > > > > > reg: > > > > maxItems: 1 > > > > @@ -24,6 +26,8 @@ properties: > > > > maxItems: 1 > > > > > > > > clocks: > > > > + minItems: 1 > > > > + maxItems: 2 > > > > items: > > > > - description: Bus Clock > > > > - description: Module Clock > > > > @@ -63,13 +67,25 @@ required: > > > > - reg > > > > - interrupts > > > > - clocks > > > > - - clock-names > > > > - phys > > > > - phy-names > > > > - resets > > > > - vcc-dsi-supply > > > > - port > > > > > > > > +allOf: > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: allwinner,sun6i-a31-mipi-dsi > > > > + then: > > > > + properties: > > > > + clocks: > > > > + minItems: 2 > > > > + required: > > > > + - clock-names > > > > + > > > > > > Your else condition should check that the number of clocks items is 1 > > > on the A64 > > > > But the minItems mentioned as 1 in clocks, which is unchanged number > > by default. doesn't it sufficient? > > In the main schema, it's said that the clocks property can have one or > two elements (to cover the A31 case that has one, and the A64 case > that has 2). > > This is fine. > > Later on, you enforce that the A64 has two elements, and this is fine > too. Actually A31 case has 2 and A64 case has 1. > > However, you never check that on the A31 you only have one clock, and > you could very well have two and no one would notice. I did check A31 case for 2 but not in A64. this is what you mean? so adding A64 check like below would fine? allOf: - if: properties: compatible: contains: const: allwinner,sun6i-a31-mipi-dsi then: properties: clocks: minItems: 2 required: - clock-names - if: properties: compatible: contains: const: allwinner,sun50i-a64-mipi-dsi then: properties: clocks: minItems: 1
On Mon, Dec 16, 2019 at 10:29:08PM +0530, Jagan Teki wrote: > On Mon, Dec 16, 2019 at 4:50 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > On Mon, Dec 16, 2019 at 04:37:20PM +0530, Jagan Teki wrote: > > > On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > > > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > > > > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > > > > to have separate compatible for A64 on the same driver. > > > > > > > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > > > > is not required for A64. > > > > > > > > > > On that note > > > > > - A64 require minimum of 1 clock like the bus clock > > > > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > > > > > > > So, update dt-bindings so-that it can document both A33, > > > > > A64 bindings requirements. > > > > > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > --- > > > > > Changes for v12: > > > > > - Use 'enum' instead of oneOf+const > > > > > > > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++-- > > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > index dafc0980c4fa..b91446475f35 100644 > > > > > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > @@ -15,7 +15,9 @@ properties: > > > > > "#size-cells": true > > > > > > > > > > compatible: > > > > > - const: allwinner,sun6i-a31-mipi-dsi > > > > > + enum: > > > > > + - allwinner,sun6i-a31-mipi-dsi > > > > > + - allwinner,sun50i-a64-mipi-dsi > > > > > > > > > > reg: > > > > > maxItems: 1 > > > > > @@ -24,6 +26,8 @@ properties: > > > > > maxItems: 1 > > > > > > > > > > clocks: > > > > > + minItems: 1 > > > > > + maxItems: 2 > > > > > items: > > > > > - description: Bus Clock > > > > > - description: Module Clock > > > > > @@ -63,13 +67,25 @@ required: > > > > > - reg > > > > > - interrupts > > > > > - clocks > > > > > - - clock-names > > > > > - phys > > > > > - phy-names > > > > > - resets > > > > > - vcc-dsi-supply > > > > > - port > > > > > > > > > > +allOf: > > > > > + - if: > > > > > + properties: > > > > > + compatible: > > > > > + contains: > > > > > + const: allwinner,sun6i-a31-mipi-dsi > > > > > + then: > > > > > + properties: > > > > > + clocks: > > > > > + minItems: 2 > > > > > + required: > > > > > + - clock-names > > > > > + > > > > > > > > Your else condition should check that the number of clocks items is 1 > > > > on the A64 > > > > > > But the minItems mentioned as 1 in clocks, which is unchanged number > > > by default. doesn't it sufficient? > > > > In the main schema, it's said that the clocks property can have one or > > two elements (to cover the A31 case that has one, and the A64 case > > that has 2). > > > > This is fine. > > > > Later on, you enforce that the A64 has two elements, and this is fine > > too. > > Actually A31 case has 2 and A64 case has 1. > > > > > However, you never check that on the A31 you only have one clock, and > > you could very well have two and no one would notice. > > I did check A31 case for 2 but not in A64. this is what you mean? so > adding A64 check like below would fine? > > allOf: > - if: > properties: > compatible: > contains: > const: allwinner,sun6i-a31-mipi-dsi You need a new line here > then: This is the wrong level of indentation, it needs to be at the same level than if > properties: > clocks: > minItems: 2 Newline > required: > - clock-names Newline > - if: > properties: > compatible: > contains: > const: allwinner,sun50i-a64-mipi-dsi > then: > properties: > clocks: > minItems: 1 (and same thing here). But yeah, otherwise that's what I meant Maxime
On Tue, Dec 17, 2019 at 12:04 AM Maxime Ripard <mripard@kernel.org> wrote: > > On Mon, Dec 16, 2019 at 10:29:08PM +0530, Jagan Teki wrote: > > On Mon, Dec 16, 2019 at 4:50 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > On Mon, Dec 16, 2019 at 04:37:20PM +0530, Jagan Teki wrote: > > > > On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > > > > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > > > > > > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > > > > > to have separate compatible for A64 on the same driver. > > > > > > > > > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > > > > > is not required for A64. > > > > > > > > > > > > On that note > > > > > > - A64 require minimum of 1 clock like the bus clock > > > > > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > > > > > > > > > So, update dt-bindings so-that it can document both A33, > > > > > > A64 bindings requirements. > > > > > > > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > > --- > > > > > > Changes for v12: > > > > > > - Use 'enum' instead of oneOf+const > > > > > > > > > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++-- > > > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > > index dafc0980c4fa..b91446475f35 100644 > > > > > > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > > @@ -15,7 +15,9 @@ properties: > > > > > > "#size-cells": true > > > > > > > > > > > > compatible: > > > > > > - const: allwinner,sun6i-a31-mipi-dsi > > > > > > + enum: > > > > > > + - allwinner,sun6i-a31-mipi-dsi > > > > > > + - allwinner,sun50i-a64-mipi-dsi > > > > > > > > > > > > reg: > > > > > > maxItems: 1 > > > > > > @@ -24,6 +26,8 @@ properties: > > > > > > maxItems: 1 > > > > > > > > > > > > clocks: > > > > > > + minItems: 1 > > > > > > + maxItems: 2 > > > > > > items: > > > > > > - description: Bus Clock > > > > > > - description: Module Clock > > > > > > @@ -63,13 +67,25 @@ required: > > > > > > - reg > > > > > > - interrupts > > > > > > - clocks > > > > > > - - clock-names > > > > > > - phys > > > > > > - phy-names > > > > > > - resets > > > > > > - vcc-dsi-supply > > > > > > - port > > > > > > > > > > > > +allOf: > > > > > > + - if: > > > > > > + properties: > > > > > > + compatible: > > > > > > + contains: > > > > > > + const: allwinner,sun6i-a31-mipi-dsi > > > > > > + then: > > > > > > + properties: > > > > > > + clocks: > > > > > > + minItems: 2 > > > > > > + required: > > > > > > + - clock-names > > > > > > + > > > > > > > > > > Your else condition should check that the number of clocks items is 1 > > > > > on the A64 > > > > > > > > But the minItems mentioned as 1 in clocks, which is unchanged number > > > > by default. doesn't it sufficient? > > > > > > In the main schema, it's said that the clocks property can have one or > > > two elements (to cover the A31 case that has one, and the A64 case > > > that has 2). > > > > > > This is fine. > > > > > > Later on, you enforce that the A64 has two elements, and this is fine > > > too. > > > > Actually A31 case has 2 and A64 case has 1. > > > > > > > > However, you never check that on the A31 you only have one clock, and > > > you could very well have two and no one would notice. > > > > I did check A31 case for 2 but not in A64. this is what you mean? so > > adding A64 check like below would fine? > > > > allOf: > > - if: > > properties: > > compatible: > > contains: > > const: allwinner,sun6i-a31-mipi-dsi > > You need a new line here I couldn't see new line before then: in example schema - https://elixir.bootlin.com/linux/v5.5-rc2/source/Documentation/devicetree/bindings/example-schema.yaml#L208 May be a new changes for this ML or so? > > > then: > > This is the wrong level of indentation, it needs to be at the same level than if True, I have seen it in example schema. > > > properties: > > clocks: > > minItems: 2 > > Newline > > > required: > > - clock-names > > Newline > > > - if: > > properties: > > compatible: > > contains: > > const: allwinner,sun50i-a64-mipi-dsi > > then: > > properties: > > clocks: > > minItems: 1 > > (and same thing here). > > But yeah, otherwise that's what I meant Thanks for the detailed review.
diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml index dafc0980c4fa..b91446475f35 100644 --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml @@ -15,7 +15,9 @@ properties: "#size-cells": true compatible: - const: allwinner,sun6i-a31-mipi-dsi + enum: + - allwinner,sun6i-a31-mipi-dsi + - allwinner,sun50i-a64-mipi-dsi reg: maxItems: 1 @@ -24,6 +26,8 @@ properties: maxItems: 1 clocks: + minItems: 1 + maxItems: 2 items: - description: Bus Clock - description: Module Clock @@ -63,13 +67,25 @@ required: - reg - interrupts - clocks - - clock-names - phys - phy-names - resets - vcc-dsi-supply - port +allOf: + - if: + properties: + compatible: + contains: + const: allwinner,sun6i-a31-mipi-dsi + then: + properties: + clocks: + minItems: 2 + required: + - clock-names + additionalProperties: false examples: