[2/8] dt-bindings: display: mxsfb: Add and fix compatible strings
diff mbox series

Message ID 20200813012910.13576-3-laurent.pinchart@ideasonboard.com
State New
Headers show
Series
  • drm: mxsfb: Allow overriding bus width
Related show

Commit Message

Laurent Pinchart Aug. 13, 2020, 1:29 a.m. UTC
Additional compatible strings have been added in DT source for the
i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
Most of the upstream DT sources use the fsl,imx28-lcdif compatible
string, which mostly predates the realization that the LCDIF in the
i.MX6 and newer SoCs have extra features compared to the i.MX28.

Update the bindings to add the missing compatible strings, with the
correct fallback values. This fails to validate some of the upstream DT
sources. Instead of adding the incorrect compatible fallback to the
binding, the sources should be updated separately.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Sam Ravnborg Aug. 16, 2020, 6:39 a.m. UTC | #1
On Thu, Aug 13, 2020 at 04:29:04AM +0300, Laurent Pinchart wrote:
> Additional compatible strings have been added in DT source for the
> i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
> Most of the upstream DT sources use the fsl,imx28-lcdif compatible
> string, which mostly predates the realization that the LCDIF in the
> i.MX6 and newer SoCs have extra features compared to the i.MX28.
> 
> Update the bindings to add the missing compatible strings, with the
> correct fallback values. This fails to validate some of the upstream DT
> sources. Instead of adding the incorrect compatible fallback to the
> binding, the sources should be updated separately.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml
> index 202381ec5bb7..ec6533b1d4a3 100644
> --- a/Documentation/devicetree/bindings/display/mxsfb.yaml
> +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
> @@ -15,11 +15,19 @@ description: |
>  
>  properties:
>    compatible:
> -    enum:
> -      - fsl,imx23-lcdif
> -      - fsl,imx28-lcdif
> -      - fsl,imx6sx-lcdif
> -      - fsl,imx8mq-lcdif
> +    oneOf:
> +      - enum:
> +          - fsl,imx23-lcdif
> +          - fsl,imx28-lcdif
> +          - fsl,imx6sx-lcdif
Indent correct.

> +      - items:
> +        - enum:
> +          - fsl,imx6sl-lcdif
> +          - fsl,imx6sll-lcdif
> +          - fsl,imx6ul-lcdif
> +          - fsl,imx7d-lcdif
> +          - fsl,imx8mq-lcdif
Indent shall be two more spaces like above.
(This is at least my best uderstanding)

> +        - const: fsl,imx6sx-lcdif

With the above the following compatibles are supported:

	"fsl,imx23-lcdif"
	"fsl,imx28-lcdif"
	"fsl,imx6sx-lcdif"
	"fsl,imx8mq-lcdif"

	"fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif"
	"fsl,imx6sll-lcdif", "fsl,imx6sx-lcdif"
	"fsl,imx6ul-lcdif", "fsl,imx6sx-lcdif"
	"fsl,imx7d-lcdif", "fsl,imx6sx-lcdif"
	"fsl,imx8mq-lcdif", "fsl,imx6sx-lcdif"

So the fallback value is the later "fsl,imx6sx-lcdif" which looks ok.

With indent fixed (or explained why I am wrong):
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>


>  
>    reg:
>      maxItems: 1
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart Aug. 17, 2020, 12:04 a.m. UTC | #2
Hi Sam,

On Sun, Aug 16, 2020 at 08:39:33AM +0200, Sam Ravnborg wrote:
> On Thu, Aug 13, 2020 at 04:29:04AM +0300, Laurent Pinchart wrote:
> > Additional compatible strings have been added in DT source for the
> > i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
> > Most of the upstream DT sources use the fsl,imx28-lcdif compatible
> > string, which mostly predates the realization that the LCDIF in the
> > i.MX6 and newer SoCs have extra features compared to the i.MX28.
> > 
> > Update the bindings to add the missing compatible strings, with the
> > correct fallback values. This fails to validate some of the upstream DT
> > sources. Instead of adding the incorrect compatible fallback to the
> > binding, the sources should be updated separately.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml
> > index 202381ec5bb7..ec6533b1d4a3 100644
> > --- a/Documentation/devicetree/bindings/display/mxsfb.yaml
> > +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
> > @@ -15,11 +15,19 @@ description: |
> >  
> >  properties:
> >    compatible:
> > -    enum:
> > -      - fsl,imx23-lcdif
> > -      - fsl,imx28-lcdif
> > -      - fsl,imx6sx-lcdif
> > -      - fsl,imx8mq-lcdif
> > +    oneOf:
> > +      - enum:
> > +          - fsl,imx23-lcdif
> > +          - fsl,imx28-lcdif
> > +          - fsl,imx6sx-lcdif
>
> Indent correct.
> 
> > +      - items:
> > +        - enum:
> > +          - fsl,imx6sl-lcdif
> > +          - fsl,imx6sll-lcdif
> > +          - fsl,imx6ul-lcdif
> > +          - fsl,imx7d-lcdif
> > +          - fsl,imx8mq-lcdif
>
> Indent shall be two more spaces like above.
> (This is at least my best uderstanding)

I think you're right. I wonder why dt_binding_check doesn't complain.
I'll fix it anyway.

> > +        - const: fsl,imx6sx-lcdif
> 
> With the above the following compatibles are supported:
> 
> 	"fsl,imx23-lcdif"
> 	"fsl,imx28-lcdif"
> 	"fsl,imx6sx-lcdif"
> 	"fsl,imx8mq-lcdif"
> 
> 	"fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif"
> 	"fsl,imx6sll-lcdif", "fsl,imx6sx-lcdif"
> 	"fsl,imx6ul-lcdif", "fsl,imx6sx-lcdif"
> 	"fsl,imx7d-lcdif", "fsl,imx6sx-lcdif"
> 	"fsl,imx8mq-lcdif", "fsl,imx6sx-lcdif"
> 
> So the fallback value is the later "fsl,imx6sx-lcdif" which looks ok.
> 
> With indent fixed (or explained why I am wrong):
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> >  
> >    reg:
> >      maxItems: 1
Stefan Agner Aug. 21, 2020, 2:53 p.m. UTC | #3
On 2020-08-13 03:29, Laurent Pinchart wrote:
> Additional compatible strings have been added in DT source for the
> i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
> Most of the upstream DT sources use the fsl,imx28-lcdif compatible
> string, which mostly predates the realization that the LCDIF in the
> i.MX6 and newer SoCs have extra features compared to the i.MX28.

Agreed, we should add fsl,imx6sx-lcdif for those devices.

But shouldn't we also keep fsl,imx28-lcdif? From what I can tell, the
devices can be driven by a driver only supporting fsl,imx28-lcdif
semantics, right?

--
Stefan

> 
> Update the bindings to add the missing compatible strings, with the
> correct fallback values. This fails to validate some of the upstream DT
> sources. Instead of adding the incorrect compatible fallback to the
> binding, the sources should be updated separately.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml
> b/Documentation/devicetree/bindings/display/mxsfb.yaml
> index 202381ec5bb7..ec6533b1d4a3 100644
> --- a/Documentation/devicetree/bindings/display/mxsfb.yaml
> +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
> @@ -15,11 +15,19 @@ description: |
>  
>  properties:
>    compatible:
> -    enum:
> -      - fsl,imx23-lcdif
> -      - fsl,imx28-lcdif
> -      - fsl,imx6sx-lcdif
> -      - fsl,imx8mq-lcdif
> +    oneOf:
> +      - enum:
> +          - fsl,imx23-lcdif
> +          - fsl,imx28-lcdif
> +          - fsl,imx6sx-lcdif
> +      - items:
> +        - enum:
> +          - fsl,imx6sl-lcdif
> +          - fsl,imx6sll-lcdif
> +          - fsl,imx6ul-lcdif
> +          - fsl,imx7d-lcdif
> +          - fsl,imx8mq-lcdif
> +        - const: fsl,imx6sx-lcdif
>  
>    reg:
>      maxItems: 1
Laurent Pinchart Aug. 23, 2020, 11:26 p.m. UTC | #4
Hi Stefan,

On Fri, Aug 21, 2020 at 04:53:56PM +0200, Stefan Agner wrote:
> On 2020-08-13 03:29, Laurent Pinchart wrote:
> > Additional compatible strings have been added in DT source for the
> > i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
> > Most of the upstream DT sources use the fsl,imx28-lcdif compatible
> > string, which mostly predates the realization that the LCDIF in the
> > i.MX6 and newer SoCs have extra features compared to the i.MX28.
> 
> Agreed, we should add fsl,imx6sx-lcdif for those devices.
> 
> But shouldn't we also keep fsl,imx28-lcdif? From what I can tell, the
> devices can be driven by a driver only supporting fsl,imx28-lcdif
> semantics, right?

Isn't it kept by this patch ?

