diff mbox series

[10/54] dt-bindings: display: panel-lvds: Document panel compatibles

Message ID 20210721140424.725744-11-maxime@cerno.tech
State New
Headers show
Series ARM: dts: Last round of DT schema fixes | expand

Commit Message

Maxime Ripard July 21, 2021, 2:03 p.m. UTC
The binding mentions that all the drivers using that driver must use a
vendor-specific compatible but never enforces it, nor documents the
vendor-specific compatibles.

Let's make we document all of them, and that the binding will create an
error if we add one that isn't.

Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 .../bindings/display/panel/lvds.yaml           | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Rob Herring (Arm) July 22, 2021, 2:29 a.m. UTC | #1
On Wed, Jul 21, 2021 at 04:03:40PM +0200, Maxime Ripard wrote:
> The binding mentions that all the drivers using that driver must use a

> vendor-specific compatible but never enforces it, nor documents the

> vendor-specific compatibles.

> 

> Let's make we document all of them, and that the binding will create an

> error if we add one that isn't.

> 

> Cc: dri-devel@lists.freedesktop.org

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Cc: Sam Ravnborg <sam@ravnborg.org>

> Cc: Thierry Reding <thierry.reding@gmail.com>

> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

> ---

>  .../bindings/display/panel/lvds.yaml           | 18 ++++++++++++------

>  1 file changed, 12 insertions(+), 6 deletions(-)

> 

> diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml

> index 49460c9dceea..d1513111eb48 100644

> --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml

> +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml

> @@ -31,12 +31,18 @@ allOf:

>  

>  properties:

>    compatible:

> -    contains:

> -      const: panel-lvds

> -    description:

> -      Shall contain "panel-lvds" in addition to a mandatory panel-specific

> -      compatible string defined in individual panel bindings. The "panel-lvds"

> -      value shall never be used on its own.

> +    items:

> +      - enum:

> +          - advantech,idk-1110wr


At least this one is documented elsewhere. You can add 'minItems: 2' if 
you want to just enforce having 2 compatibles. Or do:

items:
  - {}
  - const: panel-lvds

Which also enforces the order.

> +          - advantech,idk-2121wr

> +          - auo,b101ew05

> +          - innolux,ee101ia-01d

> +          - mitsubishi,aa104xd12

> +          - mitsubishi,aa121td01

> +          - sgd,gktw70sdae4se

> +          - sharp,lq150x1lg11

> +          - tbs,a711-panel

> +      - const: panel-lvds

>  

>    data-mapping:

>      enum:

> -- 

> 2.31.1

> 

>
Maxime Ripard Aug. 18, 2021, 12:43 p.m. UTC | #2
Hi Rob, Sam,

On Wed, Jul 21, 2021 at 08:29:47PM -0600, Rob Herring wrote:
> On Wed, Jul 21, 2021 at 04:03:40PM +0200, Maxime Ripard wrote:

> > The binding mentions that all the drivers using that driver must use a

> > vendor-specific compatible but never enforces it, nor documents the

> > vendor-specific compatibles.

> > 

> > Let's make we document all of them, and that the binding will create an

> > error if we add one that isn't.

> > 

> > Cc: dri-devel@lists.freedesktop.org

> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > Cc: Sam Ravnborg <sam@ravnborg.org>

> > Cc: Thierry Reding <thierry.reding@gmail.com>

> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>

> > ---

> >  .../bindings/display/panel/lvds.yaml           | 18 ++++++++++++------

> >  1 file changed, 12 insertions(+), 6 deletions(-)

> > 

> > diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml

> > index 49460c9dceea..d1513111eb48 100644

> > --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml

> > +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml

> > @@ -31,12 +31,18 @@ allOf:

> >  

> >  properties:

> >    compatible:

> > -    contains:

> > -      const: panel-lvds

> > -    description:

> > -      Shall contain "panel-lvds" in addition to a mandatory panel-specific

