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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 :-)

> I noticed I can remove the "allOf" keywords from these too.
> 
> > > +  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.

> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - ports
> > > +  - adi,input-depth
> > > +  - adi,input-colorspace
> > > +  - adi,input-clock
> > 
> > Shouldn't the power supplies be required ?
> 
> Good question. The original binding is not completely clear on
> this. There's a "Required properties" section at the top that doesn't
> include the supplies, the supplies block for the ADV7511/11w/13 looks
> like a gray area in that regard, while the same block for ADV7533/35 is
> more explicit in that they are required, and they are not listed in the
> "Optional properties" section.
> 
> However, most of the DTs that use this binding don't define any
> supplies. And AFAICT from looking at the code, regulator_get() will use
> a dummy regulator and show a warning for every expected regulator that's
> not defined in the DT.
> 
> If we want to be more strict and require the definition of all the
> supplies, there will be many more DTs changes in the series, and I'm not
> sure I'll be able to do that in a reasonable amount of time. I'm looking
> at them and it's not always clear which regulators to use or if they are
> even defined.

We can decouple the two though (I think). The bindings should reflect
what we consider right, and the dts files could be fixed on top.

> > > +description: |
> > > +  The ADV7533 and ADV7535 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 DSI for input pixels.
> > 
> > I would write the last sentence as "The transmitter input is MIPI DSI.".
> >
> > ...
> >
> > > +      Disables the interal timing generator. The chip will rely on the
> > 
> > s/interal/internal/
> > 
> > > +      sync signals in the DSI data lanes, rather than generate its own
> > 
> > s/generate/generating/
> >
> > ...
> >
> > > +    properties:
> > > +      port@0:
> > > +        description:
> > > +          Video port for the RGB, YUV or DSI input. The remote endpoint
> > 
> > s/RGB, YUV or //
> 
> Ok.
> 
> > > +        ports {
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            port@0 {
> > > +                reg = <0>;
> > > +                adv7511w_in: endpoint {
> > > +                    remote-endpoint = <&dpi_out>;
> > > +                };
> > > +            };
> > > +
> > > +            port@1 {
> > > +                reg = <1>;
> > > +                adv7511_out: endpoint {
> > > +                    remote-endpoint = <&hdmi_connector_in>;
> > > +                };
> > > +            };
> > 
> > The name of the two endpoints doesn't match the adv7533. The remote
> > endpoint phandle for port 0 should have dsi in its name.
> 
> Oops, thanks for catching these.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel