Re: [PATCH v2 6/6] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ricardo,

Thank you for the patch.

On Mon, May 11, 2020 at 01:06:11PM +0200, Ricardo Cañuelo wrote:
> Convert the ADV7511/11w/13/33/35 DT bindings to json-schema. The
> original binding has been split into two files: adi,adv7511.yaml for
> ADV7511/11W/13 and adi,adv7533.yaml for ADV7533/35.
> 
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@xxxxxxxxxxxxx>
> ---
>  .../bindings/display/bridge/adi,adv7511.txt   | 143 -----------
>  .../bindings/display/bridge/adi,adv7511.yaml  | 230 ++++++++++++++++++
>  .../bindings/display/bridge/adi,adv7533.yaml  | 166 +++++++++++++
>  3 files changed, 396 insertions(+), 143 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> deleted file mode 100644
> index 659523f538bf..000000000000
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ /dev/null
> @@ -1,143 +0,0 @@
> -Analog Devices ADV7511(W)/13/33/35 HDMI Encoders
> -------------------------------------------------
> -
> -The ADV7511, ADV7511W, ADV7513, ADV7533 and ADV7535 are HDMI audio and video
> -transmitters compatible with HDMI 1.4 and DVI 1.0. They support color space
> -conversion, S/PDIF, CEC and HDCP. ADV7533/5 supports the DSI interface for input
> -pixels, while the others support RGB interface.
> -
> -Required properties:
> -
> -- compatible: Should be one of:
> -		"adi,adv7511"
> -		"adi,adv7511w"
> -		"adi,adv7513"
> -		"adi,adv7533"
> -		"adi,adv7535"
> -
> -- reg: I2C slave addresses
> -  The ADV7511 internal registers are split into four pages exposed through
> -  different I2C addresses, creating four register maps. Each map has it own
> -  I2C address and acts as a standard slave device on the I2C bus. The main
> -  address is mandatory, others are optional and revert to defaults if not
> -  specified.
> -
> -
> -The ADV7511 supports a large number of input data formats that differ by their
> -color depth, color format, clock mode, bit justification and random
> -arrangement of components on the data bus. The combination of the following
> -properties describe the input and map directly to the video input tables of the
> -ADV7511 datasheet that document all the supported combinations.
> -
> -- adi,input-depth: Number of bits per color component at the input (8, 10 or
> -  12).
> -- adi,input-colorspace: The input color space, one of "rgb", "yuv422" or
> -  "yuv444".
> -- adi,input-clock: The input clock type, one of "1x" (one clock cycle per
> -  pixel), "2x" (two clock cycles per pixel), "ddr" (one clock cycle per pixel,
> -  data driven on both edges).
> -
> -The following input format properties are required except in "rgb 1x" and
> -"yuv444 1x" modes, in which case they must not be specified.
> -
> -- adi,input-style: The input components arrangement variant (1, 2 or 3), as
> -  listed in the input format tables in the datasheet.
> -- adi,input-justification: The input bit justification ("left", "evenly",
> -  "right").
> -
> -- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
> -- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
> -- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
> -- dvdd-3v-supply: A 3.3V supply that powers up the pin called DVDD_3V
> -  on the chip.
> -- bgvdd-supply: A 1.8V supply that powers up the BGVDD pin. This is
> -  needed only for ADV7511.
> -
> -The following properties are required for ADV7533 and ADV7535:
> -
> -- adi,dsi-lanes: Number of DSI data lanes connected to the DSI host. It should
> -  be one of 1, 2, 3 or 4.
> -- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
> -- v3p3-supply: A 3.3V supply that powers up the V3P3 pin on the chip.
> -- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
> -  either 1.2V or 1.8V for ADV7533 but only 1.8V for ADV7535.
> -
> -Optional properties:
> -
> -- interrupts: Specifier for the ADV7511 interrupt
> -- pd-gpios: Specifier for the GPIO connected to the power down signal
> -
> -- adi,clock-delay: Video data clock delay relative to the pixel clock, in ps
> -  (-1200 ps .. 1600 ps). Defaults to no delay.
> -- adi,embedded-sync: The input uses synchronization signals embedded in the
> -  data stream (similar to BT.656). Defaults to separate H/V synchronization
> -  signals.
> -- adi,disable-timing-generator: Only for ADV7533 and ADV7535. Disables the
> -  internal timing generator. The chip will rely on the sync signals in the
> -  DSI data lanes, rather than generate its own timings for HDMI output.
> -- clocks: from common clock binding: reference to the CEC clock.
> -- clock-names: from common clock binding: must be "cec".
> -- reg-names : Names of maps with programmable addresses.
> -	It can contain any map needing a non-default address.
> -	Possible maps names are : "main", "edid", "cec", "packet"
> -
> -Required nodes:
> -
> -The ADV7511 has two video ports. Their connections are modelled using the OF
> -graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> -
> -- Video port 0 for the RGB, YUV or DSI input. In the case of ADV7533/5, the
> -  remote endpoint phandle should be a reference to a valid mipi_dsi_host device
> -  node.
> -- Video port 1 for the HDMI output
> -- Audio port 2 for the HDMI audio input
> -
> -
> -Example
> --------
> -
> -	adv7511w: hdmi@39 {
> -		compatible = "adi,adv7511w";
> -		/*
> -		 * The EDID page will be accessible on address 0x66 on the I2C
> -		 * bus. All other maps continue to use their default addresses.
> -		 */
> -		reg = <0x39>, <0x66>;
> -		reg-names = "main", "edid";
> -		interrupt-parent = <&gpio3>;
> -		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> -		clocks = <&cec_clock>;
> -		clock-names = "cec";
> -
> -		adi,input-depth = <8>;
> -		adi,input-colorspace = "rgb";
> -		adi,input-clock = "1x";
> -		adi,input-style = <1>;
> -		adi,input-justification = "evenly";
> -
> -		ports {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -
> -			port@0 {
> -				reg = <0>;
> -				adv7511w_in: endpoint {
> -					remote-endpoint = <&dpi_out>;
> -				};
> -			};
> -
> -			port@1 {
> -				reg = <1>;
> -				adv7511_out: endpoint {
> -					remote-endpoint = <&hdmi_connector_in>;
> -				};
> -			};
> -
> -			port@2 {
> -				reg = <2>;
> -				codec_endpoint: endpoint {
> -					remote-endpoint = <&i2s0_cpu_endpoint>;
> -				};
> -			};
> -		};
> -	};
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml
> new file mode 100644
> index 000000000000..a306adba105f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml
> @@ -0,0 +1,233 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/adi,adv7511.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADV7511/11W/13 HDMI Encoders
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> +
> +description: |
> +  The ADV7511, ADV7511W and ADV7513 are HDMI audio and video
> +  transmitters compatible with HDMI 1.4 and DVI 1.0. They support color
> +  space conversion, S/PDIF, CEC and HDCP. They support RGB input
> +  interface.

I would write the last sentence as "The transmitter input is parallel
RGB or YUV data." as YUV is also supported.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adv7511
> +      - adi,adv7511w
> +      - adi,adv7513
> +
> +  reg:
> +    description: |
> +      I2C slave addresses.
> +
> +      The ADV7511/11W/13 internal registers are split into four pages
> +      exposed through different I2C addresses, creating four register
> +      maps. Each map has it own I2C address and acts as a standard slave
> +      device on the I2C bus. The main address is mandatory, others are
> +      optional and revert to defaults if not specified.
> +    minItems: 1
> +    maxItems: 4
> +
> +  reg-names:
> +    description:
> +      Names of maps with programmable addresses. It can contain any map
> +      needing a non-default address.
> +    minItems: 1
> +    items:
> +      - const: main
> +      - const: edid
> +      - const: cec
> +      - const: packet
> +
> +  clocks:
> +    description: Reference to the CEC clock.
> +    maxItems: 1
> +
> +  clock-names:
> +    const: cec
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  pd-gpios:
> +    description: GPIO connected to the power down signal.
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: A 1.8V supply that powers up the AVDD pin.
> +
> +  dvdd-supply:
> +    description: A 1.8V supply that powers up the DVDD pin.
> +
> +  pvdd-supply:
> +    description: A 1.8V supply that powers up the PVDD pin.
> +
> +  dvdd-3v-supply:
> +    description: A 3.3V supply that powers up the DVDD_3V pin.
> +
> +  bgvdd-supply:
> +    description: A 1.8V supply that powers up the BGVDD pin.
> +
> +  adi,input-depth:
> +    description: Number of bits per color component at the input.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [ 8, 10, 12 ]
> +
> +  adi,input-colorspace:
> +    description: Input color space.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/string
> +      - enum: [ rgb, yuv422, yuv444 ]

Isn't string implied ? Can't you write

  adi,input-colorspace:
    description: Input color space.
    enum: [ rgb, yuv422, yuv444 ]

Same for the other properties below.

> +
> +  adi,input-clock:
> +    description: |
> +      Input clock type.
> +        "1x": one clock cycle per pixel
> +        "2x": two clock cycles per pixel
> +        "dd": one clock cycle per pixel, data driven on both edges
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/string
> +      - enum: [ 1x, 2x, dd ]
> +
> +  adi,clock-delay:
> +    description:
> +      Video data clock delay relative to the pixel clock, in ps
> +      (-1200ps .. 1600 ps).
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - default: 0
> +
> +  adi,embedded-sync:
> +    description:
> +      The input uses synchronization signals embedded in the data
> +      stream (similar to BT.656). Defaults to 0 (separate H/V
> +      synchronization signals).
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [ 0, 1 ]
> +      - default: 0

This be a boolean property (it is read as a bool by the driver, the
property being absent means false, the property being present means
true).

> +
> +  adi,input-style:
> +    description:
> +      Input components arrangement variant as listed in the input
> +      format tables in the datasheet.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [ 1, 2, 3 ]
> +
> +  adi,input-justification:
> +    description: Input bit justification.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/string
> +      - enum: [ left, evenly, right ]
> +
> +  ports:
> +    description:
> +      The ADV7511(W)/13 has two video ports and one audio port. This node
> +      models their connections as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +      Documentation/devicetree/bindings/graph.txt
> +    type: object
> +    properties:
> +      port@0:
> +        description: Video port for the RGB, YUV or DSI input.

s/RGB, YUV or DSI/RGB or YUV/

> +        type: object
> +
> +      port@1:
> +        description: Video port for the HDMI output.
> +        type: object
> +
> +      port@2:
> +        description: Audio port for the HDMI output.
> +        type: object
> +
> +# adi,input-colorspace and adi,input-clock are required except in
> +# "rgb 1x" and "yuv444 1x" modes, in which case they must not be
> +# specified.
> +if:
> +  not:
> +    properties:
> +      adi,input-colorspace:
> +        contains:
> +          enum: [ rgb, yuv444 ]
> +      adi,input-clock:
> +        contains:
> +          const: 1x

As both properties take a single value, I think you can omit
"contains:".

> +
> +then:
> +  required:
> +    - adi,input-style
> +    - adi,input-justification
> +
> +else:
> +  properties:
> +    adi,input-style: false
> +    adi,input-justification: false
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - ports
> +  - adi,input-depth
> +  - adi,input-colorspace
> +  - adi,input-clock

Shouldn't the power supplies be required ?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    adv7511w: hdmi@39 {
> +        compatible = "adi,adv7511w";
> +        /*
> +         * The EDID page will be accessible on address 0x66 on the I2C
> +         * bus. All other maps continue to use their default addresses.
> +         */
> +        reg = <0x39>, <0x66>;
> +        reg-names = "main", "edid";
> +        interrupt-parent = <&gpio3>;
> +        interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> +        clocks = <&cec_clock>;
> +        clock-names = "cec";
> +
> +        adi,input-depth = <8>;
> +        adi,input-colorspace = "yuv422";
> +        adi,input-clock = "1x";
> +
> +        adi,input-style = <3>;
> +        adi,input-justification = "right";
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                adv7511w_in: endpoint {
> +                    remote-endpoint = <&dpi_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                adv7511_out: endpoint {
> +                    remote-endpoint = <&hdmi_connector_in>;
> +                };
> +            };
> +
> +            port@2 {
> +                reg = <2>;
> +                codec_endpoint: endpoint {
> +                    remote-endpoint = <&i2s0_cpu_endpoint>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml
> new file mode 100644
> index 000000000000..dfcc63dfc5c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml
> @@ -0,0 +1,166 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/adi,adv7533.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADV7533/35 HDMI Encoders
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> +
> +description: |
> +  The ADV7533 and ADV7535 are HDMI audio and video transmitters
> +  compatible with HDMI 1.4 and DVI 1.0. They support color space
> +  conversion, S/PDIF, CEC and HDCP. They support DSI for input pixels.

I would write the last sentence as "The transmitter input is MIPI DSI.".

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adv7533
> +      - adi,adv7535
> +
> +  reg:
> +    description: |
> +      I2C slave addresses.
> +
> +      The ADV7533/35 internal registers are split into four pages
> +      exposed through different I2C addresses, creating four register
> +      maps. Each map has it own I2C address and acts as a standard slave
> +      device on the I2C bus. The main address is mandatory, others are
> +      optional and revert to defaults if not specified.
> +    minItems: 1
> +    maxItems: 4
> +
> +  reg-names:
> +    description:
> +      Names of maps with programmable addresses. It can contain any map
> +      needing a non-default address.
> +    minItems: 1
> +    items:
> +      - const: main
> +      - const: edid
> +      - const: cec
> +      - const: packet
> +
> +  clocks:
> +    description: Reference to the CEC clock.
> +    maxItems: 1
> +
> +  clock-names:
> +    const: cec
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  pd-gpios:
> +    description: GPIO connected to the power down signal.
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: A 1.8V supply that powers up the AVDD pin.
> +
> +  dvdd-supply:
> +    description: A 1.8V supply that powers up the DVDD pin.
> +
> +  pvdd-supply:
> +    description: A 1.8V supply that powers up the PVDD pin.
> +
> +  a2vdd-supply:
> +    description: A 1.8V supply that powers up the A2VDD pin.
> +
> +  v3p3-supply:
> +    description: A 3.3V supply that powers up the V3P3 pin.
> +
> +  v1p2-supply:
> +    description:
> +      A supply that powers up the V1P2 pin. It can be either 1.2V
> +      or 1.8V for ADV7533 but only 1.8V for ADV7535.
> +
> +  adi,disable-timing-generator:
> +    description:
> +      Disables the interal timing generator. The chip will rely on the

s/interal/internal/

> +      sync signals in the DSI data lanes, rather than generate its own

s/generate/generating/

> +      timings for HDMI output.
> +    type: boolean
> +
> +  adi,dsi-lanes:
> +    description: Number of DSI data lanes connected to the DSI host.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [ 1, 2, 3, 4 ]
> +
> +  ports:
> +    description:
> +      The ADV7533/35 has two video ports and one audio port. This node
> +      models their connections as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +      Documentation/devicetree/bindings/graph.txt
> +    type: object
> +    properties:
> +      port@0:
> +        description:
> +          Video port for the RGB, YUV or DSI input. The remote endpoint

s/RGB, YUV or //

> +          phandle should be a reference to a valid mipi_dsi_host_device.
> +        type: object
> +
> +      port@1:
> +        description: Video port for the HDMI output.
> +        type: object
> +
> +      port@2:
> +        description: Audio port for the HDMI output.
> +        type: object
> +
> +required:
> +  - compatible
> +  - reg
> +  - ports
> +  - adi,dsi-lanes

Shouldn't the power supplies be required ?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    adv7533: hdmi@39 {
> +        compatible = "adi,adv7533";
> +        /*
> +         * The EDID page will be accessible on address 0x66 on the I2C
> +         * bus. All other maps continue to use their default addresses.
> +         */
> +        reg = <0x39>, <0x66>;
> +        reg-names = "main", "edid";
> +        interrupt-parent = <&gpio3>;
> +        interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> +        clocks = <&cec_clock>;
> +        clock-names = "cec";
> +        adi,dsi-lanes = <4>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                adv7511w_in: endpoint {
> +                    remote-endpoint = <&dpi_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                adv7511_out: endpoint {
> +                    remote-endpoint = <&hdmi_connector_in>;
> +                };
> +            };

The name of the two endpoints doesn't match the adv7533. The remote
endpoint phandle for port 0 should have dsi in its name.

> +
> +            port@2 {
> +                reg = <2>;
> +                codec_endpoint: endpoint {
> +                    remote-endpoint = <&i2s0_cpu_endpoint>;
> +                };
> +            };
> +        };
> +    };
> +
> +...

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux