Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings

Hi,

On Sat, Jan 30, 2021 at 10:10 AM Marek Vasut marex@denx.de wrote:

Add DT binding document for TI SN65DSI83 DSI to LVDS bridge.

Signed-off-by: Marek Vasut marex@denx.de Cc: Douglas Anderson dianders@chromium.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Linus Walleij linus.walleij@linaro.org Cc: Sam Ravnborg sam@ravnborg.org Cc: Stephen Boyd swboyd@chromium.org Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org

---

.../bindings/display/bridge/ti,sn65dsi83.yaml | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+)

I'll preface by saying that I'm nowhere near an expert on any of this stuff. I've done a bunch of hacking on the sn65dsi86 driver but mostly I'm a DRM noob and don't have any super expertise in MIPI or LVDS.

create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml new file mode 100644 index 000000000000..77e1bafd8cd8 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -0,0 +1,128 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi83.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: SN65DSI83 DSI to LVDS bridge chip

+maintainers:

• • Marek Vasut marex@denx.de

+description: |

• The Texas Instruments SN65DSI83 bridge takes MIPI DSI in and outputs LVDS.
• https://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65...

+properties:

• compatible:
• const: ti,sn65dsi83
• reg:
• const: 0x2d
• enable-gpios:
• maxItems: 1
• description: GPIO specifier for bridge_en pin (active high).

I see two regulators: vcc and vcore. Shouldn't those be listed?

I also see an interrupt pin on the datasheet. Probably should be listed too even if the chip can be made to work fine without hooking it up. It can just be optional, right?

It wouldn't hurt to list the refclk here too even if the code doesn't use it. From sn65dsi86 it was handy that the bindings already had all this type of stuff so that when we added the feature we didn't have to go back to the bindings.

• ports:
• type: object
• additionalProperties: false
• properties:

•  "#address-cells":
  

•    const: 1
  

•  "#size-cells":
  

•    const: 0
  

•  port@0:
  

•    type: object
  

•    additionalProperties: false
  

•    description:
  

•      Video port for MIPI DSI input
  

•    properties:
  

•      reg:
  

•        const: 0
  

•      endpoint:
  

•        type: object
  

•        additionalProperties: false
  

•        properties:
  

•          remote-endpoint: true
  

•          data-lanes:
  

•            description: array of physical DSI data lane indexes.
  

The chip doesn't allow for arbitrary remapping here, right? So you're just using this as the official way to specify the number of lanes? I guess the only valid values are:

<0> <0 1> <0 1 2> <0 1 2 3>

In sn65dsi86 we attempted to enforce that a valid option was selected for the output lanes. Could you do something similar? If nothing else adding a description of the valid options would be good.

•    required:
  

•      - reg
  

•  port@1:
  

•    type: object
  

•    additionalProperties: false
  

•    description:
  

•      Video port for LVDS output (panel or bridge).
  

•    properties:
  

•      reg:
  

•        const: 1
  

•      endpoint:
  

•        type: object
  

•        additionalProperties: false
  

•        properties:
  

•          remote-endpoint: true
  

Worth adding the data-lanes here too? I guess this part allows you two different orders for the LVDS outputs?

•    required:
  

•      - reg
  

• required:

•  - "#address-cells"
  

•  - "#size-cells"
  

•  - port@0
  

•  - port@1
  

+required:

• • compatible
• • reg
• • enable-gpios
• • ports

+additionalProperties: false

+examples:

• • |
• #include <dt-bindings/gpio/gpio.h>
• i2c {

•  #address-cells = <1>;
  

•  #size-cells = <0>;
  

•  bridge@2d {
  

•    compatible = "ti,sn65dsi83";
  

•    reg = <0x2d>;
  

•    enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
  

•    ports {
  

•      #address-cells = <1>;
  

•      #size-cells = <0>;
  

•      port@0 {
  

•        reg = <0>;
  

•        endpoint {
  

•          remote-endpoint = <&dsi0_out>;
  

•          data-lanes = <1 2 3 4>;
  

Should the above be <0 1 2 3>?

•        };
  

•      };
  

•      port@1 {
  

•        reg = <1>;
  

•        endpoint {
  

•          remote-endpoint = <&panel_in_lvds>;
  

•        };
  

•      };
  

•    };
  

•  };
  

• };

-- 2.29.2