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

Am 28.09.2021 um 11:18 schrieb Maxime Ripard [off-list ref]:

On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:

quoted

quoted

quoted

+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.

quoted

quoted

quoted

+  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?

No, please see bridge/synopsys,dw-hdmi.yaml where it is derived from:

  reg-io-width:
    description:
      Width (in bytes) of the registers specified by the reg property.
    allOf:
      - $ref: /schemas/types.yaml#/definitions/uint32
      - enum: [1, 4]
    default: 1

Other bindings define it explicitly to be 4, e.g.

Documentation//devicetree/bindings/display/intel,keembay-msscam.yaml:  reg-io-width:
Documentation//devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml:  reg-io-width:

Therefore I'd assume that a regmap is not properly set up
if we don't require the DTS to include it with const: 4.

quoted

quoted

quoted

+  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

quoted

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

quoted

quoted

+  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

Nice. It defines:

  clocks:
    minItems: 2
    maxItems: 5
    items:
      - description: The bus clock for either AHB and APB
      - description: The internal register configuration clock
    additionalItems: true

yourself indeed. You can just put clock-names: true

Or should we do

  clocks:
    maxItems: 2
    additionalItems: false

quoted

[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.

Ok, I see. it is not a simple "overwrite" rule.

quoted

quoted

quoted

+      - 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

Ok.

quoted

quoted

quoted

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

Ok. I'll try to make the best out of it for v5 series. Maybe
it is still not perfect by then, but close...

Maxime

BR and thanks,
Nikolaus