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

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