Re: [PATCH v2 6/6] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml

[Rob Herring <robh@kernel.org>]

On Thu, May 14, 2020 at 06:22:39PM +0300, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> On Thu, May 14, 2020 at 11:36:17AM +0200, Ricardo Cañuelo wrote:
> > Hi Laurent, thanks for the thorough review. Some comments below:
> > 
> > On jue 14-05-2020 04:54:12, Laurent Pinchart wrote:
> > > > +description: |
> > > > +  The ADV7511, ADV7511W and ADV7513 are HDMI audio and video
> > > > +  transmitters compatible with HDMI 1.4 and DVI 1.0. They support color
> > > > +  space conversion, S/PDIF, CEC and HDCP. They support RGB input
> > > > +  interface.
> > > 
> > > I would write the last sentence as "The transmitter input is parallel
> > > RGB or YUV data." as YUV is also supported.
> > 
> > Ok.
> > 
> > > > +  adi,input-colorspace:
> > > > +    description: Input color space.
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/string
> > > > +      - enum: [ rgb, yuv422, yuv444 ]
> > > 
> > > Isn't string implied ? Can't you write
> > > 
> > >   adi,input-colorspace:
> > >     description: Input color space.
> > >     enum: [ rgb, yuv422, yuv444 ]
> > 
> > example-schema.yaml says that
> > 
> >     Vendor specific properties have slightly different schema
> >     requirements than common properties. They must have at least a type
> >     definition and 'description'.
> > 
> > However, dt_binding_check doesn't seem to enforce this rule for string
> > properties, and I saw a couple of vendor-specific string properties in
> > other bindings that don't define the type either, so I'm going to follow
> > your suggestion but only for string properties, the rest need a type
> > definition.
> 
> I'll defer to Rob to tell the law here :-)

Yes, if you have a string with defined values, then a type isn't needed. 
That only applies to strings as numeric values need a size.

> 
> > I noticed I can remove the "allOf" keywords from these too.

Yes, that's a recent change. Both forms still work.


> > > > +  adi,embedded-sync:
> > > > +    description:
> > > > +      The input uses synchronization signals embedded in the data
> > > > +      stream (similar to BT.656). Defaults to 0 (separate H/V
> > > > +      synchronization signals).
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > > +      - enum: [ 0, 1 ]
> > > > +      - default: 0
> > > 
> > > This be a boolean property (it is read as a bool by the driver, the
> > > property being absent means false, the property being present means
> > > true).
> > 
> > You're completely right.
> > 
> > > > +  ports:
> > > > +    description:
> > > > +      The ADV7511(W)/13 has two video ports and one audio port. This node
> > > > +      models their connections as documented in
> > > > +      Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > +      Documentation/devicetree/bindings/graph.txt
> > > > +    type: object
> > > > +    properties:
> > > > +      port@0:
> > > > +        description: Video port for the RGB, YUV or DSI input.
> > > 
> > > s/RGB, YUV or DSI/RGB or YUV/
> > 
> > Ok.
> > 
> > > > +if:
> > > > +  not:
> > > > +    properties:
> > > > +      adi,input-colorspace:
> > > > +        contains:
> > > > +          enum: [ rgb, yuv444 ]
> > > > +      adi,input-clock:
> > > > +        contains:
> > > > +          const: 1x
> > > 
> > > As both properties take a single value, I think you can omit
> > > "contains:".
> > 
> > I think it's required here, removing it makes the test fail.
> 
> I thought the following could work:
> 
> if:
>   not:
>     properties:
>       adi,input-colorspace:
>         enum: [ rgb, yuv444 ]
>       adi,input-clock:
>         items:
>           - const: 1x
> 
> But no big deal, contains: is ok too.

In theory the above should work. However, this is probably a case where 
we don't fix-up the properties. If you look at the DT yaml encoding, 
everything is an array (as dtc doesn't know). For schemas, the tooling 
expands scalars to arrays.

Rob