Re: [PATCH] dt-bindings: display: Convert a bunch of panels to DT schema

[Rob Herring <robh@kernel.org>]

On Sat, Nov 30, 2019 at 1:43 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Rob.
>
> Thanks for doing this boring, trivial and tiresome task.

It was somewhat scripted at least...

> On Tue, Nov 19, 2019 at 05:13:09PM -0600, Rob Herring wrote:
> > Convert all the 'simple' panels which either use the single
> > 'power-supply' property or don't say and just reference
> > simple-panel.txt. In the later case, bindings are clear as to which
> > properties are required or unused.
> >
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Chen-Yu Tsai <wens@csie.org>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> So Thierry and I ended up as Maintianes for them all.
> I gues thats OK as we look after the panel stuff anyway.
>
> > ---
> > We could perhaps consolidate a bunch of these, but I have no idea how
> > accurate some of these are WRT what's required or not. Seems strange
> > that 'backlight' is optional unless the backlight is tied full on for
> > example. If that's the case, then should backlight ever be required?
> I do not really follow you here.
> Looking through the patch set things looks normal to me.
>
> What do I miss here?

I'm saying a bunch of these could just be a single schema doc with a
long list of compatibles. The variation is in what properties are
required or not.

> I did not find anything I consider bugs. It is mostly small
> inconsistencies.
>
> - Almost all new .yaml files ends with "..."
>   Except one file: nec,nl12880b20-05.yaml
>
>
> - When there is more than one compatible the extra compatible is specified
>   in two ways:

They are different meanings.

>
>   Like this with consts:
>     properties:
>     +  compatible:
>     +    items:
>     +      - const: bananapi,lhr050h41
>     +      - const: ilitek,ili9881c
>     +

This means 'compatible' must have 2 strings in the order specified.

>
>   Link this with enum:
>     +properties:
>     +  compatible:
>     +    enum:
>     +      - urt,umsh-8596md-t
>     +      - urt,umsh-8596md-1t
>     +      - urt,umsh-8596md-7t
>     +      - urt,umsh-8596md-11t
>     +      - urt,umsh-8596md-19t
>     +      - urt,umsh-8596md-20t

This means 'compatible' is a single string of one of the above.

>
> - My OCD prefer only one method to list more than
>   one compatible. Using "enum" syntax above seems to be the common
>   case - and the simple syntax.
>
> - In several cases the original binding provided an example
>   which is now dropped. Is this on purpose?
>   This is very simple examples - so I am happy to see them go.
>   They really did not provide anything extra.

Exactly.

>   I have mentioned it for some - but I stopped as I think
>   they are left out on purpose.
>   The changelog should mention this.

Okay.

>
> - There are some bindings that list a reg property.
>   I have noted that their comment is not keept.

These are all DSI panels. Linus' DSI controller binding defines what
'reg' is, so I prefer not duplicating that everywhere.

> - It seems inconsistent what is listed as properties and mandatory.

That's partly what my comment above on 'backlight' was about. I don't
really know what's just copy-n-paste inconsistencies vs. actual h/w
differences.

>   Most, but not all, include "enable-gpios: true" in properties.
>   And the listed mandatory properties sometimes
>   differ even when the description does not give a hint why.
>   Maybe this was needed to verify existing bindings?

I just maintained what was documented before and haven't checked each
one against usage in in-tree dts files. This is why I've tried to
enforce that folks list which 'simple panel' properties they use.

For example, there's 3 cases for a panel with 'enable-gpios':
- No enable input
- Enable input line is tied off to active state
- Enable input line is connected to GPIO

What's written for the binding probably depends on the board design
the person writing the binding is using. Arguably, 'enable-gpios'
should always be optional because of the 2nd case unless the h/w
always needs s/w control of the enable line.

>
> See a few comments in the following.
>
>         Sam
>
>
> > 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 47950fced28d..a5e6735fe34b 100644
> > --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> > +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> > @@ -85,7 +85,7 @@ examples:
> >          panel@0 {
> >                  compatible = "bananapi,lhr050h41", "ilitek,ili9881c";
> >                  reg = <0>;
> > -                power-gpios = <&pio 1 7 0>; /* PB07 */
> > +                power-supply = <&reg>;
> >                  reset-gpios = <&r_pio 0 5 1>; /* PL05 */
> >                  backlight = <&pwm_bl>;
> >          };
>
> This looks like an unrelated change - drop?

It's not because the example starts failing when the schema validates
it. I can split it out though if you prefer.

[...]

> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8d711f764dfb..ff8e38b269d7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5431,7 +5431,6 @@ S:      Supported
> >  F:   drivers/gpu/drm/fsl-dcu/
> >  F:   Documentation/devicetree/bindings/display/fsl,dcu.txt
> >  F:   Documentation/devicetree/bindings/display/fsl,tcon.txt
> > -F:   Documentation/devicetree/bindings/display/panel/nec,nl4827hc19-05b.txt
> >  T:   git git://anongit.freedesktop.org/drm/drm-misc
> >
> >  DRM DRIVERS FOR FREESCALE IMX
>
> The binding for nec,nl4827hc19-05b.txt should list the original
> maintainers:
> M:      Stefan Agner <stefan@agner.ch>
> M:      Alison Wang <alison.wang@nxp.com>
>
>
> I did not check all - but the files I checked did not have an explicit
> maintainer in MAINTAINERS.

Will double check.

Rob