diff mbox series

[v2,2/4] dt-bindings: display: bridge: add it66121 bindings

Message ID 20200311125135.30832-3-ple@baylibre.com (mailing list archive)
State New, archived
Headers show
Series Add it66121 driver | expand

Commit Message

Phong LE March 11, 2020, 12:51 p.m. UTC
Add the ITE bridge HDMI it66121 bindings.

Signed-off-by: Phong LE <ple@baylibre.com>
---
 .../bindings/display/bridge/ite,it66121.yaml  | 98 +++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml

Comments

Laurent Pinchart March 13, 2020, 1:40 p.m. UTC | #1
Hi Phong,

Thank you for the patch.

On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> Add the ITE bridge HDMI it66121 bindings.
> 
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> new file mode 100644
> index 000000000000..1717e880d130
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ITE it66121 HDMI bridge Device Tree Bindings
> +
> +maintainers:
> +  - Phong LE <ple@baylibre.com>
> +  - Neil Armstrong <narmstrong@baylibre.com>
> +
> +description: |
> +  The IT66121 is a high-performance and low-power single channel HDMI
> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
> +  to DVI 1.0 specifications.
> +
> +properties:
> +  compatible:
> +    const: ite,it66121
> +
> +  reg:
> +    maxItems: 1
> +    description: base I2C address of the device
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO connected to active low reset
> +
> +  vrf12-supply:
> +    maxItems: 1
> +    description: Regulator for 1.2V analog core power.
> +
> +  vcn33-supply:
> +    maxItems: 1
> +    description: Regulator for 3.3V digital core power.
> +
> +  vcn18-supply:
> +    maxItems: 1
> +    description: Regulator for 1.8V IO core power.
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  pclk-dual-edge:
> +    maxItems: 1
> +    description: enable pclk dual edge mode.

I'm having a bit of trouble understanding how this operates. Looking at
the driver code the property is only taken into account to calculate the
maximum allowed frequency. How is the IT66121 configured for single vs.
dual pixel clock edge mode ?

> +
> +  port:
> +    type: object
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        description: |
> +          Input endpoints of the bridge.
> +
> +    required:
> +      - endpoint

You should have two ports, one for the bridge input, and one for the
bridge output.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - vrf12-supply
> +  - vcn33-supply
> +  - vcn18-supply
> +  - interrupts
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c6 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      it66121hdmitx: it66121hdmitx@4c {
> +        compatible = "ite,it66121";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&ite_pins_default>;
> +        vcn33-supply = <&mt6358_vcn33_wifi_reg>;
> +        vcn18-supply = <&mt6358_vcn18_reg>;
> +        vrf12-supply = <&mt6358_vrf12_reg>;
> +        reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
> +        interrupt-parent = <&pio>;
> +        interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> +        reg = <0x4c>;
> +        pclk-dual-edge;
> +
> +        port {
> +          it66121_in: endpoint {
> +            remote-endpoint = <&display_out>;
> +          };
> +        };
> +      };
> +    };
Neil Armstrong March 13, 2020, 2:12 p.m. UTC | #2
On 13/03/2020 14:40, Laurent Pinchart wrote:
> Hi Phong,
> 
> Thank you for the patch.
> 
> On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
>> Add the ITE bridge HDMI it66121 bindings.
>>
>> Signed-off-by: Phong LE <ple@baylibre.com>
>> ---
>>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++++++++++++++++++
>>  1 file changed, 98 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>> new file mode 100644
>> index 000000000000..1717e880d130
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>> @@ -0,0 +1,98 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ITE it66121 HDMI bridge Device Tree Bindings
>> +
>> +maintainers:
>> +  - Phong LE <ple@baylibre.com>
>> +  - Neil Armstrong <narmstrong@baylibre.com>
>> +
>> +description: |
>> +  The IT66121 is a high-performance and low-power single channel HDMI
>> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
>> +  to DVI 1.0 specifications.
>> +
>> +properties:
>> +  compatible:
>> +    const: ite,it66121
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: base I2C address of the device
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +    description: GPIO connected to active low reset
>> +
>> +  vrf12-supply:
>> +    maxItems: 1
>> +    description: Regulator for 1.2V analog core power.
>> +
>> +  vcn33-supply:
>> +    maxItems: 1
>> +    description: Regulator for 3.3V digital core power.
>> +
>> +  vcn18-supply:
>> +    maxItems: 1
>> +    description: Regulator for 1.8V IO core power.
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  pclk-dual-edge:
>> +    maxItems: 1
>> +    description: enable pclk dual edge mode.
> 
> I'm having a bit of trouble understanding how this operates. Looking at
> the driver code the property is only taken into account to calculate the
> maximum allowed frequency. How is the IT66121 configured for single vs.
> dual pixel clock edge mode ?


Dual edge mode is Dual-Data-Rate mode, the normal mode is MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a single clock period.

This should be negociated at runtime, but the bus width should be specified somewhere to select
one of the modes.

Neil

> 
>> +
>> +  port:
>> +    type: object
>> +
>> +    properties:
>> +      endpoint:
>> +        type: object
>> +        description: |
>> +          Input endpoints of the bridge.
>> +
>> +    required:
>> +      - endpoint
> 
> You should have two ports, one for the bridge input, and one for the
> bridge output.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reset-gpios
>> +  - vrf12-supply
>> +  - vcn33-supply
>> +  - vcn18-supply
>> +  - interrupts
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c6 {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      it66121hdmitx: it66121hdmitx@4c {
>> +        compatible = "ite,it66121";
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&ite_pins_default>;
>> +        vcn33-supply = <&mt6358_vcn33_wifi_reg>;
>> +        vcn18-supply = <&mt6358_vcn18_reg>;
>> +        vrf12-supply = <&mt6358_vrf12_reg>;
>> +        reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
>> +        interrupt-parent = <&pio>;
>> +        interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
>> +        reg = <0x4c>;
>> +        pclk-dual-edge;
>> +
>> +        port {
>> +          it66121_in: endpoint {
>> +            remote-endpoint = <&display_out>;
>> +          };
>> +        };
>> +      };
>> +    };
>
Laurent Pinchart March 13, 2020, 2:17 p.m. UTC | #3
Hi Neil,

On Fri, Mar 13, 2020 at 03:12:13PM +0100, Neil Armstrong wrote:
> On 13/03/2020 14:40, Laurent Pinchart wrote:
> > On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> >> Add the ITE bridge HDMI it66121 bindings.
> >>
> >> Signed-off-by: Phong LE <ple@baylibre.com>
> >> ---
> >>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++++++++++++++++++
> >>  1 file changed, 98 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >> new file mode 100644
> >> index 000000000000..1717e880d130
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >> @@ -0,0 +1,98 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: ITE it66121 HDMI bridge Device Tree Bindings
> >> +
> >> +maintainers:
> >> +  - Phong LE <ple@baylibre.com>
> >> +  - Neil Armstrong <narmstrong@baylibre.com>
> >> +
> >> +description: |
> >> +  The IT66121 is a high-performance and low-power single channel HDMI
> >> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
> >> +  to DVI 1.0 specifications.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: ite,it66121
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +    description: base I2C address of the device
> >> +
> >> +  reset-gpios:
> >> +    maxItems: 1
> >> +    description: GPIO connected to active low reset
> >> +
> >> +  vrf12-supply:
> >> +    maxItems: 1
> >> +    description: Regulator for 1.2V analog core power.
> >> +
> >> +  vcn33-supply:
> >> +    maxItems: 1
> >> +    description: Regulator for 3.3V digital core power.
> >> +
> >> +  vcn18-supply:
> >> +    maxItems: 1
> >> +    description: Regulator for 1.8V IO core power.
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> >> +  pclk-dual-edge:
> >> +    maxItems: 1
> >> +    description: enable pclk dual edge mode.
> > 
> > I'm having a bit of trouble understanding how this operates. Looking at
> > the driver code the property is only taken into account to calculate the
> > maximum allowed frequency. How is the IT66121 configured for single vs.
> > dual pixel clock edge mode ?
> 
> Dual edge mode is Dual-Data-Rate mode, the normal mode is MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
> MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a single clock period.
> 
> This should be negociated at runtime, but the bus width should be specified somewhere to select
> one of the modes.

How about replacing this property by bus-width to report the connected
bus width ? It should then become an endpoint property.

> >> +
> >> +  port:
> >> +    type: object
> >> +
> >> +    properties:
> >> +      endpoint:
> >> +        type: object
> >> +        description: |
> >> +          Input endpoints of the bridge.
> >> +
> >> +    required:
> >> +      - endpoint
> > 
> > You should have two ports, one for the bridge input, and one for the
> > bridge output.
> > 
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - reset-gpios
> >> +  - vrf12-supply
> >> +  - vcn33-supply
> >> +  - vcn18-supply
> >> +  - interrupts
> >> +  - port
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    i2c6 {
> >> +      #address-cells = <1>;
> >> +      #size-cells = <0>;
> >> +
> >> +      it66121hdmitx: it66121hdmitx@4c {
> >> +        compatible = "ite,it66121";
> >> +        pinctrl-names = "default";
> >> +        pinctrl-0 = <&ite_pins_default>;
> >> +        vcn33-supply = <&mt6358_vcn33_wifi_reg>;
> >> +        vcn18-supply = <&mt6358_vcn18_reg>;
> >> +        vrf12-supply = <&mt6358_vrf12_reg>;
> >> +        reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
> >> +        interrupt-parent = <&pio>;
> >> +        interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> >> +        reg = <0x4c>;
> >> +        pclk-dual-edge;
> >> +
> >> +        port {
> >> +          it66121_in: endpoint {
> >> +            remote-endpoint = <&display_out>;
> >> +          };
> >> +        };
> >> +      };
> >> +    };
Neil Armstrong March 13, 2020, 3:29 p.m. UTC | #4
On 13/03/2020 15:17, Laurent Pinchart wrote:
> Hi Neil,
> 
> On Fri, Mar 13, 2020 at 03:12:13PM +0100, Neil Armstrong wrote:
>> On 13/03/2020 14:40, Laurent Pinchart wrote:
>>> On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
>>>> Add the ITE bridge HDMI it66121 bindings.
>>>>
>>>> Signed-off-by: Phong LE <ple@baylibre.com>
>>>> ---
>>>>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++++++++++++++++++
>>>>  1 file changed, 98 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>>>>

[...]

>>>> +
>>>> +  pclk-dual-edge:
>>>> +    maxItems: 1
>>>> +    description: enable pclk dual edge mode.
>>>
>>> I'm having a bit of trouble understanding how this operates. Looking at
>>> the driver code the property is only taken into account to calculate the
>>> maximum allowed frequency. How is the IT66121 configured for single vs.
>>> dual pixel clock edge mode ?
>>
>> Dual edge mode is Dual-Data-Rate mode, the normal mode is MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
>> MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a single clock period.
>>
>> This should be negociated at runtime, but the bus width should be specified somewhere to select
>> one of the modes.
> 
> How about replacing this property by bus-width to report the connected
> bus width ? It should then become an endpoint property.

It was my thought.

The mediatek dpi also uses this property, which is also very wrong in the same way.

Neil

[...]
Rob Herring (Arm) March 23, 2020, 10:09 p.m. UTC | #5
On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> Add the ITE bridge HDMI it66121 bindings.
> 
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> new file mode 100644
> index 000000000000..1717e880d130
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ITE it66121 HDMI bridge Device Tree Bindings
> +
> +maintainers:
> +  - Phong LE <ple@baylibre.com>
> +  - Neil Armstrong <narmstrong@baylibre.com>
> +
> +description: |
> +  The IT66121 is a high-performance and low-power single channel HDMI
> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
> +  to DVI 1.0 specifications.
> +
> +properties:
> +  compatible:
> +    const: ite,it66121
> +
> +  reg:
> +    maxItems: 1
> +    description: base I2C address of the device
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO connected to active low reset
> +
> +  vrf12-supply:
> +    maxItems: 1

Drop this. Supplies are always a single item.

> +    description: Regulator for 1.2V analog core power.
> +
> +  vcn33-supply:
> +    maxItems: 1
> +    description: Regulator for 3.3V digital core power.
> +
> +  vcn18-supply:
> +    maxItems: 1
> +    description: Regulator for 1.8V IO core power.
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  pclk-dual-edge:
> +    maxItems: 1

Not a valid property if this is boolean.

> +    description: enable pclk dual edge mode.
> +
> +  port:

You should have an output port to a connector node.

> +    type: object
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        description: |
> +          Input endpoints of the bridge.
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - vrf12-supply
> +  - vcn33-supply
> +  - vcn18-supply
> +  - interrupts
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c6 {

i2c {

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      it66121hdmitx: it66121hdmitx@4c {
> +        compatible = "ite,it66121";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&ite_pins_default>;
> +        vcn33-supply = <&mt6358_vcn33_wifi_reg>;
> +        vcn18-supply = <&mt6358_vcn18_reg>;
> +        vrf12-supply = <&mt6358_vrf12_reg>;
> +        reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
> +        interrupt-parent = <&pio>;
> +        interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> +        reg = <0x4c>;
> +        pclk-dual-edge;
> +
> +        port {
> +          it66121_in: endpoint {
> +            remote-endpoint = <&display_out>;
> +          };
> +        };
> +      };
> +    };
> -- 
> 2.17.1
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
new file mode 100644
index 000000000000..1717e880d130
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
@@ -0,0 +1,98 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ITE it66121 HDMI bridge Device Tree Bindings
+
+maintainers:
+  - Phong LE <ple@baylibre.com>
+  - Neil Armstrong <narmstrong@baylibre.com>
+
+description: |
+  The IT66121 is a high-performance and low-power single channel HDMI
+  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
+  to DVI 1.0 specifications.
+
+properties:
+  compatible:
+    const: ite,it66121
+
+  reg:
+    maxItems: 1
+    description: base I2C address of the device
+
+  reset-gpios:
+    maxItems: 1
+    description: GPIO connected to active low reset
+
+  vrf12-supply:
+    maxItems: 1
+    description: Regulator for 1.2V analog core power.
+
+  vcn33-supply:
+    maxItems: 1
+    description: Regulator for 3.3V digital core power.
+
+  vcn18-supply:
+    maxItems: 1
+    description: Regulator for 1.8V IO core power.
+
+  interrupts:
+    maxItems: 1
+
+  pclk-dual-edge:
+    maxItems: 1
+    description: enable pclk dual edge mode.
+
+  port:
+    type: object
+
+    properties:
+      endpoint:
+        type: object
+        description: |
+          Input endpoints of the bridge.
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - vrf12-supply
+  - vcn33-supply
+  - vcn18-supply
+  - interrupts
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c6 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      it66121hdmitx: it66121hdmitx@4c {
+        compatible = "ite,it66121";
+        pinctrl-names = "default";
+        pinctrl-0 = <&ite_pins_default>;
+        vcn33-supply = <&mt6358_vcn33_wifi_reg>;
+        vcn18-supply = <&mt6358_vcn18_reg>;
+        vrf12-supply = <&mt6358_vrf12_reg>;
+        reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
+        interrupt-parent = <&pio>;
+        interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
+        reg = <0x4c>;
+        pclk-dual-edge;
+
+        port {
+          it66121_in: endpoint {
+            remote-endpoint = <&display_out>;
+          };
+        };
+      };
+    };