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

[Doug Anderson]

Hi,

On Sat, Jan 30, 2021 at 10:10 AM Marek Vasut <marex at denx.de> wrote:
>
> Add DT binding document for TI SN65DSI83 DSI to LVDS bridge.
>
> Signed-off-by: Marek Vasut <marex at denx.de>
> Cc: Douglas Anderson <dianders at chromium.org>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Linus Walleij <linus.walleij at linaro.org>
> Cc: Sam Ravnborg <sam at ravnborg.org>
> Cc: Stephen Boyd <swboyd at chromium.org>
> Cc: devicetree at vger.kernel.org
> To: dri-devel at 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 at 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=sn65dsi83&fileType=pdf
> +
> +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 at 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 at 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 at 0
> +      - port at 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - enable-gpios
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      bridge at 2d {
> +        compatible = "ti,sn65dsi83";
> +        reg = <0x2d>;
> +
> +        enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
> +
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          port at 0 {
> +            reg = <0>;
> +            endpoint {
> +              remote-endpoint = <&dsi0_out>;
> +              data-lanes = <1 2 3 4>;

Should the above be <0 1 2 3>?



> +            };
> +          };
> +
> +          port at 1 {
> +            reg = <1>;
> +            endpoint {
> +              remote-endpoint = <&panel_in_lvds>;
> +            };
> +          };
> +        };
> +      };
> +    };
> --
> 2.29.2
>