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

["H. Nikolaus Schaller" <hns@goldelico.com>]

Hi,

> Am 27.09.2021 um 19:07 schrieb maxime@cerno.tech:
> 
> Hi,
> 
> On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:
>> From: Sam Ravnborg <sam@ravnborg.org>
>> 
>> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
>> Based on .txt binding from Zubair Lutfullah Kakakhel
>> 
>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
>> 1 file changed, 85 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> new file mode 100644
>> index 000000000000..5e60cdac4f63
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
>> @@ -0,0 +1,85 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bindings for Ingenic JZ4780 HDMI Transmitter
>> +
>> +maintainers:
>> +  - H. Nikolaus Schaller <hns@goldelico.com>
>> +
>> +description: |
>> +  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
>> +  TX controller IP with accompanying PHY IP.
>> +
>> +allOf:
>> +  - $ref: panel/panel-common.yaml#
> 
> Is it a panel though?

Good question. 

Appears to have to be changed to

  - $ref: bridge/synopsys,dw-hdmi.yaml#

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

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: the address & size of the LCD controller registers
> 
> There's no need for that description, it's obvious enough

Indeed.

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

> 
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Specifies the interrupt provided by parent
> 
> There's no need for that description, it's obvious enough

Indeed.

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

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?

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

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

>> +
>> +  hdmi-regulator: true
>> +    description: Optional regulator to provide +5V at the connector
> 
> regulators need to be suffixed by -supply

My omission...

And, it should be "hdmi-5v-supply" to match driver and device tree.

> 
> You also can just provide the description, you don't need the true there
> 
>> +  ddc-i2c-bus: true
> 
> ditto

Ok

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

> 
> You should also include /schemas/graph.yaml#/$defs/port-base

Ok.

BR and thanks,
Nikolaus