Message-Id: <C529DB99-709A-4C24-B647-3A2004CBFE18@goldelico.com>
Date:   Tue, 28 Sep 2021 10:59:45 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Maxime Ripard <maxime@...no.tech>, Sam Ravnborg <sam@...nborg.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>
Cc:     Paul Cercueil <paul@...pouillou.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Kees Cook <keescook@...omium.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Harry Wentland <harry.wentland@....com>,
        Sam Ravnborg <sam@...nborg.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Paul Boddie <paul@...die.org.uk>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-mips <linux-mips@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, Jonas Karlman <jonas@...boo.se>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi
 DT Schema

Hi,

> Am 27.09.2021 um 19:07 schrieb maxime@...no.tech:
> 
> Hi,
> 
> On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:
>> From: Sam Ravnborg <sam@...nborg.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@...nborg.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>> Cc: Rob Herring <robh@...nel.org>
>> Cc: devicetree@...r.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@...delico.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

Powered by blists - more mailing lists