Date:   Fri, 30 Aug 2019 20:01:09 +0200
From:   Jacopo Mondi <jacopo@...ndi.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        Simon Horman <horms@...ge.net.au>, Ulrich Hecht <uli@...nd.eu>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Koji Matsuoka <koji.matsuoka.xm@...esas.com>, muroya@....co.jp,
        VenkataRajesh.Kalakodima@...bosch.com,
        Harsha.ManjulaMallikarjun@...bosch.com,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v3 01/14] dt-bindings: display: renesas,cmm: Add R-Car
 CMM documentation

Hi Laurent,

On Mon, Aug 26, 2019 at 01:15:50PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 26, 2019 at 09:59:43AM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 26, 2019 at 09:34:41AM +0200, Geert Uytterhoeven wrote:
> > > On Sun, Aug 25, 2019 at 3:50 PM Jacopo Mondi <jacopo+renesas@...ndi.org> wrote:
> > > > Add device tree bindings documentation for the Renesas R-Car Display
> > > > Unit Color Management Module.
> > > >
> > > > CMM is the image enhancement module available on each R-Car DU video
> > > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.org>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > > @@ -0,0 +1,33 @@
> > > > +* Renesas R-Car Color Management Module (CMM)
> > > > +
> > > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > > +
> > > > +Required properties:
> > > > + - compatible: shall be one or more of the following:
> > > > +   - "renesas,cmm-r8a7795": for R8A7795 (R-Car H3) compatible CMM.
> > > > +   - "renesas,cmm-r8a7796": for R8A7796 (R-Car M3-W) compatible CMM.
> > > > +   - "renesas,cmm-r8a77965": for R8A77965 (R-Car M3-N) compatible CMM.
> > > > +   - "renesas,cmm-r8a77990": for R8A77990 (R-Car E3) compatible CMM.
> > > > +   - "renesas,cmm-r8a77995": for R8A77995 (R-Car D3) compatible CMM.
> > >
> > > Please use "renesas,<socype->-cmm" instead of "renesas,cmm-<soctype>".
> >
> > I actually copied it from the r-car gpio bindings, and I liked
> > cmm-<soctype> better. If you prefer I can change it though.
> >
> > > > +   - "renesas,rcar-gen3-cmm": for a generic R-Car Gen3 compatible CMM.
> > > > +   - "renesas,rcar-gen2-cmm": for a generic R-Car Gen2 compatible CMM.
> > > > +
> > > > +   When the generic compatible string is specified, the SoC-specific
> > > > +   version corresponding to the platform should be listed first.
> > > > +
> > > > + - reg: the address base and length of the memory area where CMM control
> > > > +   registers are mapped to.
> > > > +
> > > > + - clocks: phandle and clock-specifier pair to the CMM functional clock
> > > > +   supplier.
> > >
> > > Thinking about yaml validation:
> > >
> > > power-domains?
> > > resets?
> >
> > They should indeed be documented.
>
> How about converting this binding to yaml alreay ? It should be fairly
> simple.

I'm trying to, and I'm having my portion of fun time at it.

The definition of the schema itself seems good, but I wonder, is this
the first renesas schema we have? Because it seems to me the schema
validator is having an hard time to digest the examplea 'clocks' and
'power-domains' properties, which have 1 phandle and 2 specifiers and 1
phandle and 1 specifier respectively for Rensas SoCs.

In other words, if in the example I have:

 examples:
   - |
     cmm0: cmm@...40000 {
          compatible = "renesas,r8a7796-cmm";
          reg = <0 0xfea40000 0 0x1000>;
          clocks = <&cpg 711>              <---- 1 phandle + 1 specifier
          resets = <&cpg 711>;
          power-domains = <&sysc>;         <---- 1 phandle
     };

The schema validation is good.

While if I use an actual example
   - |
     cmm0: cmm@...40000 {
          compatible = "renesas,r8a7796-cmm";
          reg = <0 0xfea40000 0 0x1000>;
          clocks = <&cpg CPG_MOD 711>         <---- 1 phandle + 2 specifier
          resets = <&cpg 711>;
          power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; <---- 1 phandle
     };                                                       + 1 specfier

The schema validation fails...
Error: Documentation/devicetree/bindings/display/renesas,cmm.example.dts:20.29-30 syntax error
FATAL ERROR: Unable to parse input tree

Are clocks properties with > 2 entries and power-domains properties with
> 1 entries supported?

Because from what I read here:
https://github.com/robherring/yaml-bindings/blob/master/schemas/clock/clock.yaml
"The length of a clock specifier is defined by the value of a #clock-cells
property in the clock provider node."

And that's expected, but is the examples actually validated against the
clock provider pointed by the phandle? Because in that case, if we had a
yaml schema for the cpg-mssr provider, it would indeed specify clock-cells=2.

Do we need a schema for cpg-mssr first, or am I doing something else
wrong?

Thanks
   j

>
> > > > +Example:
> > > > +--------
> > > > +
> > > > +       cmm0: cmm@...40000 {
> > > > +               compatible = "renesas,cmm-r8a7796";
> > > > +               reg = <0 0xfea40000 0 0x1000>;
> > > > +               power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> > > > +               clocks = <&cpg CPG_MOD 711>;
> > > > +               resets = <&cpg 711>;
> > > > +       };
>
> --
> Regards,
>
> Laurent Pinchart

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists