diff mbox series

[PATCH/RFC,4/6] dt-bindings: display: rockchip: dw-hdmi: Convert binding to YAML

Message ID 20200405233935.27599-5-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series dt-bindings: display: Convert DWC HDMI TX bindings to YAML | expand

Commit Message

Laurent Pinchart April 5, 2020, 11:39 p.m. UTC
Convert the Rockchip HDMI TX text binding to YAML.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
 .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
 2 files changed, 178 insertions(+), 74 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
 create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml

Comments

Maxime Ripard April 6, 2020, 8 a.m. UTC | #1
Hi,

On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> Convert the Rockchip HDMI TX text binding to YAML.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
>  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
>  2 files changed, 178 insertions(+), 74 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
>  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> deleted file mode 100644
> index 3d32ce137e7f..000000000000
> --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -Rockchip DWC HDMI TX Encoder
> -============================
> -
> -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> -with a companion PHY IP.
> -
> -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> -following device-specific properties.
> -
> -
> -Required properties:
> -
> -- compatible: should be one of the following:
> -		"rockchip,rk3228-dw-hdmi"
> -		"rockchip,rk3288-dw-hdmi"
> -		"rockchip,rk3328-dw-hdmi"
> -		"rockchip,rk3399-dw-hdmi"
> -- reg: See dw_hdmi.txt.
> -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> -- interrupts: HDMI interrupt number
> -- clocks: See dw_hdmi.txt.
> -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> -  corresponding to the video input of the controller. The port shall have two
> -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> -
> -Optional properties
> -
> -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> -  connected to a system I2C master this property contains a phandle to that
> -  I2C master controller.
> -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> -- clock-names: May contain "grf", power for grf io.
> -- clock-names: May contain "vpll", external clock for some hdmi phy.
> -- phys: from general PHY binding: the phandle for the PHY device.
> -- phy-names: Should be "hdmi" if phys references an external phy.
> -
> -Optional pinctrl entry:
> -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> -  cause the SDA line to be driven low to work around a hardware
> -  errata.
> -
> -Example:
> -
> -hdmi: hdmi@ff980000 {
> -	compatible = "rockchip,rk3288-dw-hdmi";
> -	reg = <0xff980000 0x20000>;
> -	reg-io-width = <4>;
> -	ddc-i2c-bus = <&i2c5>;
> -	rockchip,grf = <&grf>;
> -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> -	clock-names = "iahb", "isfr";
> -	ports {
> -		hdmi_in: port {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -			hdmi_in_vopb: endpoint@0 {
> -				reg = <0>;
> -				remote-endpoint = <&vopb_out_hdmi>;
> -			};
> -			hdmi_in_vopl: endpoint@1 {
> -				reg = <1>;
> -				remote-endpoint = <&vopl_out_hdmi>;
> -			};
> -		};
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> new file mode 100644
> index 000000000000..8ff544ae0ac2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> @@ -0,0 +1,178 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip DWC HDMI TX Encoder
> +
> +maintainers:
> +  - Mark Yao <mark.yao@rock-chips.com>
> +
> +description: |
> +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> +  with a companion PHY IP.
> +
> +allOf:
> +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3228-dw-hdmi
> +      - rockchip,rk3288-dw-hdmi
> +      - rockchip,rk3328-dw-hdmi
> +      - rockchip,rk3399-dw-hdmi
> +
> +  reg: true
> +
> +  reg-io-width:
> +    const: 4
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 5
> +    items:
> +      - description: The bus clock for either AHB and APB
> +      - description: The internal register configuration clock
> +      - description: The HDMI CEC controller main clock
> +      - description: Power for GRF IO
> +      - description: External clock for some HDMI PHY
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 5
> +    items:
> +      - const: iahb
> +      - const: isfr
> +      - enum:
> +        - cec
> +        - grf
> +        - vpll
> +      - enum:
> +        - cec
> +        - grf
> +        - vpll
> +      - enum:
> +        - cec
> +        - grf
> +        - vpll

IIRC Rob wanted us to standardize the order of the clocks if possible,
since it's a pain to support properly here, and your description won't
match what you describe here either (and in general it's just a best
practice), so if all your DTs have the same order you should just set
that order in stone.

> +  ddc-i2c-bus:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The HDMI DDC bus can be connected to either a system I2C master or the
> +      functionally-reduced I2C master contained in the DWC HDMI. When connected
> +      to a system I2C master this property contains a phandle to that I2C
> +      master controller.
> +
> +  interrupts: true
> +
> +  phys:
> +    maxItems: 1
> +    description: The HDMI PHY
> +
> +  phy-names:
> +    const: hdmi
> +
> +  pinctrl-0: true
> +  pinctrl-1: true

These two are already set by the tools on any schemas (up to
pinctrl-255 actually).

Maxime
Laurent Pinchart April 6, 2020, 11:19 a.m. UTC | #2
Hi Maxime,

On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > Convert the Rockchip HDMI TX text binding to YAML.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> >  2 files changed, 178 insertions(+), 74 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > deleted file mode 100644
> > index 3d32ce137e7f..000000000000
> > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > +++ /dev/null
> > @@ -1,74 +0,0 @@
> > -Rockchip DWC HDMI TX Encoder
> > -============================
> > -
> > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > -with a companion PHY IP.
> > -
> > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > -following device-specific properties.
> > -
> > -
> > -Required properties:
> > -
> > -- compatible: should be one of the following:
> > -		"rockchip,rk3228-dw-hdmi"
> > -		"rockchip,rk3288-dw-hdmi"
> > -		"rockchip,rk3328-dw-hdmi"
> > -		"rockchip,rk3399-dw-hdmi"
> > -- reg: See dw_hdmi.txt.
> > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > -- interrupts: HDMI interrupt number
> > -- clocks: See dw_hdmi.txt.
> > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > -  corresponding to the video input of the controller. The port shall have two
> > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > -
> > -Optional properties
> > -
> > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > -  connected to a system I2C master this property contains a phandle to that
> > -  I2C master controller.
> > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > -- clock-names: May contain "grf", power for grf io.
> > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > -- phys: from general PHY binding: the phandle for the PHY device.
> > -- phy-names: Should be "hdmi" if phys references an external phy.
> > -
> > -Optional pinctrl entry:
> > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > -  cause the SDA line to be driven low to work around a hardware
> > -  errata.
> > -
> > -Example:
> > -
> > -hdmi: hdmi@ff980000 {
> > -	compatible = "rockchip,rk3288-dw-hdmi";
> > -	reg = <0xff980000 0x20000>;
> > -	reg-io-width = <4>;
> > -	ddc-i2c-bus = <&i2c5>;
> > -	rockchip,grf = <&grf>;
> > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > -	clock-names = "iahb", "isfr";
> > -	ports {
> > -		hdmi_in: port {
> > -			#address-cells = <1>;
> > -			#size-cells = <0>;
> > -			hdmi_in_vopb: endpoint@0 {
> > -				reg = <0>;
> > -				remote-endpoint = <&vopb_out_hdmi>;
> > -			};
> > -			hdmi_in_vopl: endpoint@1 {
> > -				reg = <1>;
> > -				remote-endpoint = <&vopl_out_hdmi>;
> > -			};
> > -		};
> > -	};
> > -};
> > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > new file mode 100644
> > index 000000000000..8ff544ae0ac2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > @@ -0,0 +1,178 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip DWC HDMI TX Encoder
> > +
> > +maintainers:
> > +  - Mark Yao <mark.yao@rock-chips.com>
> > +
> > +description: |
> > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > +  with a companion PHY IP.
> > +
> > +allOf:
> > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3228-dw-hdmi
> > +      - rockchip,rk3288-dw-hdmi
> > +      - rockchip,rk3328-dw-hdmi
> > +      - rockchip,rk3399-dw-hdmi
> > +
> > +  reg: true
> > +
> > +  reg-io-width:
> > +    const: 4
> > +
> > +  clocks:
> > +    minItems: 2
> > +    maxItems: 5
> > +    items:
> > +      - description: The bus clock for either AHB and APB
> > +      - description: The internal register configuration clock
> > +      - description: The HDMI CEC controller main clock
> > +      - description: Power for GRF IO
> > +      - description: External clock for some HDMI PHY
> > +
> > +  clock-names:
> > +    minItems: 2
> > +    maxItems: 5
> > +    items:
> > +      - const: iahb
> > +      - const: isfr
> > +      - enum:
> > +        - cec
> > +        - grf
> > +        - vpll
> > +      - enum:
> > +        - cec
> > +        - grf
> > +        - vpll
> > +      - enum:
> > +        - cec
> > +        - grf
> > +        - vpll
> 
> IIRC Rob wanted us to standardize the order of the clocks if possible,
> since it's a pain to support properly here, and your description won't
> match what you describe here either (and in general it's just a best
> practice), so if all your DTs have the same order you should just set
> that order in stone.

But how do we handle the case where any of the cec, grf and vpll clocks
can be set ? Assuming, for instance, that

	clock-names = "iahb", "isfr", "cec";
	clock-names = "iahb", "isfr", "vpll";
	clock-names = "iahb", "isfr", "cec", "vpll";

would all be valid.

> > +  ddc-i2c-bus:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      The HDMI DDC bus can be connected to either a system I2C master or the
> > +      functionally-reduced I2C master contained in the DWC HDMI. When connected
> > +      to a system I2C master this property contains a phandle to that I2C
> > +      master controller.
> > +
> > +  interrupts: true
> > +
> > +  phys:
> > +    maxItems: 1
> > +    description: The HDMI PHY
> > +
> > +  phy-names:
> > +    const: hdmi
> > +
> > +  pinctrl-0: true
> > +  pinctrl-1: true
> 
> These two are already set by the tools on any schemas (up to
> pinctrl-255 actually).

Thank you for the information. I'll drop them.
Maxime Ripard April 6, 2020, 5:09 p.m. UTC | #3
On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > > Convert the Rockchip HDMI TX text binding to YAML.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > >  2 files changed, 178 insertions(+), 74 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > deleted file mode 100644
> > > index 3d32ce137e7f..000000000000
> > > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > +++ /dev/null
> > > @@ -1,74 +0,0 @@
> > > -Rockchip DWC HDMI TX Encoder
> > > -============================
> > > -
> > > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > -with a companion PHY IP.
> > > -
> > > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > > -following device-specific properties.
> > > -
> > > -
> > > -Required properties:
> > > -
> > > -- compatible: should be one of the following:
> > > -		"rockchip,rk3228-dw-hdmi"
> > > -		"rockchip,rk3288-dw-hdmi"
> > > -		"rockchip,rk3328-dw-hdmi"
> > > -		"rockchip,rk3399-dw-hdmi"
> > > -- reg: See dw_hdmi.txt.
> > > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > > -- interrupts: HDMI interrupt number
> > > -- clocks: See dw_hdmi.txt.
> > > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > > -  corresponding to the video input of the controller. The port shall have two
> > > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > > -
> > > -Optional properties
> > > -
> > > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > > -  connected to a system I2C master this property contains a phandle to that
> > > -  I2C master controller.
> > > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > > -- clock-names: May contain "grf", power for grf io.
> > > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > > -- phys: from general PHY binding: the phandle for the PHY device.
> > > -- phy-names: Should be "hdmi" if phys references an external phy.
> > > -
> > > -Optional pinctrl entry:
> > > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > > -  cause the SDA line to be driven low to work around a hardware
> > > -  errata.
> > > -
> > > -Example:
> > > -
> > > -hdmi: hdmi@ff980000 {
> > > -	compatible = "rockchip,rk3288-dw-hdmi";
> > > -	reg = <0xff980000 0x20000>;
> > > -	reg-io-width = <4>;
> > > -	ddc-i2c-bus = <&i2c5>;
> > > -	rockchip,grf = <&grf>;
> > > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > > -	clock-names = "iahb", "isfr";
> > > -	ports {
> > > -		hdmi_in: port {
> > > -			#address-cells = <1>;
> > > -			#size-cells = <0>;
> > > -			hdmi_in_vopb: endpoint@0 {
> > > -				reg = <0>;
> > > -				remote-endpoint = <&vopb_out_hdmi>;
> > > -			};
> > > -			hdmi_in_vopl: endpoint@1 {
> > > -				reg = <1>;
> > > -				remote-endpoint = <&vopl_out_hdmi>;
> > > -			};
> > > -		};
> > > -	};
> > > -};
> > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > new file mode 100644
> > > index 000000000000..8ff544ae0ac2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > @@ -0,0 +1,178 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rockchip DWC HDMI TX Encoder
> > > +
> > > +maintainers:
> > > +  - Mark Yao <mark.yao@rock-chips.com>
> > > +
> > > +description: |
> > > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > +  with a companion PHY IP.
> > > +
> > > +allOf:
> > > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - rockchip,rk3228-dw-hdmi
> > > +      - rockchip,rk3288-dw-hdmi
> > > +      - rockchip,rk3328-dw-hdmi
> > > +      - rockchip,rk3399-dw-hdmi
> > > +
> > > +  reg: true
> > > +
> > > +  reg-io-width:
> > > +    const: 4
> > > +
> > > +  clocks:
> > > +    minItems: 2
> > > +    maxItems: 5
> > > +    items:
> > > +      - description: The bus clock for either AHB and APB
> > > +      - description: The internal register configuration clock
> > > +      - description: The HDMI CEC controller main clock
> > > +      - description: Power for GRF IO
> > > +      - description: External clock for some HDMI PHY
> > > +
> > > +  clock-names:
> > > +    minItems: 2
> > > +    maxItems: 5
> > > +    items:
> > > +      - const: iahb
> > > +      - const: isfr
> > > +      - enum:
> > > +        - cec
> > > +        - grf
> > > +        - vpll
> > > +      - enum:
> > > +        - cec
> > > +        - grf
> > > +        - vpll
> > > +      - enum:
> > > +        - cec
> > > +        - grf
> > > +        - vpll
> >
> > IIRC Rob wanted us to standardize the order of the clocks if possible,
> > since it's a pain to support properly here, and your description won't
> > match what you describe here either (and in general it's just a best
> > practice), so if all your DTs have the same order you should just set
> > that order in stone.
>
> But how do we handle the case where any of the cec, grf and vpll clocks
> can be set ? Assuming, for instance, that
>
> 	clock-names = "iahb", "isfr", "cec";
> 	clock-names = "iahb", "isfr", "vpll";
> 	clock-names = "iahb", "isfr", "cec", "vpll";
>
> would all be valid.

It would be painful then...

The easiest way to do so would be to simply use an enum there, and not
bother checking the array at all. You'll get a warning if there's
multiple occurences of the same string, and I guess that's what you
would be really concerned about.

However, now that I think about it, what's the interaction between the
generic binding and this one when it comes to the third clock? The
generic one expects it to be cec, and here you have other options?

Maxime
Laurent Pinchart April 6, 2020, 5:50 p.m. UTC | #4
Hi Maxime,

On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> > On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > > On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > > > Convert the Rockchip HDMI TX text binding to YAML.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > ---
> > > >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > > >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > > >  2 files changed, 178 insertions(+), 74 deletions(-)
> > > >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > deleted file mode 100644
> > > > index 3d32ce137e7f..000000000000
> > > > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > +++ /dev/null
> > > > @@ -1,74 +0,0 @@
> > > > -Rockchip DWC HDMI TX Encoder
> > > > -============================
> > > > -
> > > > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > -with a companion PHY IP.
> > > > -
> > > > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > > > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > > > -following device-specific properties.
> > > > -
> > > > -
> > > > -Required properties:
> > > > -
> > > > -- compatible: should be one of the following:
> > > > -		"rockchip,rk3228-dw-hdmi"
> > > > -		"rockchip,rk3288-dw-hdmi"
> > > > -		"rockchip,rk3328-dw-hdmi"
> > > > -		"rockchip,rk3399-dw-hdmi"
> > > > -- reg: See dw_hdmi.txt.
> > > > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > > > -- interrupts: HDMI interrupt number
> > > > -- clocks: See dw_hdmi.txt.
> > > > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > > > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > > > -  corresponding to the video input of the controller. The port shall have two
> > > > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > > > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > > > -
> > > > -Optional properties
> > > > -
> > > > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > > > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > > > -  connected to a system I2C master this property contains a phandle to that
> > > > -  I2C master controller.
> > > > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > > > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > > > -- clock-names: May contain "grf", power for grf io.
> > > > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > > > -- phys: from general PHY binding: the phandle for the PHY device.
> > > > -- phy-names: Should be "hdmi" if phys references an external phy.
> > > > -
> > > > -Optional pinctrl entry:
> > > > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > > > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > > > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > > > -  cause the SDA line to be driven low to work around a hardware
> > > > -  errata.
> > > > -
> > > > -Example:
> > > > -
> > > > -hdmi: hdmi@ff980000 {
> > > > -	compatible = "rockchip,rk3288-dw-hdmi";
> > > > -	reg = <0xff980000 0x20000>;
> > > > -	reg-io-width = <4>;
> > > > -	ddc-i2c-bus = <&i2c5>;
> > > > -	rockchip,grf = <&grf>;
> > > > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > > > -	clock-names = "iahb", "isfr";
> > > > -	ports {
> > > > -		hdmi_in: port {
> > > > -			#address-cells = <1>;
> > > > -			#size-cells = <0>;
> > > > -			hdmi_in_vopb: endpoint@0 {
> > > > -				reg = <0>;
> > > > -				remote-endpoint = <&vopb_out_hdmi>;
> > > > -			};
> > > > -			hdmi_in_vopl: endpoint@1 {
> > > > -				reg = <1>;
> > > > -				remote-endpoint = <&vopl_out_hdmi>;
> > > > -			};
> > > > -		};
> > > > -	};
> > > > -};
> > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > new file mode 100644
> > > > index 000000000000..8ff544ae0ac2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > @@ -0,0 +1,178 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rockchip DWC HDMI TX Encoder
> > > > +
> > > > +maintainers:
> > > > +  - Mark Yao <mark.yao@rock-chips.com>
> > > > +
> > > > +description: |
> > > > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > +  with a companion PHY IP.
> > > > +
> > > > +allOf:
> > > > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - rockchip,rk3228-dw-hdmi
> > > > +      - rockchip,rk3288-dw-hdmi
> > > > +      - rockchip,rk3328-dw-hdmi
> > > > +      - rockchip,rk3399-dw-hdmi
> > > > +
> > > > +  reg: true
> > > > +
> > > > +  reg-io-width:
> > > > +    const: 4
> > > > +
> > > > +  clocks:
> > > > +    minItems: 2
> > > > +    maxItems: 5
> > > > +    items:
> > > > +      - description: The bus clock for either AHB and APB
> > > > +      - description: The internal register configuration clock
> > > > +      - description: The HDMI CEC controller main clock
> > > > +      - description: Power for GRF IO
> > > > +      - description: External clock for some HDMI PHY
> > > > +
> > > > +  clock-names:
> > > > +    minItems: 2
> > > > +    maxItems: 5
> > > > +    items:
> > > > +      - const: iahb
> > > > +      - const: isfr
> > > > +      - enum:
> > > > +        - cec
> > > > +        - grf
> > > > +        - vpll
> > > > +      - enum:
> > > > +        - cec
> > > > +        - grf
> > > > +        - vpll
> > > > +      - enum:
> > > > +        - cec
> > > > +        - grf
> > > > +        - vpll
> > >
> > > IIRC Rob wanted us to standardize the order of the clocks if possible,
> > > since it's a pain to support properly here, and your description won't
> > > match what you describe here either (and in general it's just a best
> > > practice), so if all your DTs have the same order you should just set
> > > that order in stone.
> >
> > But how do we handle the case where any of the cec, grf and vpll clocks
> > can be set ? Assuming, for instance, that
> >
> > 	clock-names = "iahb", "isfr", "cec";
> > 	clock-names = "iahb", "isfr", "vpll";
> > 	clock-names = "iahb", "isfr", "cec", "vpll";
> >
> > would all be valid.
> 
> It would be painful then...
> 
> The easiest way to do so would be to simply use an enum there, and not
> bother checking the array at all. You'll get a warning if there's
> multiple occurences of the same string, and I guess that's what you
> would be really concerned about.
> 
> However, now that I think about it, what's the interaction between the
> generic binding and this one when it comes to the third clock? The
> generic one expects it to be cec, and here you have other options?

I'm not too familiar with the platform, but as far as I understand, any
of the cec, grf and vpll clock is optional (if someone could confirm
that, it would be useful). I don't care so much about the order, but
iahb and isfr are mandatory, and thus need to be specified as two const
items in the beginning as far as I understand. It would be nice to set
something along the lines of

  minItems: 2
  maxItems: 5
  items:
    - const: iahb
    - const: isfr
    - enum:
      - cec
      - grf
      - vpll

and have the enum automatically span the last three slots of the items.
I understand this isn't possible, but an equivalent compact would to do
so would be useful.

As for the base schema, it expects three clocks only, so

	clock-names = "iahb", "isfr", "vpll";

won't validate. I can't think of a way around that other than pulling
constraints selectively from the base schema, or just not specifying it
in the base schema at all.
Maxime Ripard April 7, 2020, 7:12 a.m. UTC | #5
On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> > On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> > > On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > > > On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > > > > Convert the Rockchip HDMI TX text binding to YAML.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > ---
> > > > >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > > > >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > > > >  2 files changed, 178 insertions(+), 74 deletions(-)
> > > > >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > deleted file mode 100644
> > > > > index 3d32ce137e7f..000000000000
> > > > > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > +++ /dev/null
> > > > > @@ -1,74 +0,0 @@
> > > > > -Rockchip DWC HDMI TX Encoder
> > > > > -============================
> > > > > -
> > > > > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > -with a companion PHY IP.
> > > > > -
> > > > > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > > > > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > > > > -following device-specific properties.
> > > > > -
> > > > > -
> > > > > -Required properties:
> > > > > -
> > > > > -- compatible: should be one of the following:
> > > > > -		"rockchip,rk3228-dw-hdmi"
> > > > > -		"rockchip,rk3288-dw-hdmi"
> > > > > -		"rockchip,rk3328-dw-hdmi"
> > > > > -		"rockchip,rk3399-dw-hdmi"
> > > > > -- reg: See dw_hdmi.txt.
> > > > > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > > > > -- interrupts: HDMI interrupt number
> > > > > -- clocks: See dw_hdmi.txt.
> > > > > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > > > > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > > > > -  corresponding to the video input of the controller. The port shall have two
> > > > > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > > > > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > > > > -
> > > > > -Optional properties
> > > > > -
> > > > > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > > > > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > > > > -  connected to a system I2C master this property contains a phandle to that
> > > > > -  I2C master controller.
> > > > > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > > > > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > > > > -- clock-names: May contain "grf", power for grf io.
> > > > > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > > > > -- phys: from general PHY binding: the phandle for the PHY device.
> > > > > -- phy-names: Should be "hdmi" if phys references an external phy.
> > > > > -
> > > > > -Optional pinctrl entry:
> > > > > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > > > > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > > > > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > > > > -  cause the SDA line to be driven low to work around a hardware
> > > > > -  errata.
> > > > > -
> > > > > -Example:
> > > > > -
> > > > > -hdmi: hdmi@ff980000 {
> > > > > -	compatible = "rockchip,rk3288-dw-hdmi";
> > > > > -	reg = <0xff980000 0x20000>;
> > > > > -	reg-io-width = <4>;
> > > > > -	ddc-i2c-bus = <&i2c5>;
> > > > > -	rockchip,grf = <&grf>;
> > > > > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > > > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > > > > -	clock-names = "iahb", "isfr";
> > > > > -	ports {
> > > > > -		hdmi_in: port {
> > > > > -			#address-cells = <1>;
> > > > > -			#size-cells = <0>;
> > > > > -			hdmi_in_vopb: endpoint@0 {
> > > > > -				reg = <0>;
> > > > > -				remote-endpoint = <&vopb_out_hdmi>;
> > > > > -			};
> > > > > -			hdmi_in_vopl: endpoint@1 {
> > > > > -				reg = <1>;
> > > > > -				remote-endpoint = <&vopl_out_hdmi>;
> > > > > -			};
> > > > > -		};
> > > > > -	};
> > > > > -};
> > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..8ff544ae0ac2
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > @@ -0,0 +1,178 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Rockchip DWC HDMI TX Encoder
> > > > > +
> > > > > +maintainers:
> > > > > +  - Mark Yao <mark.yao@rock-chips.com>
> > > > > +
> > > > > +description: |
> > > > > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > +  with a companion PHY IP.
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - rockchip,rk3228-dw-hdmi
> > > > > +      - rockchip,rk3288-dw-hdmi
> > > > > +      - rockchip,rk3328-dw-hdmi
> > > > > +      - rockchip,rk3399-dw-hdmi
> > > > > +
> > > > > +  reg: true
> > > > > +
> > > > > +  reg-io-width:
> > > > > +    const: 4
> > > > > +
> > > > > +  clocks:
> > > > > +    minItems: 2
> > > > > +    maxItems: 5
> > > > > +    items:
> > > > > +      - description: The bus clock for either AHB and APB
> > > > > +      - description: The internal register configuration clock
> > > > > +      - description: The HDMI CEC controller main clock
> > > > > +      - description: Power for GRF IO
> > > > > +      - description: External clock for some HDMI PHY
> > > > > +
> > > > > +  clock-names:
> > > > > +    minItems: 2
> > > > > +    maxItems: 5
> > > > > +    items:
> > > > > +      - const: iahb
> > > > > +      - const: isfr
> > > > > +      - enum:
> > > > > +        - cec
> > > > > +        - grf
> > > > > +        - vpll
> > > > > +      - enum:
> > > > > +        - cec
> > > > > +        - grf
> > > > > +        - vpll
> > > > > +      - enum:
> > > > > +        - cec
> > > > > +        - grf
> > > > > +        - vpll
> > > >
> > > > IIRC Rob wanted us to standardize the order of the clocks if possible,
> > > > since it's a pain to support properly here, and your description won't
> > > > match what you describe here either (and in general it's just a best
> > > > practice), so if all your DTs have the same order you should just set
> > > > that order in stone.
> > >
> > > But how do we handle the case where any of the cec, grf and vpll clocks
> > > can be set ? Assuming, for instance, that
> > >
> > > 	clock-names = "iahb", "isfr", "cec";
> > > 	clock-names = "iahb", "isfr", "vpll";
> > > 	clock-names = "iahb", "isfr", "cec", "vpll";
> > >
> > > would all be valid.
> >
> > It would be painful then...
> >
> > The easiest way to do so would be to simply use an enum there, and not
> > bother checking the array at all. You'll get a warning if there's
> > multiple occurences of the same string, and I guess that's what you
> > would be really concerned about.
> >
> > However, now that I think about it, what's the interaction between the
> > generic binding and this one when it comes to the third clock? The
> > generic one expects it to be cec, and here you have other options?
>
> I'm not too familiar with the platform, but as far as I understand, any
> of the cec, grf and vpll clock is optional (if someone could confirm
> that, it would be useful). I don't care so much about the order, but
> iahb and isfr are mandatory, and thus need to be specified as two const
> items in the beginning as far as I understand. It would be nice to set
> something along the lines of
>
>   minItems: 2
>   maxItems: 5
>   items:
>     - const: iahb
>     - const: isfr
>     - enum:
>       - cec
>       - grf
>       - vpll

I guess you could do something like:

in the generic schema:

clock-names:
  allOf:
    - minItems: 2
    - enum:
       - iahb
       - isfr
       - cec
      additonalItems: true
    - items:
      - iahb
      - isfr

Or something along those lines, I haven't tested it, but the basic
idea is that you want to enforce that:
  a) there's a minimum of two clocks
  b) valid clock names are iahb, isfr and cec, but we will allow more
  c) the first two clocks are iahb and isfr

Now, on the rockchip binding, we can simply have:

clock-names:
  enum:
    - iahb
    - isfr
    - cec
    - grf
    - vpll

This way, here we enforce the available clock names, while keeping the
constraints set in the generic binding.

Maxime
Laurent Pinchart April 8, 2020, 11:45 a.m. UTC | #6
Hi Maxime,

On Tue, Apr 07, 2020 at 09:12:51AM +0200, Maxime Ripard wrote:
> On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> > On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> > > On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> > > > On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > > > > On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > > > > > Convert the Rockchip HDMI TX text binding to YAML.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > ---
> > > > > >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > > > > >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > > > > >  2 files changed, 178 insertions(+), 74 deletions(-)
> > > > > >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > deleted file mode 100644
> > > > > > index 3d32ce137e7f..000000000000
> > > > > > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > +++ /dev/null
> > > > > > @@ -1,74 +0,0 @@
> > > > > > -Rockchip DWC HDMI TX Encoder
> > > > > > -============================
> > > > > > -
> > > > > > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > > -with a companion PHY IP.
> > > > > > -
> > > > > > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > > > > > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > > > > > -following device-specific properties.
> > > > > > -
> > > > > > -
> > > > > > -Required properties:
> > > > > > -
> > > > > > -- compatible: should be one of the following:
> > > > > > -		"rockchip,rk3228-dw-hdmi"
> > > > > > -		"rockchip,rk3288-dw-hdmi"
> > > > > > -		"rockchip,rk3328-dw-hdmi"
> > > > > > -		"rockchip,rk3399-dw-hdmi"
> > > > > > -- reg: See dw_hdmi.txt.
> > > > > > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > > > > > -- interrupts: HDMI interrupt number
> > > > > > -- clocks: See dw_hdmi.txt.
> > > > > > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > > > > > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > > > > > -  corresponding to the video input of the controller. The port shall have two
> > > > > > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > > > > > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > > > > > -
> > > > > > -Optional properties
> > > > > > -
> > > > > > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > > > > > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > > > > > -  connected to a system I2C master this property contains a phandle to that
> > > > > > -  I2C master controller.
> > > > > > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > > > > > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > > > > > -- clock-names: May contain "grf", power for grf io.
> > > > > > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > > > > > -- phys: from general PHY binding: the phandle for the PHY device.
> > > > > > -- phy-names: Should be "hdmi" if phys references an external phy.
> > > > > > -
> > > > > > -Optional pinctrl entry:
> > > > > > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > > > > > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > > > > > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > > > > > -  cause the SDA line to be driven low to work around a hardware
> > > > > > -  errata.
> > > > > > -
> > > > > > -Example:
> > > > > > -
> > > > > > -hdmi: hdmi@ff980000 {
> > > > > > -	compatible = "rockchip,rk3288-dw-hdmi";
> > > > > > -	reg = <0xff980000 0x20000>;
> > > > > > -	reg-io-width = <4>;
> > > > > > -	ddc-i2c-bus = <&i2c5>;
> > > > > > -	rockchip,grf = <&grf>;
> > > > > > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > > > > > -	clock-names = "iahb", "isfr";
> > > > > > -	ports {
> > > > > > -		hdmi_in: port {
> > > > > > -			#address-cells = <1>;
> > > > > > -			#size-cells = <0>;
> > > > > > -			hdmi_in_vopb: endpoint@0 {
> > > > > > -				reg = <0>;
> > > > > > -				remote-endpoint = <&vopb_out_hdmi>;
> > > > > > -			};
> > > > > > -			hdmi_in_vopl: endpoint@1 {
> > > > > > -				reg = <1>;
> > > > > > -				remote-endpoint = <&vopl_out_hdmi>;
> > > > > > -			};
> > > > > > -		};
> > > > > > -	};
> > > > > > -};
> > > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..8ff544ae0ac2
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > @@ -0,0 +1,178 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Rockchip DWC HDMI TX Encoder
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Mark Yao <mark.yao@rock-chips.com>
> > > > > > +
> > > > > > +description: |
> > > > > > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > > +  with a companion PHY IP.
> > > > > > +
> > > > > > +allOf:
> > > > > > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - rockchip,rk3228-dw-hdmi
> > > > > > +      - rockchip,rk3288-dw-hdmi
> > > > > > +      - rockchip,rk3328-dw-hdmi
> > > > > > +      - rockchip,rk3399-dw-hdmi
> > > > > > +
> > > > > > +  reg: true
> > > > > > +
> > > > > > +  reg-io-width:
> > > > > > +    const: 4
> > > > > > +
> > > > > > +  clocks:
> > > > > > +    minItems: 2
> > > > > > +    maxItems: 5
> > > > > > +    items:
> > > > > > +      - description: The bus clock for either AHB and APB
> > > > > > +      - description: The internal register configuration clock
> > > > > > +      - description: The HDMI CEC controller main clock
> > > > > > +      - description: Power for GRF IO
> > > > > > +      - description: External clock for some HDMI PHY
> > > > > > +
> > > > > > +  clock-names:
> > > > > > +    minItems: 2
> > > > > > +    maxItems: 5
> > > > > > +    items:
> > > > > > +      - const: iahb
> > > > > > +      - const: isfr
> > > > > > +      - enum:
> > > > > > +        - cec
> > > > > > +        - grf
> > > > > > +        - vpll
> > > > > > +      - enum:
> > > > > > +        - cec
> > > > > > +        - grf
> > > > > > +        - vpll
> > > > > > +      - enum:
> > > > > > +        - cec
> > > > > > +        - grf
> > > > > > +        - vpll
> > > > >
> > > > > IIRC Rob wanted us to standardize the order of the clocks if possible,
> > > > > since it's a pain to support properly here, and your description won't
> > > > > match what you describe here either (and in general it's just a best
> > > > > practice), so if all your DTs have the same order you should just set
> > > > > that order in stone.
> > > >
> > > > But how do we handle the case where any of the cec, grf and vpll clocks
> > > > can be set ? Assuming, for instance, that
> > > >
> > > > 	clock-names = "iahb", "isfr", "cec";
> > > > 	clock-names = "iahb", "isfr", "vpll";
> > > > 	clock-names = "iahb", "isfr", "cec", "vpll";
> > > >
> > > > would all be valid.
> > >
> > > It would be painful then...
> > >
> > > The easiest way to do so would be to simply use an enum there, and not
> > > bother checking the array at all. You'll get a warning if there's
> > > multiple occurences of the same string, and I guess that's what you
> > > would be really concerned about.
> > >
> > > However, now that I think about it, what's the interaction between the
> > > generic binding and this one when it comes to the third clock? The
> > > generic one expects it to be cec, and here you have other options?
> >
> > I'm not too familiar with the platform, but as far as I understand, any
> > of the cec, grf and vpll clock is optional (if someone could confirm
> > that, it would be useful). I don't care so much about the order, but
> > iahb and isfr are mandatory, and thus need to be specified as two const
> > items in the beginning as far as I understand. It would be nice to set
> > something along the lines of
> >
> >   minItems: 2
> >   maxItems: 5
> >   items:
> >     - const: iahb
> >     - const: isfr
> >     - enum:
> >       - cec
> >       - grf
> >       - vpll
> 
> I guess you could do something like:
> 
> in the generic schema:
> 
> clock-names:
>   allOf:
>     - minItems: 2
>     - enum:
>        - iahb
>        - isfr
>        - cec
>       additonalItems: true
>     - items:
>       - iahb
>       - isfr
> 
> Or something along those lines, I haven't tested it, but the basic
> idea is that you want to enforce that:
>   a) there's a minimum of two clocks
>   b) valid clock names are iahb, isfr and cec, but we will allow more
>   c) the first two clocks are iahb and isfr

Interesting idea. I've tried

  clock-names:
    allOf:
      - minItems: 2
      - enum:
        - iahb
        - isfr
        - cec
        additionalItems: true
      - items:
        - const: iahb
        - const: isfr

in the base synopsys,dw-hdmi.yaml schema, and

  clock-names:
    maxItems: 2

in renesas,dw-hdmi.yaml, which resulted in the following validation
errors:

Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.example.dt.yaml: hdmi@fead0000: clock-names: ['iahb', 'isfr'] is too long

This is caused by the enum entry.

> Now, on the rockchip binding, we can simply have:
> 
> clock-names:
>   enum:
>     - iahb
>     - isfr
>     - cec
>     - grf
>     - vpll
> 
> This way, here we enforce the available clock names, while keeping the
> constraints set in the generic binding.
Maxime Ripard April 8, 2020, 12:25 p.m. UTC | #7
On Wed, Apr 08, 2020 at 02:45:52PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> On Tue, Apr 07, 2020 at 09:12:51AM +0200, Maxime Ripard wrote:
> > On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> > > On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> > > > On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> > > > > On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > > > > > On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > > > > > > Convert the Rockchip HDMI TX text binding to YAML.
> > > > > > >
> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > > ---
> > > > > > >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > > > > > >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > > > > > >  2 files changed, 178 insertions(+), 74 deletions(-)
> > > > > > >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > > deleted file mode 100644
> > > > > > > index 3d32ce137e7f..000000000000
> > > > > > > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > > +++ /dev/null
> > > > > > > @@ -1,74 +0,0 @@
> > > > > > > -Rockchip DWC HDMI TX Encoder
> > > > > > > -============================
> > > > > > > -
> > > > > > > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > > > -with a companion PHY IP.
> > > > > > > -
> > > > > > > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > > > > > > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > > > > > > -following device-specific properties.
> > > > > > > -
> > > > > > > -
> > > > > > > -Required properties:
> > > > > > > -
> > > > > > > -- compatible: should be one of the following:
> > > > > > > -		"rockchip,rk3228-dw-hdmi"
> > > > > > > -		"rockchip,rk3288-dw-hdmi"
> > > > > > > -		"rockchip,rk3328-dw-hdmi"
> > > > > > > -		"rockchip,rk3399-dw-hdmi"
> > > > > > > -- reg: See dw_hdmi.txt.
> > > > > > > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > > > > > > -- interrupts: HDMI interrupt number
> > > > > > > -- clocks: See dw_hdmi.txt.
> > > > > > > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > > > > > > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > > > > > > -  corresponding to the video input of the controller. The port shall have two
> > > > > > > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > > > > > > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > > > > > > -
> > > > > > > -Optional properties
> > > > > > > -
> > > > > > > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > > > > > > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > > > > > > -  connected to a system I2C master this property contains a phandle to that
> > > > > > > -  I2C master controller.
> > > > > > > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > > > > > > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > > > > > > -- clock-names: May contain "grf", power for grf io.
> > > > > > > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > > > > > > -- phys: from general PHY binding: the phandle for the PHY device.
> > > > > > > -- phy-names: Should be "hdmi" if phys references an external phy.
> > > > > > > -
> > > > > > > -Optional pinctrl entry:
> > > > > > > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > > > > > > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > > > > > > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > > > > > > -  cause the SDA line to be driven low to work around a hardware
> > > > > > > -  errata.
> > > > > > > -
> > > > > > > -Example:
> > > > > > > -
> > > > > > > -hdmi: hdmi@ff980000 {
> > > > > > > -	compatible = "rockchip,rk3288-dw-hdmi";
> > > > > > > -	reg = <0xff980000 0x20000>;
> > > > > > > -	reg-io-width = <4>;
> > > > > > > -	ddc-i2c-bus = <&i2c5>;
> > > > > > > -	rockchip,grf = <&grf>;
> > > > > > > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > > > > > > -	clock-names = "iahb", "isfr";
> > > > > > > -	ports {
> > > > > > > -		hdmi_in: port {
> > > > > > > -			#address-cells = <1>;
> > > > > > > -			#size-cells = <0>;
> > > > > > > -			hdmi_in_vopb: endpoint@0 {
> > > > > > > -				reg = <0>;
> > > > > > > -				remote-endpoint = <&vopb_out_hdmi>;
> > > > > > > -			};
> > > > > > > -			hdmi_in_vopl: endpoint@1 {
> > > > > > > -				reg = <1>;
> > > > > > > -				remote-endpoint = <&vopl_out_hdmi>;
> > > > > > > -			};
> > > > > > > -		};
> > > > > > > -	};
> > > > > > > -};
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..8ff544ae0ac2
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > > @@ -0,0 +1,178 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Rockchip DWC HDMI TX Encoder
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Mark Yao <mark.yao@rock-chips.com>
> > > > > > > +
> > > > > > > +description: |
> > > > > > > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > > > +  with a companion PHY IP.
> > > > > > > +
> > > > > > > +allOf:
> > > > > > > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    enum:
> > > > > > > +      - rockchip,rk3228-dw-hdmi
> > > > > > > +      - rockchip,rk3288-dw-hdmi
> > > > > > > +      - rockchip,rk3328-dw-hdmi
> > > > > > > +      - rockchip,rk3399-dw-hdmi
> > > > > > > +
> > > > > > > +  reg: true
> > > > > > > +
> > > > > > > +  reg-io-width:
> > > > > > > +    const: 4
> > > > > > > +
> > > > > > > +  clocks:
> > > > > > > +    minItems: 2
> > > > > > > +    maxItems: 5
> > > > > > > +    items:
> > > > > > > +      - description: The bus clock for either AHB and APB
> > > > > > > +      - description: The internal register configuration clock
> > > > > > > +      - description: The HDMI CEC controller main clock
> > > > > > > +      - description: Power for GRF IO
> > > > > > > +      - description: External clock for some HDMI PHY
> > > > > > > +
> > > > > > > +  clock-names:
> > > > > > > +    minItems: 2
> > > > > > > +    maxItems: 5
> > > > > > > +    items:
> > > > > > > +      - const: iahb
> > > > > > > +      - const: isfr
> > > > > > > +      - enum:
> > > > > > > +        - cec
> > > > > > > +        - grf
> > > > > > > +        - vpll
> > > > > > > +      - enum:
> > > > > > > +        - cec
> > > > > > > +        - grf
> > > > > > > +        - vpll
> > > > > > > +      - enum:
> > > > > > > +        - cec
> > > > > > > +        - grf
> > > > > > > +        - vpll
> > > > > >
> > > > > > IIRC Rob wanted us to standardize the order of the clocks if possible,
> > > > > > since it's a pain to support properly here, and your description won't
> > > > > > match what you describe here either (and in general it's just a best
> > > > > > practice), so if all your DTs have the same order you should just set
> > > > > > that order in stone.
> > > > >
> > > > > But how do we handle the case where any of the cec, grf and vpll clocks
> > > > > can be set ? Assuming, for instance, that
> > > > >
> > > > > 	clock-names = "iahb", "isfr", "cec";
> > > > > 	clock-names = "iahb", "isfr", "vpll";
> > > > > 	clock-names = "iahb", "isfr", "cec", "vpll";
> > > > >
> > > > > would all be valid.
> > > >
> > > > It would be painful then...
> > > >
> > > > The easiest way to do so would be to simply use an enum there, and not
> > > > bother checking the array at all. You'll get a warning if there's
> > > > multiple occurences of the same string, and I guess that's what you
> > > > would be really concerned about.
> > > >
> > > > However, now that I think about it, what's the interaction between the
> > > > generic binding and this one when it comes to the third clock? The
> > > > generic one expects it to be cec, and here you have other options?
> > >
> > > I'm not too familiar with the platform, but as far as I understand, any
> > > of the cec, grf and vpll clock is optional (if someone could confirm
> > > that, it would be useful). I don't care so much about the order, but
> > > iahb and isfr are mandatory, and thus need to be specified as two const
> > > items in the beginning as far as I understand. It would be nice to set
> > > something along the lines of
> > >
> > >   minItems: 2
> > >   maxItems: 5
> > >   items:
> > >     - const: iahb
> > >     - const: isfr
> > >     - enum:
> > >       - cec
> > >       - grf
> > >       - vpll
> >
> > I guess you could do something like:
> >
> > in the generic schema:
> >
> > clock-names:
> >   allOf:
> >     - minItems: 2
> >     - enum:
> >        - iahb
> >        - isfr
> >        - cec
> >       additonalItems: true
> >     - items:
> >       - iahb
> >       - isfr
> >
> > Or something along those lines, I haven't tested it, but the basic
> > idea is that you want to enforce that:
> >   a) there's a minimum of two clocks
> >   b) valid clock names are iahb, isfr and cec, but we will allow more
> >   c) the first two clocks are iahb and isfr
>
> Interesting idea. I've tried
>
>   clock-names:
>     allOf:
>       - minItems: 2
>       - enum:
>         - iahb
>         - isfr
>         - cec
>         additionalItems: true
>       - items:
>         - const: iahb
>         - const: isfr
>
> in the base synopsys,dw-hdmi.yaml schema, and
>
>   clock-names:
>     maxItems: 2
>
> in renesas,dw-hdmi.yaml, which resulted in the following validation
> errors:
>
> Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.example.dt.yaml: hdmi@fead0000: clock-names: ['iahb', 'isfr'] is too long
>
> This is caused by the enum entry.

What happens if you have instead of
- enum:
 ...

- items:
    enum:
      ...

Maxime
Laurent Pinchart April 8, 2020, 2:01 p.m. UTC | #8
Hi Maxime,

On Wed, Apr 08, 2020 at 02:25:19PM +0200, Maxime Ripard wrote:
> On Wed, Apr 08, 2020 at 02:45:52PM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 07, 2020 at 09:12:51AM +0200, Maxime Ripard wrote:
> >> On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> >>> On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> >>>> On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> >>>>> On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> >>>>>> On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> >>>>>>> Convert the Rockchip HDMI TX text binding to YAML.
> >>>>>>>
> >>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>> ---
> >>>>>>>  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> >>>>>>>  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> >>>>>>>  2 files changed, 178 insertions(+), 74 deletions(-)
> >>>>>>>  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >>>>>>> deleted file mode 100644
> >>>>>>> index 3d32ce137e7f..000000000000
> >>>>>>> --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >>>>>>> +++ /dev/null
> >>>>>>> @@ -1,74 +0,0 @@
> >>>>>>> -Rockchip DWC HDMI TX Encoder
> >>>>>>> -============================
> >>>>>>> -
> >>>>>>> -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> >>>>>>> -with a companion PHY IP.
> >>>>>>> -
> >>>>>>> -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> >>>>>>> -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> >>>>>>> -following device-specific properties.
> >>>>>>> -
> >>>>>>> -
> >>>>>>> -Required properties:
> >>>>>>> -
> >>>>>>> -- compatible: should be one of the following:
> >>>>>>> -		"rockchip,rk3228-dw-hdmi"
> >>>>>>> -		"rockchip,rk3288-dw-hdmi"
> >>>>>>> -		"rockchip,rk3328-dw-hdmi"
> >>>>>>> -		"rockchip,rk3399-dw-hdmi"
> >>>>>>> -- reg: See dw_hdmi.txt.
> >>>>>>> -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> >>>>>>> -- interrupts: HDMI interrupt number
> >>>>>>> -- clocks: See dw_hdmi.txt.
> >>>>>>> -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> >>>>>>> -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> >>>>>>> -  corresponding to the video input of the controller. The port shall have two
> >>>>>>> -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> >>>>>>> -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> >>>>>>> -
> >>>>>>> -Optional properties
> >>>>>>> -
> >>>>>>> -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> >>>>>>> -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> >>>>>>> -  connected to a system I2C master this property contains a phandle to that
> >>>>>>> -  I2C master controller.
> >>>>>>> -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> >>>>>>> -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> >>>>>>> -- clock-names: May contain "grf", power for grf io.
> >>>>>>> -- clock-names: May contain "vpll", external clock for some hdmi phy.
> >>>>>>> -- phys: from general PHY binding: the phandle for the PHY device.
> >>>>>>> -- phy-names: Should be "hdmi" if phys references an external phy.
> >>>>>>> -
> >>>>>>> -Optional pinctrl entry:
> >>>>>>> -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> >>>>>>> -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> >>>>>>> -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> >>>>>>> -  cause the SDA line to be driven low to work around a hardware
> >>>>>>> -  errata.
> >>>>>>> -
> >>>>>>> -Example:
> >>>>>>> -
> >>>>>>> -hdmi: hdmi@ff980000 {
> >>>>>>> -	compatible = "rockchip,rk3288-dw-hdmi";
> >>>>>>> -	reg = <0xff980000 0x20000>;
> >>>>>>> -	reg-io-width = <4>;
> >>>>>>> -	ddc-i2c-bus = <&i2c5>;
> >>>>>>> -	rockchip,grf = <&grf>;
> >>>>>>> -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> >>>>>>> -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> >>>>>>> -	clock-names = "iahb", "isfr";
> >>>>>>> -	ports {
> >>>>>>> -		hdmi_in: port {
> >>>>>>> -			#address-cells = <1>;
> >>>>>>> -			#size-cells = <0>;
> >>>>>>> -			hdmi_in_vopb: endpoint@0 {
> >>>>>>> -				reg = <0>;
> >>>>>>> -				remote-endpoint = <&vopb_out_hdmi>;
> >>>>>>> -			};
> >>>>>>> -			hdmi_in_vopl: endpoint@1 {
> >>>>>>> -				reg = <1>;
> >>>>>>> -				remote-endpoint = <&vopl_out_hdmi>;
> >>>>>>> -			};
> >>>>>>> -		};
> >>>>>>> -	};
> >>>>>>> -};
> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..8ff544ae0ac2
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >>>>>>> @@ -0,0 +1,178 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Rockchip DWC HDMI TX Encoder
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Mark Yao <mark.yao@rock-chips.com>
> >>>>>>> +
> >>>>>>> +description: |
> >>>>>>> +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> >>>>>>> +  with a companion PHY IP.
> >>>>>>> +
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    enum:
> >>>>>>> +      - rockchip,rk3228-dw-hdmi
> >>>>>>> +      - rockchip,rk3288-dw-hdmi
> >>>>>>> +      - rockchip,rk3328-dw-hdmi
> >>>>>>> +      - rockchip,rk3399-dw-hdmi
> >>>>>>> +
> >>>>>>> +  reg: true
> >>>>>>> +
> >>>>>>> +  reg-io-width:
> >>>>>>> +    const: 4
> >>>>>>> +
> >>>>>>> +  clocks:
> >>>>>>> +    minItems: 2
> >>>>>>> +    maxItems: 5
> >>>>>>> +    items:
> >>>>>>> +      - description: The bus clock for either AHB and APB
> >>>>>>> +      - description: The internal register configuration clock
> >>>>>>> +      - description: The HDMI CEC controller main clock
> >>>>>>> +      - description: Power for GRF IO
> >>>>>>> +      - description: External clock for some HDMI PHY
> >>>>>>> +
> >>>>>>> +  clock-names:
> >>>>>>> +    minItems: 2
> >>>>>>> +    maxItems: 5
> >>>>>>> +    items:
> >>>>>>> +      - const: iahb
> >>>>>>> +      - const: isfr
> >>>>>>> +      - enum:
> >>>>>>> +        - cec
> >>>>>>> +        - grf
> >>>>>>> +        - vpll
> >>>>>>> +      - enum:
> >>>>>>> +        - cec
> >>>>>>> +        - grf
> >>>>>>> +        - vpll
> >>>>>>> +      - enum:
> >>>>>>> +        - cec
> >>>>>>> +        - grf
> >>>>>>> +        - vpll
> >>>>>>
> >>>>>> IIRC Rob wanted us to standardize the order of the clocks if possible,
> >>>>>> since it's a pain to support properly here, and your description won't
> >>>>>> match what you describe here either (and in general it's just a best
> >>>>>> practice), so if all your DTs have the same order you should just set
> >>>>>> that order in stone.
> >>>>>
> >>>>> But how do we handle the case where any of the cec, grf and vpll clocks
> >>>>> can be set ? Assuming, for instance, that
> >>>>>
> >>>>> 	clock-names = "iahb", "isfr", "cec";
> >>>>> 	clock-names = "iahb", "isfr", "vpll";
> >>>>> 	clock-names = "iahb", "isfr", "cec", "vpll";
> >>>>>
> >>>>> would all be valid.
> >>>>
> >>>> It would be painful then...
> >>>>
> >>>> The easiest way to do so would be to simply use an enum there, and not
> >>>> bother checking the array at all. You'll get a warning if there's
> >>>> multiple occurences of the same string, and I guess that's what you
> >>>> would be really concerned about.
> >>>>
> >>>> However, now that I think about it, what's the interaction between the
> >>>> generic binding and this one when it comes to the third clock? The
> >>>> generic one expects it to be cec, and here you have other options?
> >>>
> >>> I'm not too familiar with the platform, but as far as I understand, any
> >>> of the cec, grf and vpll clock is optional (if someone could confirm
> >>> that, it would be useful). I don't care so much about the order, but
> >>> iahb and isfr are mandatory, and thus need to be specified as two const
> >>> items in the beginning as far as I understand. It would be nice to set
> >>> something along the lines of
> >>>
> >>>   minItems: 2
> >>>   maxItems: 5
> >>>   items:
> >>>     - const: iahb
> >>>     - const: isfr
> >>>     - enum:
> >>>       - cec
> >>>       - grf
> >>>       - vpll
> >>
> >> I guess you could do something like:
> >>
> >> in the generic schema:
> >>
> >> clock-names:
> >>   allOf:
> >>     - minItems: 2
> >>     - enum:
> >>        - iahb
> >>        - isfr
> >>        - cec
> >>       additonalItems: true
> >>     - items:
> >>       - iahb
> >>       - isfr
> >>
> >> Or something along those lines, I haven't tested it, but the basic
> >> idea is that you want to enforce that:
> >>   a) there's a minimum of two clocks
> >>   b) valid clock names are iahb, isfr and cec, but we will allow more
> >>   c) the first two clocks are iahb and isfr
> >
> > Interesting idea. I've tried
> >
> >   clock-names:
> >     allOf:
> >       - minItems: 2
> >       - enum:
> >         - iahb
> >         - isfr
> >         - cec
> >         additionalItems: true
> >       - items:
> >         - const: iahb
> >         - const: isfr
> >
> > in the base synopsys,dw-hdmi.yaml schema, and
> >
> >   clock-names:
> >     maxItems: 2
> >
> > in renesas,dw-hdmi.yaml, which resulted in the following validation
> > errors:
> >
> > Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.example.dt.yaml: hdmi@fead0000: clock-names: ['iahb', 'isfr'] is too long
> >
> > This is caused by the enum entry.
> 
> What happens if you have instead of
> - enum:
>  ...
> 
> - items:
>     enum:
>       ...

That works, but if I then specify a cec clock in the example in
renesas,dw-hdmi.yaml, I get

Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.example.dt.yaml: hdmi@fead0000: clock-names: ['iahb', 'isfr', 'cec'] is too long
Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.example.dt.yaml: hdmi@fead0000: clock-names: Additional items are not allowed ('cec' was unexpected)

which is caused by

       - items:
         - const: iahb
         - const: isfr
Rob Herring April 14, 2020, 11:10 p.m. UTC | #9
On Wed, Apr 08, 2020 at 02:45:52PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
> 
> On Tue, Apr 07, 2020 at 09:12:51AM +0200, Maxime Ripard wrote:
> > On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> > > On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> > > > On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> > > > > On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > > > > > On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > > > > > > Convert the Rockchip HDMI TX text binding to YAML.
> > > > > > >
> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > > ---
> > > > > > >  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > > > > > >  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > > > > > >  2 files changed, 178 insertions(+), 74 deletions(-)
> > > > > > >  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > > deleted file mode 100644
> > > > > > > index 3d32ce137e7f..000000000000
> > > > > > > --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > > > > > > +++ /dev/null
> > > > > > > @@ -1,74 +0,0 @@
> > > > > > > -Rockchip DWC HDMI TX Encoder
> > > > > > > -============================
> > > > > > > -
> > > > > > > -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > > > -with a companion PHY IP.
> > > > > > > -
> > > > > > > -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > > > > > > -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > > > > > > -following device-specific properties.
> > > > > > > -
> > > > > > > -
> > > > > > > -Required properties:
> > > > > > > -
> > > > > > > -- compatible: should be one of the following:
> > > > > > > -		"rockchip,rk3228-dw-hdmi"
> > > > > > > -		"rockchip,rk3288-dw-hdmi"
> > > > > > > -		"rockchip,rk3328-dw-hdmi"
> > > > > > > -		"rockchip,rk3399-dw-hdmi"
> > > > > > > -- reg: See dw_hdmi.txt.
> > > > > > > -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > > > > > > -- interrupts: HDMI interrupt number
> > > > > > > -- clocks: See dw_hdmi.txt.
> > > > > > > -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > > > > > > -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > > > > > > -  corresponding to the video input of the controller. The port shall have two
> > > > > > > -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > > > > > > -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > > > > > > -
> > > > > > > -Optional properties
> > > > > > > -
> > > > > > > -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > > > > > > -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > > > > > > -  connected to a system I2C master this property contains a phandle to that
> > > > > > > -  I2C master controller.
> > > > > > > -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > > > > > > -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > > > > > > -- clock-names: May contain "grf", power for grf io.
> > > > > > > -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > > > > > > -- phys: from general PHY binding: the phandle for the PHY device.
> > > > > > > -- phy-names: Should be "hdmi" if phys references an external phy.
> > > > > > > -
> > > > > > > -Optional pinctrl entry:
> > > > > > > -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > > > > > > -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > > > > > > -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > > > > > > -  cause the SDA line to be driven low to work around a hardware
> > > > > > > -  errata.
> > > > > > > -
> > > > > > > -Example:
> > > > > > > -
> > > > > > > -hdmi: hdmi@ff980000 {
> > > > > > > -	compatible = "rockchip,rk3288-dw-hdmi";
> > > > > > > -	reg = <0xff980000 0x20000>;
> > > > > > > -	reg-io-width = <4>;
> > > > > > > -	ddc-i2c-bus = <&i2c5>;
> > > > > > > -	rockchip,grf = <&grf>;
> > > > > > > -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > > > > > > -	clock-names = "iahb", "isfr";
> > > > > > > -	ports {
> > > > > > > -		hdmi_in: port {
> > > > > > > -			#address-cells = <1>;
> > > > > > > -			#size-cells = <0>;
> > > > > > > -			hdmi_in_vopb: endpoint@0 {
> > > > > > > -				reg = <0>;
> > > > > > > -				remote-endpoint = <&vopb_out_hdmi>;
> > > > > > > -			};
> > > > > > > -			hdmi_in_vopl: endpoint@1 {
> > > > > > > -				reg = <1>;
> > > > > > > -				remote-endpoint = <&vopl_out_hdmi>;
> > > > > > > -			};
> > > > > > > -		};
> > > > > > > -	};
> > > > > > > -};
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..8ff544ae0ac2
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > > > > > > @@ -0,0 +1,178 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Rockchip DWC HDMI TX Encoder
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Mark Yao <mark.yao@rock-chips.com>
> > > > > > > +
> > > > > > > +description: |
> > > > > > > +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > > > > > > +  with a companion PHY IP.
> > > > > > > +
> > > > > > > +allOf:
> > > > > > > +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    enum:
> > > > > > > +      - rockchip,rk3228-dw-hdmi
> > > > > > > +      - rockchip,rk3288-dw-hdmi
> > > > > > > +      - rockchip,rk3328-dw-hdmi
> > > > > > > +      - rockchip,rk3399-dw-hdmi
> > > > > > > +
> > > > > > > +  reg: true
> > > > > > > +
> > > > > > > +  reg-io-width:
> > > > > > > +    const: 4
> > > > > > > +
> > > > > > > +  clocks:
> > > > > > > +    minItems: 2
> > > > > > > +    maxItems: 5
> > > > > > > +    items:
> > > > > > > +      - description: The bus clock for either AHB and APB
> > > > > > > +      - description: The internal register configuration clock
> > > > > > > +      - description: The HDMI CEC controller main clock
> > > > > > > +      - description: Power for GRF IO
> > > > > > > +      - description: External clock for some HDMI PHY
> > > > > > > +
> > > > > > > +  clock-names:
> > > > > > > +    minItems: 2
> > > > > > > +    maxItems: 5
> > > > > > > +    items:
> > > > > > > +      - const: iahb
> > > > > > > +      - const: isfr
> > > > > > > +      - enum:
> > > > > > > +        - cec
> > > > > > > +        - grf
> > > > > > > +        - vpll
> > > > > > > +      - enum:
> > > > > > > +        - cec
> > > > > > > +        - grf
> > > > > > > +        - vpll
> > > > > > > +      - enum:
> > > > > > > +        - cec
> > > > > > > +        - grf
> > > > > > > +        - vpll
> > > > > >
> > > > > > IIRC Rob wanted us to standardize the order of the clocks if possible,
> > > > > > since it's a pain to support properly here, and your description won't
> > > > > > match what you describe here either (and in general it's just a best
> > > > > > practice), so if all your DTs have the same order you should just set
> > > > > > that order in stone.
> > > > >
> > > > > But how do we handle the case where any of the cec, grf and vpll clocks
> > > > > can be set ? Assuming, for instance, that
> > > > >
> > > > > 	clock-names = "iahb", "isfr", "cec";
> > > > > 	clock-names = "iahb", "isfr", "vpll";
> > > > > 	clock-names = "iahb", "isfr", "cec", "vpll";
> > > > >
> > > > > would all be valid.
> > > >
> > > > It would be painful then...
> > > >
> > > > The easiest way to do so would be to simply use an enum there, and not
> > > > bother checking the array at all. You'll get a warning if there's
> > > > multiple occurences of the same string, and I guess that's what you
> > > > would be really concerned about.
> > > >
> > > > However, now that I think about it, what's the interaction between the
> > > > generic binding and this one when it comes to the third clock? The
> > > > generic one expects it to be cec, and here you have other options?
> > >
> > > I'm not too familiar with the platform, but as far as I understand, any
> > > of the cec, grf and vpll clock is optional (if someone could confirm
> > > that, it would be useful). I don't care so much about the order, but
> > > iahb and isfr are mandatory, and thus need to be specified as two const
> > > items in the beginning as far as I understand. It would be nice to set
> > > something along the lines of
> > >
> > >   minItems: 2
> > >   maxItems: 5
> > >   items:
> > >     - const: iahb
> > >     - const: isfr
> > >     - enum:
> > >       - cec
> > >       - grf
> > >       - vpll
> > 
> > I guess you could do something like:
> > 
> > in the generic schema:
> > 
> > clock-names:
> >   allOf:
> >     - minItems: 2
> >     - enum:
> >        - iahb
> >        - isfr
> >        - cec
> >       additonalItems: true
> >     - items:
> >       - iahb
> >       - isfr
> > 
> > Or something along those lines, I haven't tested it, but the basic
> > idea is that you want to enforce that:
> >   a) there's a minimum of two clocks
> >   b) valid clock names are iahb, isfr and cec, but we will allow more
> >   c) the first two clocks are iahb and isfr
> 
> Interesting idea. I've tried
> 
>   clock-names:
>     allOf:
>       - minItems: 2
>       - enum:
>         - iahb
>         - isfr
>         - cec
>         additionalItems: true