> > Update the bindings to add the missing compatible strings, with the
> > correct fallback values. This fails to validate some of the upstream DT
> > sources. Instead of adding the incorrect compatible fallback to the
> > binding, the sources should be updated separately.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml
> > b/Documentation/devicetree/bindings/display/mxsfb.yaml
> > index 202381ec5bb7..ec6533b1d4a3 100644
> > --- a/Documentation/devicetree/bindings/display/mxsfb.yaml
> > +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
> > @@ -15,11 +15,19 @@ description: |
> >  
> >  properties:
> >    compatible:
> > -    enum:
> > -      - fsl,imx23-lcdif
> > -      - fsl,imx28-lcdif
> > -      - fsl,imx6sx-lcdif
> > -      - fsl,imx8mq-lcdif
> > +    oneOf:
> > +      - enum:
> > +          - fsl,imx23-lcdif
> > +          - fsl,imx28-lcdif

Here -----------------^

The binding now support any of "fsl,imx23-lcdif", "fsl,imx28-lcdif" or
"fsl,imx6sx-lcdif" alone, or "fsl,imx6sx-lcdif" with another
device-specific compatible string. The driver supports the three base
compatible strings, for V3, V4 and V6 of the IP core.

> > +          - fsl,imx6sx-lcdif
> > +      - items:
> > +        - enum:
> > +          - fsl,imx6sl-lcdif
> > +          - fsl,imx6sll-lcdif
> > +          - fsl,imx6ul-lcdif
> > +          - fsl,imx7d-lcdif
> > +          - fsl,imx8mq-lcdif
> > +        - const: fsl,imx6sx-lcdif
> >  
> >    reg:
> >      maxItems: 1
Stefan Agner Aug. 24, 2020, 2:19 p.m. UTC | #5
On 2020-08-24 01:26, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Fri, Aug 21, 2020 at 04:53:56PM +0200, Stefan Agner wrote:
>> On 2020-08-13 03:29, Laurent Pinchart wrote:
>> > Additional compatible strings have been added in DT source for the
>> > i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
>> > Most of the upstream DT sources use the fsl,imx28-lcdif compatible
>> > string, which mostly predates the realization that the LCDIF in the
>> > i.MX6 and newer SoCs have extra features compared to the i.MX28.
>>
>> Agreed, we should add fsl,imx6sx-lcdif for those devices.
>>
>> But shouldn't we also keep fsl,imx28-lcdif? From what I can tell, the
>> devices can be driven by a driver only supporting fsl,imx28-lcdif
>> semantics, right?
> 
> Isn't it kept by this patch ?
> 
>> > Update the bindings to add the missing compatible strings, with the
>> > correct fallback values. This fails to validate some of the upstream DT
>> > sources. Instead of adding the incorrect compatible fallback to the
>> > binding, the sources should be updated separately.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > ---
>> >  .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
>> >  1 file changed, 13 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml
>> > b/Documentation/devicetree/bindings/display/mxsfb.yaml
>> > index 202381ec5bb7..ec6533b1d4a3 100644
>> > --- a/Documentation/devicetree/bindings/display/mxsfb.yaml
>> > +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
>> > @@ -15,11 +15,19 @@ description: |
>> >
>> >  properties:
>> >    compatible:
>> > -    enum:
>> > -      - fsl,imx23-lcdif
>> > -      - fsl,imx28-lcdif
>> > -      - fsl,imx6sx-lcdif
>> > -      - fsl,imx8mq-lcdif
>> > +    oneOf:
>> > +      - enum:
>> > +          - fsl,imx23-lcdif
>> > +          - fsl,imx28-lcdif
> 
> Here -----------------^
> 
> The binding now support any of "fsl,imx23-lcdif", "fsl,imx28-lcdif" or
> "fsl,imx6sx-lcdif" alone, or "fsl,imx6sx-lcdif" with another
> device-specific compatible string. The driver supports the three base
> compatible strings, for V3, V4 and V6 of the IP core.

The binding yes, but I mean the device descriptions in the device tree.

Since the device can be driven by a older kernel which only knows about
the fsl,imx28-lcdif compatible string, we could keep that compatible.
From what I can tell, we can add both safely, e.g.

compatible = "fsl,imx6sx-lcdif", "fsl,imx28-lcdif"

From how I read the description this now replaces "fsl,imx28-lcdif" with
"fsl,imx6sx-lcdif" for the devices supporting the additional features,
e.g.:

--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -769,7 +769,7 @@ epdc: epdc@20f4000 {
             };
 
             lcdif: lcdif@20f8000 {
-                compatible = "fsl,imx6sl-lcdif", "fsl,imx28-lcdif";
+                compatible = "fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif";
                 reg = <0x020f8000 0x4000>;
                 interrupts = <0 39 IRQ_TYPE_LEVEL_HIGH>;
                 clocks = <&clks IMX6SL_CLK_LCDIF_PIX>,

--
Stefan

> 
>> > +          - fsl,imx6sx-lcdif
>> > +      - items:
>> > +        - enum:
>> > +          - fsl,imx6sl-lcdif
>> > +          - fsl,imx6sll-lcdif
>> > +          - fsl,imx6ul-lcdif
>> > +          - fsl,imx7d-lcdif
>> > +          - fsl,imx8mq-lcdif
>> > +        - const: fsl,imx6sx-lcdif
>> >
>> >    reg:
>> >      maxItems: 1
Rob Herring Aug. 24, 2020, 11:57 p.m. UTC | #6
On Mon, Aug 17, 2020 at 03:04:06AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> On Sun, Aug 16, 2020 at 08:39:33AM +0200, Sam Ravnborg wrote:
> > On Thu, Aug 13, 2020 at 04:29:04AM +0300, Laurent Pinchart wrote:
> > > Additional compatible strings have been added in DT source for the
> > > i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings.
> > > Most of the upstream DT sources use the fsl,imx28-lcdif compatible
> > > string, which mostly predates the realization that the LCDIF in the
> > > i.MX6 and newer SoCs have extra features compared to the i.MX28.
> > > 
> > > Update the bindings to add the missing compatible strings, with the
> > > correct fallback values. This fails to validate some of the upstream DT
> > > sources. Instead of adding the incorrect compatible fallback to the
> > > binding, the sources should be updated separately.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  .../devicetree/bindings/display/mxsfb.yaml     | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml
> > > index 202381ec5bb7..ec6533b1d4a3 100644
> > > --- a/Documentation/devicetree/bindings/display/mxsfb.yaml
> > > +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
> > > @@ -15,11 +15,19 @@ description: |
> > >  
> > >  properties:
> > >    compatible:
> > > -    enum:
> > > -      - fsl,imx23-lcdif
> > > -      - fsl,imx28-lcdif
> > > -      - fsl,imx6sx-lcdif
> > > -      - fsl,imx8mq-lcdif
> > > +    oneOf:
> > > +      - enum:
> > > +          - fsl,imx23-lcdif
> > > +          - fsl,imx28-lcdif
> > > +          - fsl,imx6sx-lcdif
> >
> > Indent correct.
> > 
> > > +      - items:
> > > +        - enum:
> > > +          - fsl,imx6sl-lcdif
> > > +          - fsl,imx6sll-lcdif
> > > +          - fsl,imx6ul-lcdif
> > > +          - fsl,imx7d-lcdif
> > > +          - fsl,imx8mq-lcdif
> >
> > Indent shall be two more spaces like above.
> > (This is at least my best uderstanding)
> 
> I think you're right. I wonder why dt_binding_check doesn't complain.
> I'll fix it anyway.

Because I haven't integrated yamllint yet. I do have a config file at 
least.

> 
> > > +        - const: fsl,imx6sx-lcdif
> > 
> > With the above the following compatibles are supported:
> > 
> > 	"fsl,imx23-lcdif"
> > 	"fsl,imx28-lcdif"
> > 	"fsl,imx6sx-lcdif"
> > 	"fsl,imx8mq-lcdif"
> > 
> > 	"fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif"
> > 	"fsl,imx6sll-lcdif", "fsl,imx6sx-lcdif"
> > 	"fsl,imx6ul-lcdif", "fsl,imx6sx-lcdif"
> > 	"fsl,imx7d-lcdif", "fsl,imx6sx-lcdif"
> > 	"fsl,imx8mq-lcdif", "fsl,imx6sx-lcdif"
> > 
> > So the fallback value is the later "fsl,imx6sx-lcdif" which looks ok.
> > 
> > With indent fixed (or explained why I am wrong):
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > 
> > >  
> > >    reg:
> > >      maxItems: 1
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml
index 202381ec5bb7..ec6533b1d4a3 100644
--- a/Documentation/devicetree/bindings/display/mxsfb.yaml
+++ b/Documentation/devicetree/bindings/display/mxsfb.yaml
@@ -15,11 +15,19 @@  description: |
 
 properties:
   compatible:
-    enum:
-      - fsl,imx23-lcdif
-      - fsl,imx28-lcdif
-      - fsl,imx6sx-lcdif
-      - fsl,imx8mq-lcdif
+    oneOf:
+      - enum:
+          - fsl,imx23-lcdif
+          - fsl,imx28-lcdif
+          - fsl,imx6sx-lcdif
+      - items:
+        - enum:
+          - fsl,imx6sl-lcdif
+          - fsl,imx6sll-lcdif
+          - fsl,imx6ul-lcdif
+          - fsl,imx7d-lcdif
+          - fsl,imx8mq-lcdif
+        - const: fsl,imx6sx-lcdif
 
   reg:
     maxItems: 1