Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

[Maxime Ripard]

On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: ingenic,jz4780-dw-hdmi
> > 
> > This can just be a const, there's no need for the items
> 
> Maybe starting with an enum is better if more compatible strings are to be added.

it's still fairly easy to change if needed, there's no need to confuse
anyone.

> > 
> >> +  reg-io-width:
> >> +    const: 4
> > 
> > If it's fixed, why do you need it in the first place?
> 
> There is a fixed default of 1 if not specified.

My point was more about why do you need to have that property at all?
Can't you just drop it and assume that the register width is 32 bits if
it's all you will ever run on?

> >> +  clocks:
> >> +    maxItems: 2
> >> +    description: Clock specifiers for isrf and iahb clocks
> > 
> > This can be defined as
> > 
> > clocks:
> >  items:
> >    - description: isrf
> >    - description: iahb
> > 
> > A better description about what these clocks are would be nice as well
> 
> Generally I see that this all is nowadays not independent of
> 
> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml
> 
> where there is already a description.

Ok, good then

> On the other hand every SoC specialization runs its own copy. e.g.
> 
> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam
> 
> > 
> >> +  clock-names:
> >> +    items:
> >> +      - const: isfr
> > 
> > Is it isfr or isrf?
> 
> isfr. Seems to be a typo in the description. See
> bridge/synopsys,dw-hdmi.yaml#
> 
> One question to the yaml specialists:
> 
> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we
> have to repeat? Or can we reduce to just the changes?

If you add the ref you mentionned above, you don't have to repeat
yourself indeed. You can just put clock-names: true

> [I am still not familiar enough with the yaml stuff to understand if
> it has sort of inheritance like device tree include files, so that you
> just have to change relevant properties]

Kind of, but not entirely. schemas are all applied separately, unlike DT
includes that will just expand to one big DT. In practice, it means that
your device must validate against all the schemas, not just the sum of
them.

For example, if you have a generic schema that has:

properties:
  compatible:
    const: vendor,my-generic-compatible


and your schema that extends the generic binding, with a ref to the
generic one that has:

properties:
  compatible:
    items:
      - const: other-vendor,my-device-compatible
      - const: vendor,my-generic-compatible


It will still fail since the generic schema expects only a single
compatible, whereas your device would have two.

> > 
> >> +      - const: iahb
> 
> would it make sense to add additionalItems: false here?
> 
> In the jz4780 case there are just two clocks while other specializations
> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.

If you want to refine the generic one, and it's all the clocks you ever
expect then there's no need for additionalItems

> > 
> >> +    description: An I2C interface if the internal DDC I2C driver is not to be used
> >> +  ports: true
> > 
> > If there's a single port, you don't need ports
> 
> There can be two ports - one for input from LCDC and one
> for output (HDMI connector). But explicitly defining an output
> port is optional to some extent (depending on driver structure).

This needs to be defined then (and port@0 made mandatory)

Maxime