Re: [PATCH 2/3] dt-bindings: display: bridge: Add documentation for LT9611

Hi Laurent,

Sorry for late reply, I was out last week.

On 28-05-20, 04:48, Laurent Pinchart wrote:

...

• interrupts:
• maxItems: 1
• description: interrupt line for the chip

I think you could drop the descriptions for the reg and interrupt properties, they don't add much.

Sure, will do

...

• reset-gpios:
• maxItems: 1
• description: GPIO connected to active high RESET pin.
• vdd-supply:
• description: Regulator for 1.8V MIPI phy power.
• vcc-supply:
• description: Regulator for 3.3V IO power.
• ports:
• type: object
• properties:

•  "#address-cells":
  

•    const: 1
  

•  "#size-cells":
  

•    const: 0
  

•  port@0:
  

•    type: object
  

•    additionalProperties: false
  

•    description: |
  

•      HDMI port for HDMI output
  

The usual practice is to have the input ports first, followed by the output ports. Is there a reason not to follow that rule ?

I was not aware of this rule, is it documented somewhere? Nevertheless will update..

...

•    properties:
  

•      reg:
  

•        const: 0
  

•    patternProperties:
  

•      endpoint:
  

If you want to use patternProperties, this should be

      "^endpoint@[0-9]+$":

(including the quotes). Same below.

Ok

...

•        type: object
  

•        additionalProperties: false
  

•        properties:
  

•          remote-endpoint: true
  

How about

          remote-endpoint:
            $ref: /schemas/types.yaml#/definitions/phandle

and the same below ?

Ok

You also need a reg property if multiple endpoints are present.

Will update

...

•    required:
  

•      - reg
  

•  port@1:
  

•    type: object
  

•    additionalProperties: false
  

•    description: |
  

•      MIPI port-1 for MIPI input
  

•    properties:
  

•      reg:
  

•        const: 1
  

•    patternProperties:
  

•      endpoint:
  

•        type: object
  

•        additionalProperties: false
  

•        properties:
  

•          remote-endpoint: true
  

•    required:
  

•      - reg
  

•  port@2:
  

•    type: object
  

•    additionalProperties: false
  

•    description: |
  

•      MIPI port-2 for MIPI input
  

A description of how the two MIPI inputs differ would be useful. In particular, are both mandatory, or is it valid to connect only one of the two ? If using a single input is supported, can it be either, or does it have to be the first one ? When using both inputs, what should be connected to them ?

Sure I will add details. port-1 is mandatory and port-2 optional. port-2 is used in combination with port-1 to drive displays for higher resolution like 4k

...

•    properties:
  

•      reg:
  

•        const: 2
  

•    patternProperties:
  

•      endpoint:
  

•        type: object
  

•        additionalProperties: false
  

•        properties:
  

•          remote-endpoint: true
  

•    required:
  

•      - reg
  

• required:

•  - "#address-cells"
  

•  - "#size-cells"
  

•  - port@0
  

•  - port@1
  

+required:

• • compatible
• • reg
• • interrupts
• • vdd-supply
• • vcc-supply
• • ports

+additionalProperties: false

+examples:

• • |
• #include <dt-bindings/gpio/gpio.h>
• #include <dt-bindings/interrupt-controller/irq.h>
• i2c10 {

•  #address-cells = <1>;
  

•  #size-cells = <0>;
  

•  lt9611_codec: hdmi-bridge@3b {
  

Please drop unused labels.

ok

...

•    compatible = "lontium,lt9611";
  

•    reg = <0x3b>;
  

•    reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
  

•    interrupts-extended = <&tlmm 84 IRQ_TYPE_EDGE_FALLING>;
  

•    vdd-supply = <&lt9611_1v8>;
  

•    vcc-supply = <&lt9611_3v3>;
  

•    ports {
  

•      #address-cells = <1>;
  

•      #size-cells = <0>;
  

•      port@0 {
  

•        reg = <0>;
  

•        lt9611_out: endpoint {
  

•          remote-endpoint = <&hdmi_con>;
  

•        };
  

•      };
  

•      port@1 {
  

•        reg = <1>;
  

•        lt9611_a: endpoint {
  

•          remote-endpoint = <&dsi0_out>;
  

•        };
  

•      };
  

•      port@2 {
  

•        reg = <2>;
  

•        lt9611_b: endpoint {
  

•          remote-endpoint = <&dsi1_out>;
  

•        };
  

•      };
  

•    };
  

•  };
  

• };

It's customary to end YAML schema files with ... on a separate line.

Will update

Thanks for the review