Instead of these 2, you want:

items:
  enum: [ iahb, isfr, cec ]
minItems: 2
maxItems: 3

Though I assume there's some others missing as with this and below, the 
3rd clock can only be 'cec'.

>       - items:
>         - const: iahb
>         - const: isfr

And this needs 'maxItems: 3' added.

Rob
Laurent Pinchart April 15, 2020, 1:06 a.m. UTC | #10
Hi Rob,

On Tue, Apr 14, 2020 at 06:10:05PM -0500, Rob Herring wrote:
> On Wed, Apr 08, 2020 at 02:45:52PM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 07, 2020 at 09:12:51AM +0200, Maxime Ripard wrote:
> >> On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> >>> On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> >>>> On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> >>>>> On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> >>>>>> On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> >>>>>>> Convert the Rockchip HDMI TX text binding to YAML.
> >>>>>>>
> >>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>> ---
> >>>>>>>  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> >>>>>>>  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> >>>>>>>  2 files changed, 178 insertions(+), 74 deletions(-)
> >>>>>>>  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >>>>>>> deleted file mode 100644
> >>>>>>> index 3d32ce137e7f..000000000000
> >>>>>>> --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> >>>>>>> +++ /dev/null
> >>>>>>> @@ -1,74 +0,0 @@
> >>>>>>> -Rockchip DWC HDMI TX Encoder
> >>>>>>> -============================
> >>>>>>> -
> >>>>>>> -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> >>>>>>> -with a companion PHY IP.
> >>>>>>> -
> >>>>>>> -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> >>>>>>> -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> >>>>>>> -following device-specific properties.
> >>>>>>> -
> >>>>>>> -
> >>>>>>> -Required properties:
> >>>>>>> -
> >>>>>>> -- compatible: should be one of the following:
> >>>>>>> -		"rockchip,rk3228-dw-hdmi"
> >>>>>>> -		"rockchip,rk3288-dw-hdmi"
> >>>>>>> -		"rockchip,rk3328-dw-hdmi"
> >>>>>>> -		"rockchip,rk3399-dw-hdmi"
> >>>>>>> -- reg: See dw_hdmi.txt.
> >>>>>>> -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> >>>>>>> -- interrupts: HDMI interrupt number
> >>>>>>> -- clocks: See dw_hdmi.txt.
> >>>>>>> -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> >>>>>>> -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> >>>>>>> -  corresponding to the video input of the controller. The port shall have two
> >>>>>>> -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> >>>>>>> -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> >>>>>>> -
> >>>>>>> -Optional properties
> >>>>>>> -
> >>>>>>> -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> >>>>>>> -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> >>>>>>> -  connected to a system I2C master this property contains a phandle to that
> >>>>>>> -  I2C master controller.
> >>>>>>> -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> >>>>>>> -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> >>>>>>> -- clock-names: May contain "grf", power for grf io.
> >>>>>>> -- clock-names: May contain "vpll", external clock for some hdmi phy.
> >>>>>>> -- phys: from general PHY binding: the phandle for the PHY device.
> >>>>>>> -- phy-names: Should be "hdmi" if phys references an external phy.
> >>>>>>> -
> >>>>>>> -Optional pinctrl entry:
> >>>>>>> -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> >>>>>>> -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> >>>>>>> -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> >>>>>>> -  cause the SDA line to be driven low to work around a hardware
> >>>>>>> -  errata.
> >>>>>>> -
> >>>>>>> -Example:
> >>>>>>> -
> >>>>>>> -hdmi: hdmi@ff980000 {
> >>>>>>> -	compatible = "rockchip,rk3288-dw-hdmi";
> >>>>>>> -	reg = <0xff980000 0x20000>;
> >>>>>>> -	reg-io-width = <4>;
> >>>>>>> -	ddc-i2c-bus = <&i2c5>;
> >>>>>>> -	rockchip,grf = <&grf>;
> >>>>>>> -	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> >>>>>>> -	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> >>>>>>> -	clock-names = "iahb", "isfr";
> >>>>>>> -	ports {
> >>>>>>> -		hdmi_in: port {
> >>>>>>> -			#address-cells = <1>;
> >>>>>>> -			#size-cells = <0>;
> >>>>>>> -			hdmi_in_vopb: endpoint@0 {
> >>>>>>> -				reg = <0>;
> >>>>>>> -				remote-endpoint = <&vopb_out_hdmi>;
> >>>>>>> -			};
> >>>>>>> -			hdmi_in_vopl: endpoint@1 {
> >>>>>>> -				reg = <1>;
> >>>>>>> -				remote-endpoint = <&vopl_out_hdmi>;
> >>>>>>> -			};
> >>>>>>> -		};
> >>>>>>> -	};
> >>>>>>> -};
> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..8ff544ae0ac2
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> >>>>>>> @@ -0,0 +1,178 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Rockchip DWC HDMI TX Encoder
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Mark Yao <mark.yao@rock-chips.com>
> >>>>>>> +
> >>>>>>> +description: |
> >>>>>>> +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> >>>>>>> +  with a companion PHY IP.
> >>>>>>> +
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    enum:
> >>>>>>> +      - rockchip,rk3228-dw-hdmi
> >>>>>>> +      - rockchip,rk3288-dw-hdmi
> >>>>>>> +      - rockchip,rk3328-dw-hdmi
> >>>>>>> +      - rockchip,rk3399-dw-hdmi
> >>>>>>> +
> >>>>>>> +  reg: true
> >>>>>>> +
> >>>>>>> +  reg-io-width:
> >>>>>>> +    const: 4
> >>>>>>> +
> >>>>>>> +  clocks:
> >>>>>>> +    minItems: 2
> >>>>>>> +    maxItems: 5
> >>>>>>> +    items:
> >>>>>>> +      - description: The bus clock for either AHB and APB
> >>>>>>> +      - description: The internal register configuration clock
> >>>>>>> +      - description: The HDMI CEC controller main clock
> >>>>>>> +      - description: Power for GRF IO
> >>>>>>> +      - description: External clock for some HDMI PHY
> >>>>>>> +
> >>>>>>> +  clock-names:
> >>>>>>> +    minItems: 2
> >>>>>>> +    maxItems: 5
> >>>>>>> +    items:
> >>>>>>> +      - const: iahb
> >>>>>>> +      - const: isfr
> >>>>>>> +      - enum:
> >>>>>>> +        - cec
> >>>>>>> +        - grf
> >>>>>>> +        - vpll
> >>>>>>> +      - enum:
> >>>>>>> +        - cec
> >>>>>>> +        - grf
> >>>>>>> +        - vpll
> >>>>>>> +      - enum:
> >>>>>>> +        - cec
> >>>>>>> +        - grf
> >>>>>>> +        - vpll
> >>>>>>
> >>>>>> IIRC Rob wanted us to standardize the order of the clocks if possible,
> >>>>>> since it's a pain to support properly here, and your description won't
> >>>>>> match what you describe here either (and in general it's just a best
> >>>>>> practice), so if all your DTs have the same order you should just set
> >>>>>> that order in stone.
> >>>>>
> >>>>> But how do we handle the case where any of the cec, grf and vpll clocks
> >>>>> can be set ? Assuming, for instance, that
> >>>>>
> >>>>> 	clock-names = "iahb", "isfr", "cec";
> >>>>> 	clock-names = "iahb", "isfr", "vpll";
> >>>>> 	clock-names = "iahb", "isfr", "cec", "vpll";
> >>>>>
> >>>>> would all be valid.
> >>>>
> >>>> It would be painful then...
> >>>>
> >>>> The easiest way to do so would be to simply use an enum there, and not
> >>>> bother checking the array at all. You'll get a warning if there's
> >>>> multiple occurences of the same string, and I guess that's what you
> >>>> would be really concerned about.
> >>>>
> >>>> However, now that I think about it, what's the interaction between the
> >>>> generic binding and this one when it comes to the third clock? The
> >>>> generic one expects it to be cec, and here you have other options?
> >>>
> >>> I'm not too familiar with the platform, but as far as I understand, any
> >>> of the cec, grf and vpll clock is optional (if someone could confirm
> >>> that, it would be useful). I don't care so much about the order, but
> >>> iahb and isfr are mandatory, and thus need to be specified as two const
> >>> items in the beginning as far as I understand. It would be nice to set
> >>> something along the lines of
> >>>
> >>>   minItems: 2
> >>>   maxItems: 5
> >>>   items:
> >>>     - const: iahb
> >>>     - const: isfr
> >>>     - enum:
> >>>       - cec
> >>>       - grf
> >>>       - vpll
> >> 
> >> I guess you could do something like:
> >> 
> >> in the generic schema:
> >> 
> >> clock-names:
> >>   allOf:
> >>     - minItems: 2
> >>     - enum:
> >>        - iahb
> >>        - isfr
> >>        - cec
> >>       additonalItems: true
> >>     - items:
> >>       - iahb
> >>       - isfr
> >> 
> >> Or something along those lines, I haven't tested it, but the basic
> >> idea is that you want to enforce that:
> >>   a) there's a minimum of two clocks
> >>   b) valid clock names are iahb, isfr and cec, but we will allow more
> >>   c) the first two clocks are iahb and isfr
> > 
> > Interesting idea. I've tried
> > 
> >   clock-names:
> >     allOf:
> >       - minItems: 2
> >       - enum:
> >         - iahb
> >         - isfr
> >         - cec
> >         additionalItems: true
> 
> Instead of these 2, you want:
> 
> items:
>   enum: [ iahb, isfr, cec ]
> minItems: 2
> maxItems: 3
> 
> Though I assume there's some others missing as with this and below, the 
> 3rd clock can only be 'cec'.

But with this bindings that include this base schema and extend the
number of clocks will not validate :-( I'd like a way for the base
schema to provide an overridable check, but I don't think that's
possible. I then tried to specify a check that would be valid for
extensions too, and that would verify that

- There are at least two clocks and maybe more (that should be easy by
  setting minItems: 2 and leaving maxItems unspecified, but the
  dt-validate tool then adds a maxItems automatically :-()

- The first two clock names are 'iahb' and 'isfr', and additional clock
  names can be anything.

Maybe YAML schemas can't support this, and I need to specify all
constraints explicitly in derived bindings ? The base
bridge/synopsys,dw-hdmi.yaml file would then be dropped completely, with
its contents copied to derived bindings. Or maybe I should selectively
include rules from the base schema in rockchip,dw-hdmi.yaml ? That's not
very user-friendly though.

> >       - items:
> >         - const: iahb
> >         - const: isfr
> 
> And this needs 'maxItems: 3' added.
Rob Herring April 15, 2020, 1:50 p.m. UTC | #11
On Tue, Apr 14, 2020 at 8:06 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Tue, Apr 14, 2020 at 06:10:05PM -0500, Rob Herring wrote:
> > On Wed, Apr 08, 2020 at 02:45:52PM +0300, Laurent Pinchart wrote:
> > > On Tue, Apr 07, 2020 at 09:12:51AM +0200, Maxime Ripard wrote:
> > >> On Mon, Apr 06, 2020 at 08:50:28PM +0300, Laurent Pinchart wrote:
> > >>> On Mon, Apr 06, 2020 at 07:09:15PM +0200, Maxime Ripard wrote:
> > >>>> On Mon, Apr 06, 2020 at 02:19:27PM +0300, Laurent Pinchart wrote:
> > >>>>> On Mon, Apr 06, 2020 at 10:00:32AM +0200, Maxime Ripard wrote:
> > >>>>>> On Mon, Apr 06, 2020 at 02:39:33AM +0300, Laurent Pinchart wrote:
> > >>>>>>> Convert the Rockchip HDMI TX text binding to YAML.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >>>>>>> ---
> > >>>>>>>  .../display/rockchip/dw_hdmi-rockchip.txt     |  74 --------
> > >>>>>>>  .../display/rockchip/rockchip,dw-hdmi.yaml    | 178 ++++++++++++++++++
> > >>>>>>>  2 files changed, 178 insertions(+), 74 deletions(-)
> > >>>>>>>  delete mode 100644 Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > >>>>>>>  create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > >>>>>>>
> > >>>>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > >>>>>>> deleted file mode 100644
> > >>>>>>> index 3d32ce137e7f..000000000000
> > >>>>>>> --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> > >>>>>>> +++ /dev/null
> > >>>>>>> @@ -1,74 +0,0 @@
> > >>>>>>> -Rockchip DWC HDMI TX Encoder
> > >>>>>>> -============================
> > >>>>>>> -
> > >>>>>>> -The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > >>>>>>> -with a companion PHY IP.
> > >>>>>>> -
> > >>>>>>> -These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > >>>>>>> -Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > >>>>>>> -following device-specific properties.
> > >>>>>>> -
> > >>>>>>> -
> > >>>>>>> -Required properties:
> > >>>>>>> -
> > >>>>>>> -- compatible: should be one of the following:
> > >>>>>>> -             "rockchip,rk3228-dw-hdmi"
> > >>>>>>> -             "rockchip,rk3288-dw-hdmi"
> > >>>>>>> -             "rockchip,rk3328-dw-hdmi"
> > >>>>>>> -             "rockchip,rk3399-dw-hdmi"
> > >>>>>>> -- reg: See dw_hdmi.txt.
> > >>>>>>> -- reg-io-width: See dw_hdmi.txt. Shall be 4.
> > >>>>>>> -- interrupts: HDMI interrupt number
> > >>>>>>> -- clocks: See dw_hdmi.txt.
> > >>>>>>> -- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> > >>>>>>> -- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
> > >>>>>>> -  corresponding to the video input of the controller. The port shall have two
> > >>>>>>> -  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
> > >>>>>>> -- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
> > >>>>>>> -
> > >>>>>>> -Optional properties
> > >>>>>>> -
> > >>>>>>> -- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
> > >>>>>>> -  or the functionally-reduced I2C master contained in the DWC HDMI. When
> > >>>>>>> -  connected to a system I2C master this property contains a phandle to that
> > >>>>>>> -  I2C master controller.
> > >>>>>>> -- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
> > >>>>>>> -- clock-names: May contain "cec" as defined in dw_hdmi.txt.
> > >>>>>>> -- clock-names: May contain "grf", power for grf io.
> > >>>>>>> -- clock-names: May contain "vpll", external clock for some hdmi phy.
> > >>>>>>> -- phys: from general PHY binding: the phandle for the PHY device.
> > >>>>>>> -- phy-names: Should be "hdmi" if phys references an external phy.
> > >>>>>>> -
> > >>>>>>> -Optional pinctrl entry:
> > >>>>>>> -- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> > >>>>>>> -  will switch to the unwedge pinctrl state for 10ms if it ever gets an
> > >>>>>>> -  i2c timeout.  It's intended that this unwedge pinctrl entry will
> > >>>>>>> -  cause the SDA line to be driven low to work around a hardware
> > >>>>>>> -  errata.
> > >>>>>>> -
> > >>>>>>> -Example:
> > >>>>>>> -
> > >>>>>>> -hdmi: hdmi@ff980000 {
> > >>>>>>> -     compatible = "rockchip,rk3288-dw-hdmi";
> > >>>>>>> -     reg = <0xff980000 0x20000>;
> > >>>>>>> -     reg-io-width = <4>;
> > >>>>>>> -     ddc-i2c-bus = <&i2c5>;
> > >>>>>>> -     rockchip,grf = <&grf>;
> > >>>>>>> -     interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > >>>>>>> -     clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
> > >>>>>>> -     clock-names = "iahb", "isfr";
> > >>>>>>> -     ports {
> > >>>>>>> -             hdmi_in: port {
> > >>>>>>> -                     #address-cells = <1>;
> > >>>>>>> -                     #size-cells = <0>;
> > >>>>>>> -                     hdmi_in_vopb: endpoint@0 {
> > >>>>>>> -                             reg = <0>;
> > >>>>>>> -                             remote-endpoint = <&vopb_out_hdmi>;
> > >>>>>>> -                     };
> > >>>>>>> -                     hdmi_in_vopl: endpoint@1 {
> > >>>>>>> -                             reg = <1>;
> > >>>>>>> -                             remote-endpoint = <&vopl_out_hdmi>;
> > >>>>>>> -                     };
> > >>>>>>> -             };
> > >>>>>>> -     };
> > >>>>>>> -};
> > >>>>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > >>>>>>> new file mode 100644
> > >>>>>>> index 000000000000..8ff544ae0ac2
> > >>>>>>> --- /dev/null
> > >>>>>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > >>>>>>> @@ -0,0 +1,178 @@
> > >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>>>>>> +%YAML 1.2
> > >>>>>>> +---
> > >>>>>>> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
> > >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>>>>> +
> > >>>>>>> +title: Rockchip DWC HDMI TX Encoder
> > >>>>>>> +
> > >>>>>>> +maintainers:
> > >>>>>>> +  - Mark Yao <mark.yao@rock-chips.com>
> > >>>>>>> +
> > >>>>>>> +description: |
> > >>>>>>> +  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > >>>>>>> +  with a companion PHY IP.
> > >>>>>>> +
> > >>>>>>> +allOf:
> > >>>>>>> +  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
> > >>>>>>> +
> > >>>>>>> +properties:
> > >>>>>>> +  compatible:
> > >>>>>>> +    enum:
> > >>>>>>> +      - rockchip,rk3228-dw-hdmi
> > >>>>>>> +      - rockchip,rk3288-dw-hdmi
> > >>>>>>> +      - rockchip,rk3328-dw-hdmi
> > >>>>>>> +      - rockchip,rk3399-dw-hdmi
> > >>>>>>> +
> > >>>>>>> +  reg: true
> > >>>>>>> +
> > >>>>>>> +  reg-io-width:
> > >>>>>>> +    const: 4
> > >>>>>>> +
> > >>>>>>> +  clocks:
> > >>>>>>> +    minItems: 2
> > >>>>>>> +    maxItems: 5
> > >>>>>>> +    items:
> > >>>>>>> +      - description: The bus clock for either AHB and APB
> > >>>>>>> +      - description: The internal register configuration clock
> > >>>>>>> +      - description: The HDMI CEC controller main clock
> > >>>>>>> +      - description: Power for GRF IO
> > >>>>>>> +      - description: External clock for some HDMI PHY
> > >>>>>>> +
> > >>>>>>> +  clock-names:
> > >>>>>>> +    minItems: 2
> > >>>>>>> +    maxItems: 5
> > >>>>>>> +    items:
> > >>>>>>> +      - const: iahb
> > >>>>>>> +      - const: isfr
> > >>>>>>> +      - enum:
> > >>>>>>> +        - cec
> > >>>>>>> +        - grf
> > >>>>>>> +        - vpll
> > >>>>>>> +      - enum:
> > >>>>>>> +        - cec
> > >>>>>>> +        - grf
> > >>>>>>> +        - vpll
> > >>>>>>> +      - enum:
> > >>>>>>> +        - cec
> > >>>>>>> +        - grf
> > >>>>>>> +        - vpll
> > >>>>>>
> > >>>>>> IIRC Rob wanted us to standardize the order of the clocks if possible,
> > >>>>>> since it's a pain to support properly here, and your description won't
> > >>>>>> match what you describe here either (and in general it's just a best
> > >>>>>> practice), so if all your DTs have the same order you should just set
> > >>>>>> that order in stone.
> > >>>>>
> > >>>>> But how do we handle the case where any of the cec, grf and vpll clocks
> > >>>>> can be set ? Assuming, for instance, that
> > >>>>>
> > >>>>>         clock-names = "iahb", "isfr", "cec";
> > >>>>>         clock-names = "iahb", "isfr", "vpll";
> > >>>>>         clock-names = "iahb", "isfr", "cec", "vpll";
> > >>>>>
> > >>>>> would all be valid.
> > >>>>
> > >>>> It would be painful then...
> > >>>>
> > >>>> The easiest way to do so would be to simply use an enum there, and not
> > >>>> bother checking the array at all. You'll get a warning if there's
> > >>>> multiple occurences of the same string, and I guess that's what you
> > >>>> would be really concerned about.
> > >>>>
> > >>>> However, now that I think about it, what's the interaction between the
> > >>>> generic binding and this one when it comes to the third clock? The
> > >>>> generic one expects it to be cec, and here you have other options?
> > >>>
> > >>> I'm not too familiar with the platform, but as far as I understand, any
> > >>> of the cec, grf and vpll clock is optional (if someone could confirm
> > >>> that, it would be useful). I don't care so much about the order, but
> > >>> iahb and isfr are mandatory, and thus need to be specified as two const
> > >>> items in the beginning as far as I understand. It would be nice to set
> > >>> something along the lines of
> > >>>
> > >>>   minItems: 2
> > >>>   maxItems: 5
> > >>>   items:
> > >>>     - const: iahb
> > >>>     - const: isfr
> > >>>     - enum:
> > >>>       - cec
> > >>>       - grf
> > >>>       - vpll
> > >>
> > >> I guess you could do something like:
> > >>
> > >> in the generic schema:
> > >>
> > >> clock-names:
> > >>   allOf:
> > >>     - minItems: 2
> > >>     - enum:
> > >>        - iahb
> > >>        - isfr
> > >>        - cec
> > >>       additonalItems: true
> > >>     - items:
> > >>       - iahb
> > >>       - isfr
> > >>
> > >> Or something along those lines, I haven't tested it, but the basic
> > >> idea is that you want to enforce that:
> > >>   a) there's a minimum of two clocks
> > >>   b) valid clock names are iahb, isfr and cec, but we will allow more
> > >>   c) the first two clocks are iahb and isfr
> > >
> > > Interesting idea. I've tried
> > >
> > >   clock-names:
> > >     allOf:
> > >       - minItems: 2
> > >       - enum:
> > >         - iahb
> > >         - isfr
> > >         - cec
> > >         additionalItems: true
> >
> > Instead of these 2, you want:
> >
> > items:
> >   enum: [ iahb, isfr, cec ]
> > minItems: 2
> > maxItems: 3
> >
> > Though I assume there's some others missing as with this and below, the
> > 3rd clock can only be 'cec'.
>
> But with this bindings that include this base schema and extend the
> number of clocks will not validate :-( I'd like a way for the base
> schema to provide an overridable check, but I don't think that's
> possible. I then tried to specify a check that would be valid for
> extensions too, and that would verify that

But this is not the base schema?

> - There are at least two clocks and maybe more (that should be easy by
>   setting minItems: 2 and leaving maxItems unspecified, but the
>   dt-validate tool then adds a maxItems automatically :-()

That's to avoid boilerplate in the vast majority of cases.

Can't you just do 'maxItems: 1000' or some sane limit?

>
> - The first two clock names are 'iahb' and 'isfr', and additional clock
>   names can be anything.

So, in the base schema:

items:
  - const: iahb
  - const: isfr
minItems: 2
maxItems: 10
additionalItems: true

In the device schema you can do something like:

items:
  - {}
  - {}
  - enum: [ a, b, c ]
  - enum: [ a, b, c ]

If there's no order of the last items. Or you can use the schema
flavor (rather than list) of 'items' and list iahb and isfr again.

> Maybe YAML schemas can't support this, and I need to specify all
> constraints explicitly in derived bindings ? The base
> bridge/synopsys,dw-hdmi.yaml file would then be dropped completely, with
> its contents copied to derived bindings. Or maybe I should selectively
> include rules from the base schema in rockchip,dw-hdmi.yaml ? That's not
> very user-friendly though.

You could get into using definitions and referencing those
individually, but I wouldn't recommend it.

My hope is in new bindings the difficulty in expressing all the schema
variations will prevent some of this needless variation. The reality
is for the same IP block, the clocks and other resources shouldn't
vary so much. :)

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
deleted file mode 100644
index 3d32ce137e7f..000000000000
--- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
+++ /dev/null
@@ -1,74 +0,0 @@ 
-Rockchip DWC HDMI TX Encoder
-============================
-
-The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
-with a companion PHY IP.
-
-These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
-Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
-following device-specific properties.
-
-
-Required properties:
-
-- compatible: should be one of the following:
-		"rockchip,rk3228-dw-hdmi"
-		"rockchip,rk3288-dw-hdmi"
-		"rockchip,rk3328-dw-hdmi"
-		"rockchip,rk3399-dw-hdmi"
-- reg: See dw_hdmi.txt.
-- reg-io-width: See dw_hdmi.txt. Shall be 4.
-- interrupts: HDMI interrupt number
-- clocks: See dw_hdmi.txt.
-- clock-names: Shall contain "iahb" and "isfr" as defined in dw_hdmi.txt.
-- ports: See dw_hdmi.txt. The DWC HDMI shall have a single port numbered 0
-  corresponding to the video input of the controller. The port shall have two
-  endpoints, numbered 0 and 1, connected respectively to the vopb and vopl.
-- rockchip,grf: Shall reference the GRF to mux vopl/vopb.
-
-Optional properties
-
-- ddc-i2c-bus: The HDMI DDC bus can be connected to either a system I2C master
-  or the functionally-reduced I2C master contained in the DWC HDMI. When
-  connected to a system I2C master this property contains a phandle to that
-  I2C master controller.
-- clock-names: See dw_hdmi.txt. The "cec" clock is optional.
-- clock-names: May contain "cec" as defined in dw_hdmi.txt.
-- clock-names: May contain "grf", power for grf io.
-- clock-names: May contain "vpll", external clock for some hdmi phy.
-- phys: from general PHY binding: the phandle for the PHY device.
-- phy-names: Should be "hdmi" if phys references an external phy.
-
-Optional pinctrl entry:
-- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
-  will switch to the unwedge pinctrl state for 10ms if it ever gets an
-  i2c timeout.  It's intended that this unwedge pinctrl entry will
-  cause the SDA line to be driven low to work around a hardware
-  errata.
-
-Example:
-
-hdmi: hdmi@ff980000 {
-	compatible = "rockchip,rk3288-dw-hdmi";
-	reg = <0xff980000 0x20000>;
-	reg-io-width = <4>;
-	ddc-i2c-bus = <&i2c5>;
-	rockchip,grf = <&grf>;
-	interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
-	clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
-	clock-names = "iahb", "isfr";
-	ports {
-		hdmi_in: port {
-			#address-cells = <1>;
-			#size-cells = <0>;
-			hdmi_in_vopb: endpoint@0 {
-				reg = <0>;
-				remote-endpoint = <&vopb_out_hdmi>;
-			};
-			hdmi_in_vopl: endpoint@1 {
-				reg = <1>;
-				remote-endpoint = <&vopl_out_hdmi>;
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
new file mode 100644
index 000000000000..8ff544ae0ac2
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
@@ -0,0 +1,178 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip DWC HDMI TX Encoder
+
+maintainers:
+  - Mark Yao <mark.yao@rock-chips.com>
+
+description: |
+  The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
+  with a companion PHY IP.
+
+allOf:
+  - $ref: ../bridge/synopsys,dw-hdmi.yaml#
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3228-dw-hdmi
+      - rockchip,rk3288-dw-hdmi
+      - rockchip,rk3328-dw-hdmi
+      - rockchip,rk3399-dw-hdmi
+
+  reg: true
+
+  reg-io-width:
+    const: 4
+
+  clocks:
+    minItems: 2
+    maxItems: 5
+    items:
+      - description: The bus clock for either AHB and APB
+      - description: The internal register configuration clock
+      - description: The HDMI CEC controller main clock
+      - description: Power for GRF IO
+      - description: External clock for some HDMI PHY
+
+  clock-names:
+    minItems: 2
+    maxItems: 5
+    items:
+      - const: iahb
+      - const: isfr
+      - enum:
+        - cec
+        - grf
+        - vpll
+      - enum:
+        - cec
+        - grf
+        - vpll
+      - enum:
+        - cec
+        - grf
+        - vpll
+
+  ddc-i2c-bus:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      The HDMI DDC bus can be connected to either a system I2C master or the
+      functionally-reduced I2C master contained in the DWC HDMI. When connected
+      to a system I2C master this property contains a phandle to that I2C
+      master controller.
+
+  interrupts: true
+
+  phys:
+    maxItems: 1
+    description: The HDMI PHY
+
+  phy-names:
+    const: hdmi
+
+  pinctrl-0: true
+  pinctrl-1: true
+
+  pinctrl-names:
+    description:
+      The unwedge pinctrl entry shall drive the DDC SDA line low. This is
+      intended to work around a hardware errata that can cause the DDC I2C
+      bus to be wedged.
+    items:
+      - const: default
+      - const: unwedge
+
+  ports:
+    type: object
+    description: |
+      This device has one video port, corresponding to the input of the DWC
+      HDMI TX. Its connections are modelled using the OF graph bindings
+      specified in Documentation/devicetree/bindings/graph.txt.
+
+    properties:
+      port:
+        type: object
+        description: Input of the DWC HDMI TX
+
+        properties:
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+          endpoint@0:
+            type: object
+            description: Connection to the VOPB
+
+          endpoint@1:
+            type: object
+            description: Connection to the VOPL
+
+        required:
+          - endpoint@0
+          - endpoint@1
+
+        additionalProperties: false
+
+    required:
+      - port
+
+    additionalProperties: false
+
+  rockchip,grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to the GRF to mux vopl/vopb.
+
+required:
+  - compatible
+  - reg
+  - reg-io-width
+  - clocks
+  - clock-names
+  - interrupts
+  - ports
+  - rockchip,grf
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3288-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    hdmi: hdmi@ff980000 {
+        compatible = "rockchip,rk3288-dw-hdmi";
+        reg = <0xff980000 0x20000>;
+        reg-io-width = <4>;
+        ddc-i2c-bus = <&i2c5>;
+        rockchip,grf = <&grf>;
+        interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&cru  PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_HDCP>;
+        clock-names = "iahb", "isfr";
+
+        ports {
+            port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                hdmi_in_vopb: endpoint@0 {
+                    reg = <0>;
+                    remote-endpoint = <&vopb_out_hdmi>;
+                };
+                hdmi_in_vopl: endpoint@1 {
+                    reg = <1>;
+                    remote-endpoint = <&vopl_out_hdmi>;
+                };
+            };
+        };
+    };
+
+...