| Message ID | 20210130181014.161457-1-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings | expand |
On Sat, Jan 30, 2021 at 7:10 PM 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 This looks correct to me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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=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@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 >
On 2/4/21 6:15 PM, Doug Anderson wrote: Hi, [...] >> +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? Those are not implemented and not tested, so if someone needs them later on, they can be added then. > 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 is optional and again completely untested, so it can be added later if needed. > 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. In my case, the refclock are derived from the DSI link. >> + 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> Shouldn't that be <1 2 3 4> ? > 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. I saw the binding, but I was under the impressions the DSI86 can do lane reordering, isn't that the case ? Maybe I misunderstood it. But yes, if you have a suggestion how to make a non-cryptic list of those four lane mapping options, please do share this info. >> + 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? I don't really want to add any properties which I cannot test and then end up with DT bindings which would become poor ABI in the future. >> + >> + 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>? Well, git grep data-lanes seems to indicate some count from 1, some from 0 .
Hi, On Thu, Feb 4, 2021 at 10:09 AM Marek Vasut <marex@denx.de> wrote: > > On 2/4/21 6:15 PM, Doug Anderson wrote: > > Hi, > > [...] > > >> +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? > > Those are not implemented and not tested, so if someone needs them later > on, they can be added then. Sure. I guess it can go either way. For the regulator it'd the kind of thing that's super easy to add support for and hard to mess up. > >> + 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> > > Shouldn't that be <1 2 3 4> ? The data manual refers to the channels starting at 0, so if it's arbitrary that seems a better way to go? > > 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. > > I saw the binding, but I was under the impressions the DSI86 can do lane > reordering, isn't that the case ? Maybe I misunderstood it. DSI86 can reorder the output lanes quite flexibly. It can't reorder the input lanes, though. > But yes, if you have a suggestion how to make a non-cryptic list of > those four lane mapping options, please do share this info. I doubt I can write this correctly without a whole lot of futzing / messing, but maybe something like: data-lanes: oneOf: - items: - 0 - items: - 0 - 1 - items: - 0 - 1 - 2 - items: - 0 - 1 - 2 - 3
On 2/4/21 7:38 PM, Doug Anderson wrote: > Hi, Hi, [...] >>>> +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? >> >> Those are not implemented and not tested, so if someone needs them later >> on, they can be added then. > > Sure. I guess it can go either way. For the regulator it'd the kind > of thing that's super easy to add support for and hard to mess up. If someone can test those regulators (I might be able to, in next revision of hardware, we'll see), then this can be added. >>>> + 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> >> >> Shouldn't that be <1 2 3 4> ? > > The data manual refers to the channels starting at 0, so if it's > arbitrary that seems a better way to go? Either way is OK, but before I change this, I would like some confirmation this enumeration really is arbitrary. >>> 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. >> >> I saw the binding, but I was under the impressions the DSI86 can do lane >> reordering, isn't that the case ? Maybe I misunderstood it. > > DSI86 can reorder the output lanes quite flexibly. It can't reorder > the input lanes, though. The eDP ones ? OK >> But yes, if you have a suggestion how to make a non-cryptic list of >> those four lane mapping options, please do share this info. > > I doubt I can write this correctly without a whole lot of futzing / > messing, but maybe something like: > > data-lanes: > oneOf: > - items: > - 0 > - items: > - 0 > - 1 > - items: > - 0 > - 1 > - 2 > - items: > - 0 > - 1 > - 2 > - 3 > I was hoping for some better syntax. Maybe there is one ?
On Sat, Jan 30, 2021 at 07:10:13PM +0100, Marek Vasut 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(+) > 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=sn65dsi83&fileType=pdf > + > +properties: > + compatible: > + const: ti,sn65dsi83 > + > + reg: > + const: 0x2d > + > + enable-gpios: > + maxItems: 1 > + description: GPIO specifier for bridge_en pin (active high). > + > + 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. This all needs to use graph.yaml and video-interfaces.yaml. The latter is in the media tree. See examples there for what to do. It will have to wait for rc1 to apply to drm-misc. For data-lanes, you need to specify how many lanes are valid. If there's only 1 possible setting (in the h/w, not driver), then it doesn't need to be in DT. I agree with Doug on adding the regulators. Hard to get wrong in the binding. You or someone can add them to the driver when you can test. > + > + 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 > + > + 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>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + endpoint { > + remote-endpoint = <&panel_in_lvds>; > + }; > + }; > + }; > + }; > + }; > -- > 2.29.2 >
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=sn65dsi83&fileType=pdf + +properties: + compatible: + const: ti,sn65dsi83 + + reg: + const: 0x2d + + enable-gpios: + maxItems: 1 + description: GPIO specifier for bridge_en pin (active high). + + 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. + + 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 + + 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>; + }; + }; + + port@1 { + reg = <1>; + endpoint { + remote-endpoint = <&panel_in_lvds>; + }; + }; + }; + }; + };
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(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml