Message-ID:
 <TY3PR01MB11346863C2ED6BE08C7F0746C86442@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Mon, 14 Oct 2024 08:30:30 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Liu Ying <victor.liu@....com>, Dmitry Baryshkov
	<dmitry.baryshkov@...aro.org>
CC: "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "andrzej.hajda@...el.com"
	<andrzej.hajda@...el.com>, "neil.armstrong@...aro.org"
	<neil.armstrong@...aro.org>, "rfoss@...nel.org" <rfoss@...nel.org>,
	laurent.pinchart <laurent.pinchart@...asonboard.com>, "jonas@...boo.se"
	<jonas@...boo.se>, "jernej.skrabec@...il.com" <jernej.skrabec@...il.com>,
	"airlied@...il.com" <airlied@...il.com>, "simona@...ll.ch" <simona@...ll.ch>,
	"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
	"mripard@...nel.org" <mripard@...nel.org>, "tzimmermann@...e.de"
	<tzimmermann@...e.de>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "shawnguo@...nel.org" <shawnguo@...nel.org>,
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>, "kernel@...gutronix.de"
	<kernel@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>,
	"catalin.marinas@....com" <catalin.marinas@....com>, "will@...nel.org"
	<will@...nel.org>, "quic_bjorande@...cinc.com" <quic_bjorande@...cinc.com>,
	"geert+renesas@...der.be" <geert+renesas@...der.be>, "arnd@...db.de"
	<arnd@...db.de>, "nfraprado@...labora.com" <nfraprado@...labora.com>,
	"o.rempel@...gutronix.de" <o.rempel@...gutronix.de>, "y.moog@...tec.de"
	<y.moog@...tec.de>, "marex@...x.de" <marex@...x.de>,
	"isaac.scott@...asonboard.com" <isaac.scott@...asonboard.com>
Subject: RE: [PATCH v2 5/9] dt-bindings: display: bridge: Add ITE IT6263 LVDS
 to HDMI converter

Hi Liu,

> -----Original Message-----
> From: Liu Ying <victor.liu@....com>
> Sent: Monday, October 14, 2024 6:34 AM
> Subject: Re: [PATCH v2 5/9] dt-bindings: display: bridge: Add ITE IT6263 LVDS to HDMI converter
> 
> On 10/14/2024, Dmitry Baryshkov wrote:
> > On Sat, Oct 12, 2024 at 05:14:13PM +0800, Liu Ying wrote:
> >> On 10/12/2024, Dmitry Baryshkov wrote:
> >>> On Sat, Oct 12, 2024 at 03:35:39PM +0800, Liu Ying wrote:
> >>>> Document ITE IT6263 LVDS to HDMI converter.
> >>>>
> >>>> Product link:
> >>>> https://www.ite.com.tw/en/product/cate1/IT6263
> >>>>
> >>>> Signed-off-by: Liu Ying <victor.liu@....com>
> >>>> ---
> >>>> v2:
> >>>> * Document number of LVDS link data lanes.  (Biju)
> >>>> * Simplify ports property by dropping "oneOf".  (Rob)
> >>>>
> >>>>  .../bindings/display/bridge/ite,it6263.yaml   | 276 ++++++++++++++++++
> >>>>  1 file changed, 276 insertions(+)
> >>>>  create mode 100644
> >>>> Documentation/devicetree/bindings/display/bridge/ite,it6263.yaml
> >>>>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/display/bridge/ite,it6263.yaml
> >>>> b/Documentation/devicetree/bindings/display/bridge/ite,it6263.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..bc2bbec07623
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6263.y
> >>>> +++ aml
> >>>> @@ -0,0 +1,276 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
> >>>> +1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/display/bridge/ite,it6263.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: ITE IT6263 LVDS to HDMI converter
> >>>> +
> >>>> +maintainers:
> >>>> +  - Liu Ying <victor.liu@....com>
> >>>> +
> >>>> +description: |
> >>>> +  The IT6263 is a high-performance single-chip De-SSC(De-Spread
> >>>> +Spectrum) LVDS
> >>>> +  to HDMI converter.  Combined with LVDS receiver and HDMI 1.4a
> >>>> +transmitter,
> >>>> +  the IT6263 supports LVDS input and HDMI 1.4 output by conversion function.
> >>>> +  The built-in LVDS receiver can support single-link and dual-link
> >>>> +LVDS inputs,
> >>>> +  and the built-in HDMI transmitter is fully compliant with HDMI
> >>>> +1.4a/3D, HDCP
> >>>> +  1.2 and backward compatible with DVI 1.0 specification.
> >>>> +
> >>>> +  The IT6263 also encodes and transmits up to 8 channels of I2S
> >>>> + digital audio,  with sampling rate up to 192KHz and sample size
> >>>> + up to 24 bits. In addition,  an S/PDIF input port takes in compressed audio of up to 192KHz
> frame rate.
> >>>> +
> >>>> +  The newly supported High-Bit Rate(HBR) audio by HDMI
> >>>> + specifications v1.3 is  provided by the IT6263 in two interfaces:
> >>>> + the four I2S input ports or the  S/PDIF input port.  With both
> >>>> + interfaces the highest possible HBR frame rate  is supported at up to 768KHz.
> >>>> +
> >>>> +properties:
> >>>
> >>> No LVDS data-mapping support?
> >>
> >> It is enough to document number of LVDS link data lanes because OS
> >> should be able to determine the data-mapping by looking at the number
> >> and the data-mapping capability of the other side of the LVDS link.
> >
> > From what I can see, data-mapping is specified on the consumer sink
> > side of the LVDS link. This means it should go to the bridge's device node.
> 
> Then, I won't define data-lanes, because data-mapping implies it, e.g., jeida-24 implies data lanes
> 0/1/2/3, see lvds-data-mapping.yaml.
> 
> Please let me know which one you prefer.

For consistency, maybe use data-mapping. See [1]

[1]
https://elixir.bootlin.com/linux/v6.0-rc4/source/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml#L26

Cheers,
Biju

> 
> >
> >>
> >>>
> >>>> +  compatible:
> >>>> +    const: ite,it6263
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  clocks:
> >>>> +    maxItems: 1
> >>>> +    description: audio master clock
> >>>> +
> >>>> +  clock-names:
> >>>> +    const: mclk
> >>>> +
> >>>> +  reset-gpios:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  ivdd-supply:
> >>>> +    description: 1.8V digital logic power
> >>>> +
> >>>> +  ovdd-supply:
> >>>> +    description: 3.3V I/O pin power
> >>>> +
> >>>> +  txavcc18-supply:
> >>>> +    description: 1.8V HDMI analog frontend power
> >>>> +
> >>>> +  txavcc33-supply:
> >>>> +    description: 3.3V HDMI analog frontend power
> >>>> +
> >>>> +  pvcc1-supply:
> >>>> +    description: 1.8V HDMI frontend core PLL power
> >>>> +
> >>>> +  pvcc2-supply:
> >>>> +    description: 1.8V HDMI frontend filter PLL power
> >>>> +
> >>>> +  avcc-supply:
> >>>> +    description: 3.3V LVDS frontend power
> >>>> +
> >>>> +  anvdd-supply:
> >>>> +    description: 1.8V LVDS frontend analog power
> >>>> +
> >>>> +  apvdd-supply:
> >>>> +    description: 1.8V LVDS frontend PLL power
> >>>> +
> >>>> +  "#sound-dai-cells":
> >>>> +    const: 0
> >>>> +
> >>>> +  ite,lvds-link-num-data-lanes:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint8
> >>>> +    enum: [3, 4, 5]
> >>>> +    description: number of data lanes per LVDS link
> >>>
> >>> Please use data-lanes property inside the OF graph.
> >>
> >> In both port@0 and port@1?
> >
> > Yes
> 
> I'm assuming if data-mapping is defined, then no need to define data-lanes.
> 
> >
> >>
> >>>
> >>>> +
> >>>> +  ite,i2s-audio-fifo-sources:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >>>> +    minItems: 1
> >>>> +    maxItems: 4
> >>>> +    items:
> >>>> +      enum: [0, 1, 2, 3]
> >>>> +    description:
> >>>> +      Each array element indicates the pin number of an I2S serial data input
> >>>> +      line which is connected to an audio FIFO, from audio FIFO0 to FIFO3.
> >>>
> >>> What does that mean from the board point of view? Routed audio links
> >>> for the multichannel audio?
> >>
> >> Yes, also for single channel audio.
> >>
> >>>
> >>>> +
> >>>> +  ite,rl-channel-swap-audio-sources:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >>>> +    minItems: 1
> >>>> +    maxItems: 4
> >>>> +    uniqueItems: true
> >>>> +    items:
> >>>> +      enum: [0, 1, 2, 3]
> >>>> +    description:
> >>>> +      Each array element indicates an audio source whose right channel and left
> >>>> +      channel are swapped by this converter. For I2S, the element is the pin
> >>>> +      number of an I2S serial data input line. For S/PDIF, the element is always
> >>>> +      0.
> >>>
> >>> Why?
> >>
> >> Because this converter has the capability to swap right channel and
> >> left channel and S/PDIF always uses audio source0.
> >>
> >>>
> >>>> +
> >>>> +  ports:
> >>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>> +
> >>>> +    properties:
> >>>> +      port@0:
> >>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >>>> +        unevaluatedProperties: false
> >>>> +        description:
> >>>> +          The first LVDS input link.
> >>>> +          In dual-link LVDS mode, this link works together with the second LVDS
> >>>> +          input link, and one link receives odd pixels while the other receives
> >>>> +          even pixels. Specify the pixels with one of the dual-lvds-odd-pixels
> >>>> +          and dual-lvds-even-pixels properties when and only when dual-link LVDS
> >>>> +          mode is used.
> >>>> +
> >>>> +        properties:
> >>>> +          dual-lvds-odd-pixels:
> >>>> +            type: boolean
> >>>> +            description: the first sink port for odd pixels
> >>>> +
> >>>> +          dual-lvds-even-pixels:
> >>>> +            type: boolean
> >>>> +            description: the first sink port for even pixels
> >>>> +
> >>>> +      port@1:
> >>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >>>> +        unevaluatedProperties: false
> >>>> +        description: the second LVDS input link
> >>>> +
> >>>> +        properties:
> >>>> +          dual-lvds-even-pixels:
> >>>> +            type: boolean
> >>>> +            description: the second sink port for even pixels
> >>>> +
> >>>> +          dual-lvds-odd-pixels:
> >>>> +            type: boolean
> >>>> +            description: the second sink port for odd pixels
> >>>> +
> >>>> +        oneOf:
> >>>> +          - required: [dual-lvds-even-pixels]
> >>>> +          - required: [dual-lvds-odd-pixels]
> >>>
> >>> This still allows one to specify that both ports provide odd pixels.
> >>> Is that expected? Also why do you need two properties which specify
> >>> the same option.
> >>
> >> No, that is not expected. Description for port@0 already mentions one
> >> link receives odd pixels while the other receives even pixels.
> >
> > That's not expected, but permitted by the bindings.
> 
> It is not permitted by port@1. If "dual-lvds-odd-pixels;" is added to port@1 in the dual-link LVDS
> example, the below warning will be generated by dt_binding_check.
> 
> Documentation/devicetree/bindings/display/bridge/ite,it6263.example.dtb: hdmi@4c: ports:port@1:
> {'reg': [[1]], 'dual-lvds-even-pixels': True, 'dual-lvds-odd-pixels': True, 'endpoint': {'remote-
> endpoint': [[4294967295]]}} is valid under each of {'required': ['dual-lvds-odd-pixels']},
> {'required': ['dual-lvds-even-pixels']}
> 	from schema $id: http://devicetree.org/schemas/display/bridge/ite,it6263.yaml#
> 
> However, the binding for port@0 does allow DT writers to specify both even and odd pixels. I raised
> similar concerns in v1 discussion.
> According to Rob's comment in #devicetree IRC, the ports property is simplified to this form and more
> description for port@0 is added to tell when to specify the even/odd pixels.  If you know any better
> way to indicate the constraints, please shout.
> 
> >
> >>
> >> Two options are supported for dual-link LVDS, not the same option:
> >> 1) LVDS link1(port@0) gets odd pixels
> >>    and
> >>    LVDS link2(port@1) gets even pixels.
> >>
> >> 2) LVDS link1(port@0) gets even pixels
> >>    and
> >>    LVDS link2(port@1) gets odd pixels.
> >> That's the reason why each port needs two properties to define
> >> odd/even pixels.
> >>
> >>>
> >>> My suggestion would be to add a single root-level property which
> >>> specifies which port provides even pixel data.
> >>
> >> That won't work.  The LVDS source side expects the ports of the sink
> >> side specify dual-lvds-{odd,even}-pixels properties.
> >
> > I didn't notice that these properties are already defined.
> >
> > As these properties are common between several schema files, please
> > extract them to a common schema file (like lvds.yaml).
> 
> I'm not sure how to do that. Is it obvious?
> Please shed some light.
> 
> Only two panel schema files are defining even/odd pixels now - advantech,idk-2121wr.yaml and panel-
> simple-lvds-dual-ports.yaml.
> Maybe, extract them later when more schema files(especially for
> bridges) try to define the same?  I'd like to keep a low profile for now.
> 
> >
> >>
> >>>
> >>>> +
> >>>> +      port@2:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: video port for the HDMI output
> >>>> +
> >>>> +      port@3:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: sound input port
> >>>> +
> >>>> +    required:
> >>>> +      - port@0
> >>>> +      - port@2
> >>>> +
> >>>> +required:
> >>>> +  - compatible
> >>>> +  - reg
> >>>> +  - ivdd-supply
> >>>> +  - ovdd-supply
> >>>> +  - txavcc18-supply
> >>>> +  - txavcc33-supply
> >>>> +  - pvcc1-supply
> >>>> +  - pvcc2-supply
> >>>> +  - avcc-supply
> >>>> +  - anvdd-supply
> >>>> +  - apvdd-supply
> >>>> +  - ite,lvds-link-num-data-lanes
> >>>> +  - ports
> >>>> +
> >>>> +additionalProperties: false
> >>>> +
> >>>> +examples:
> >>>> +  - |
> >>>> +    /* single-link LVDS input */
> >>>> +    #include <dt-bindings/gpio/gpio.h>
> >>>> +
> >>>> +    i2c {
> >>>> +        #address-cells = <1>;
> >>>> +        #size-cells = <0>;
> >>>> +
> >>>> +        hdmi@4c {
> >>>> +            compatible = "ite,it6263";
> >>>> +            reg = <0x4c>;
> >>>> +            reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
> >>>> +            ivdd-supply = <&reg_buck5>;
> >>>> +            ovdd-supply = <&reg_vext_3v3>;
> >>>> +            txavcc18-supply = <&reg_buck5>;
> >>>> +            txavcc33-supply = <&reg_vext_3v3>;
> >>>> +            pvcc1-supply = <&reg_buck5>;
> >>>> +            pvcc2-supply = <&reg_buck5>;
> >>>> +            avcc-supply = <&reg_vext_3v3>;
> >>>> +            anvdd-supply = <&reg_buck5>;
> >>>> +            apvdd-supply = <&reg_buck5>;
> >>>> +            ite,lvds-link-num-data-lanes = /bits/ 8 <4>;
> >>>> +
> >>>> +            ports {
> >>>> +                #address-cells = <1>;
> >>>> +                #size-cells = <0>;
> >>>> +
> >>>> +                port@0 {
> >>>> +                    reg = <0>;
> >>>> +
> >>>> +                    it6263_lvds_link1: endpoint {
> >>>> +                        remote-endpoint = <&ldb_lvds_ch0>;
> >>>> +                    };
> >>>> +                };
> >>>> +
> >>>> +                port@2 {
> >>>> +                    reg = <2>;
> >>>> +
> >>>> +                    it6263_out: endpoint {
> >>>> +                        remote-endpoint = <&hdmi_in>;
> >>>> +                    };
> >>>> +                };
> >>>> +            };
> >>>> +        };
> >>>> +    };
> >>>> +
> >>>> +  - |
> >>>> +    /* dual-link LVDS input */
> >>>> +    #include <dt-bindings/gpio/gpio.h>
> >>>> +
> >>>> +    i2c {
> >>>> +        #address-cells = <1>;
> >>>> +        #size-cells = <0>;
> >>>> +
> >>>> +        hdmi@4c {
> >>>> +            compatible = "ite,it6263";
> >>>> +            reg = <0x4c>;
> >>>> +            reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
> >>>> +            ivdd-supply = <&reg_buck5>;
> >>>> +            ovdd-supply = <&reg_vext_3v3>;
> >>>> +            txavcc18-supply = <&reg_buck5>;
> >>>> +            txavcc33-supply = <&reg_vext_3v3>;
> >>>> +            pvcc1-supply = <&reg_buck5>;
> >>>> +            pvcc2-supply = <&reg_buck5>;
> >>>> +            avcc-supply = <&reg_vext_3v3>;
> >>>> +            anvdd-supply = <&reg_buck5>;
> >>>> +            apvdd-supply = <&reg_buck5>;
> >>>> +            ite,lvds-link-num-data-lanes = /bits/ 8 <4>;
> >>>> +
> >>>> +            ports {
> >>>> +                #address-cells = <1>;
> >>>> +                #size-cells = <0>;
> >>>> +
> >>>> +                port@0 {
> >>>> +                    reg = <0>;
> >>>> +                    dual-lvds-odd-pixels;
> >>>> +
> >>>> +                    it6263_lvds_link1_dual: endpoint {
> >>>> +                        remote-endpoint = <&ldb_lvds_ch0>;
> >>>> +                    };
> >>>> +                };
> >>>> +
> >>>> +                port@1 {
> >>>> +                    reg = <1>;
> >>>> +                    dual-lvds-even-pixels;
> >>>> +
> >>>> +                    it6263_lvds_link2_dual: endpoint {
> >>>> +                        remote-endpoint = <&ldb_lvds_ch1>;
> >>>> +                    };
> >>>> +                };
> >>>> +
> >>>> +                port@2 {
> >>>> +                    reg = <2>;
> >>>> +
> >>>> +                    it6263_out_dual: endpoint {
> >>>> +                        remote-endpoint = <&hdmi_in>;
> >>>> +                    };
> >>>> +                };
> >>>> +            };
> >>>> +        };
> >>>> +    };
> >>>> --
> >>>> 2.34.1
> >>>>
> >>>
> >>
> >> --
> >> Regards,
> >> Liu Ying
> >>
> >
> 
> --
> Regards,
> Liu Ying

Powered by blists - more mailing lists