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