> > -      compatible string defined in individual panel bindings. The "panel-lvds"

> > -      value shall never be used on its own.

> > +    items:

> > +      - enum:

> > +          - advantech,idk-1110wr

> 

> At least this one is documented elsewhere.


Indeed, I missed it.

> You can add 'minItems: 2' if you want to just enforce having 2 compatibles. Or do:

> 

> items:

>   - {}

>   - const: panel-lvds

> 

> Which also enforces the order.


It's not just about the order since a missing compatible will also raise
a warning.

Some of those panels have a binding of their own, but some probably
won't (and I can't find anything specific about the one I'm most
interested in: tbs,a711-panel)

Can we have something like:

compatible:
  oneOf:
    - items:
      - enum:
	- tbs,a711-panel
      - const: panel-lvds

    - items:
      - {}
      - const: panel-lvds

That would work for both cases I guess?

Maxime
Rob Herring (Arm) Aug. 18, 2021, 1:48 p.m. UTC | #3
On Wed, Aug 18, 2021 at 7:43 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Rob, Sam,
>
> On Wed, Jul 21, 2021 at 08:29:47PM -0600, Rob Herring wrote:
> > On Wed, Jul 21, 2021 at 04:03:40PM +0200, Maxime Ripard wrote:
> > > The binding mentions that all the drivers using that driver must use a
> > > vendor-specific compatible but never enforces it, nor documents the
> > > vendor-specific compatibles.
> > >
> > > Let's make we document all of them, and that the binding will create an
> > > error if we add one that isn't.
> > >
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Thierry Reding <thierry.reding@gmail.com>
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >  .../bindings/display/panel/lvds.yaml           | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml
> > > index 49460c9dceea..d1513111eb48 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml
> > > +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml
> > > @@ -31,12 +31,18 @@ allOf:
> > >
> > >  properties:
> > >    compatible:
> > > -    contains:
> > > -      const: panel-lvds
> > > -    description:
> > > -      Shall contain "panel-lvds" in addition to a mandatory panel-specific
> > > -      compatible string defined in individual panel bindings. The "panel-lvds"
> > > -      value shall never be used on its own.
> > > +    items:
> > > +      - enum:
> > > +          - advantech,idk-1110wr
> >
> > At least this one is documented elsewhere.
>
> Indeed, I missed it.
>
> > You can add 'minItems: 2' if you want to just enforce having 2 compatibles. Or do:
> >
> > items:
> >   - {}
> >   - const: panel-lvds
> >
> > Which also enforces the order.
>
> It's not just about the order since a missing compatible will also raise
> a warning.
>
> Some of those panels have a binding of their own, but some probably
> won't (and I can't find anything specific about the one I'm most
> interested in: tbs,a711-panel)
>
> Can we have something like:
>
> compatible:
>   oneOf:
>     - items:
>       - enum:
>         - tbs,a711-panel
>       - const: panel-lvds
>
>     - items:
>       - {}
>       - const: panel-lvds
>
> That would work for both cases I guess?

No, both conditions will be true. If you use 'anyOf', then we're never
really checking the specific compatible.

I think the problem here is trying to mix a common binding (aka an
incomplete collection of properties) and a specific binding. The
former is characterized with 'additionalProperties: true' as we have
here. You need to create a 'panel-simple-lvds.yaml' schema file that
references this one, defines all the 'simple' compatibles, and sets
'unevaluatedProperties: false'.

Rob
Maxime Ripard Aug. 23, 2021, 4:31 p.m. UTC | #4
Hi,

On Wed, Aug 18, 2021 at 08:48:46AM -0500, Rob Herring wrote:
> On Wed, Aug 18, 2021 at 7:43 AM Maxime Ripard <maxime@cerno.tech> wrote:

> >

> > Hi Rob, Sam,

> >

> > On Wed, Jul 21, 2021 at 08:29:47PM -0600, Rob Herring wrote:

> > > On Wed, Jul 21, 2021 at 04:03:40PM +0200, Maxime Ripard wrote:

> > > > The binding mentions that all the drivers using that driver must use a

> > > > vendor-specific compatible but never enforces it, nor documents the

> > > > vendor-specific compatibles.

> > > >

> > > > Let's make we document all of them, and that the binding will create an

> > > > error if we add one that isn't.

> > > >

> > > > Cc: dri-devel@lists.freedesktop.org

> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > > Cc: Sam Ravnborg <sam@ravnborg.org>

> > > > Cc: Thierry Reding <thierry.reding@gmail.com>

> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>

> > > > ---

> > > >  .../bindings/display/panel/lvds.yaml           | 18 ++++++++++++------

> > > >  1 file changed, 12 insertions(+), 6 deletions(-)

> > > >

> > > > diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml

> > > > index 49460c9dceea..d1513111eb48 100644

> > > > --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml

> > > > +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml

> > > > @@ -31,12 +31,18 @@ allOf:

> > > >

> > > >  properties:

> > > >    compatible:

> > > > -    contains:

> > > > -      const: panel-lvds

> > > > -    description:

> > > > -      Shall contain "panel-lvds" in addition to a mandatory panel-specific

> > > > -      compatible string defined in individual panel bindings. The "panel-lvds"

> > > > -      value shall never be used on its own.

> > > > +    items:

> > > > +      - enum:

> > > > +          - advantech,idk-1110wr

> > >

> > > At least this one is documented elsewhere.

> >

> > Indeed, I missed it.

> >

> > > You can add 'minItems: 2' if you want to just enforce having 2 compatibles. Or do:

> > >

> > > items:

> > >   - {}

> > >   - const: panel-lvds

> > >

> > > Which also enforces the order.

> >

> > It's not just about the order since a missing compatible will also raise

> > a warning.

> >

> > Some of those panels have a binding of their own, but some probably

> > won't (and I can't find anything specific about the one I'm most

> > interested in: tbs,a711-panel)

> >

> > Can we have something like:

> >

> > compatible:

> >   oneOf:

> >     - items:

> >       - enum:

> >         - tbs,a711-panel

> >       - const: panel-lvds

> >

> >     - items:

> >       - {}

> >       - const: panel-lvds

> >

> > That would work for both cases I guess?

> 

> No, both conditions will be true. If you use 'anyOf', then we're never

> really checking the specific compatible.

> 

> I think the problem here is trying to mix a common binding (aka an

> incomplete collection of properties) and a specific binding.


I'm not entirely sure why we have specific bindings for this in the
first place.

We currently have 6 specific bindings, and for 5 of them the only
specific thing in there are the data-mapping value to force and their
dimension.

I'd argue that the dimension shouldn't even be set in stone: you could
very well imagine a screen with exactly the same timings but a different
size. We would consider it compatible.

And the data-mapping can be dealt with with an if clause fairly easily.

And for the last one, the specific thing about it is that it's using a
dual-link output, which is a generic binding and could thus be described
in panel-lvds too.

Maxime
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml
index 49460c9dceea..d1513111eb48 100644
--- a/Documentation/devicetree/bindings/display/panel/lvds.yaml
+++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml
@@ -31,12 +31,18 @@  allOf:
 
 properties:
   compatible:
-    contains:
-      const: panel-lvds
-    description:
-      Shall contain "panel-lvds" in addition to a mandatory panel-specific
-      compatible string defined in individual panel bindings. The "panel-lvds"
-      value shall never be used on its own.
+    items:
+      - enum:
+          - advantech,idk-1110wr
+          - advantech,idk-2121wr
+          - auo,b101ew05
+          - innolux,ee101ia-01d
+          - mitsubishi,aa104xd12
+          - mitsubishi,aa121td01
+          - sgd,gktw70sdae4se
+          - sharp,lq150x1lg11
+          - tbs,a711-panel
+      - const: panel-lvds
 
   data-mapping:
     enum: