On Sat, Nov 30, 2019 at 1:43 PM Sam Ravnborg <sam@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> > Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>
> 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 = <®>;
> > 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@xxxxxxxx>
> M: Alison Wang <alison.wang@xxxxxxx>
>
>
> I did not check all - but the files I checked did not have an explicit
> maintainer in MAINTAINERS.
Will double check.
Rob
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
