* [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-01-30 18:10 Marek Vasut
2021-02-02 19:41 ` Linus Walleij
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Marek Vasut @ 2021-01-30 18:10 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Douglas Anderson, Laurent Pinchart, Linus Walleij,
Sam Ravnborg, Stephen Boyd, devicetree
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.
+
+ 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
2021-01-30 18:10 [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings Marek Vasut
@ 2021-02-02 19:41 ` Linus Walleij
2021-02-04 17:15 ` Doug Anderson
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-02-02 19:41 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:DRM PANEL DRIVERS, Douglas Anderson, Laurent Pinchart,
Sam Ravnborg, Stephen Boyd,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-02-02 19:41 ` Linus Walleij
0 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-02-02 19:41 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Douglas Anderson, open list:DRM PANEL DRIVERS, Stephen Boyd,
Laurent Pinchart, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
2021-01-30 18:10 [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings Marek Vasut
@ 2021-02-04 17:15 ` Doug Anderson
2021-02-04 17:15 ` Doug Anderson
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2021-02-04 17:15 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Laurent Pinchart, Linus Walleij, Sam Ravnborg,
Stephen Boyd,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE 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=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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-02-04 17:15 ` Doug Anderson
0 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2021-02-04 17:15 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
dri-devel, Stephen Boyd, Laurent Pinchart, Sam Ravnborg
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
[not found] ` <20210130181014.161457-2-marex@denx.de>
@ 2021-02-04 17:15 ` Doug Anderson
2021-02-04 18:40 ` Marek Vasut
0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2021-02-04 17:15 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Stephen Boyd, Philippe Schenker, Laurent Pinchart,
Valentin Raevsky, Sam Ravnborg
Hi,
On Sat, Jan 30, 2021 at 10:10 AM Marek Vasut <marex@denx.de> wrote:
>
> Add driver for TI SN65DSI83 Single-Channel DSI to Single-Link LVDS bridge.
> The driver operates the chip over I2C bus. Currently the LVDS clock are
> always derived from DSI clock lane, which is the usual mode of operation,
> support for clock from external oscillator is not implemented, but it is
> easy to add if ever needed. Only RGB888 pixel format is implemented, the
> LVDS666 is not supported, but could be added if needed.
>
> 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: Philippe Schenker <philippe.schenker@toradex.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Valentin Raevsky <valentin@compulab.co.il>
> To: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/bridge/Kconfig | 10 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 535 ++++++++++++++++++++++++++
> 3 files changed, 546 insertions(+)
I shouldn't be the main reviewer because, as I've said elsewhere, I'm
a DRM noob and know almost nothing about LVDS or MIPI. :-P ...but
I've hacked a bunch on the sn65dsi86 driver and lots of others have
helped me with reviews, so I'll give it a shot...
> create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi83.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index cc401786962a..5e4f674e3fdb 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -234,6 +234,16 @@ config DRM_TI_TFP410
> help
> Texas Instruments TFP410 DVI/HDMI Transmitter driver
>
> +config DRM_TI_SN65DSI83
> + tristate "TI SN65DSI83 DSI to LVDS bridge"
> + depends on OF
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + select DRM_PANEL
> + select DRM_MIPI_DSI
> + help
> + Texas Instruments SN65DSI83 DSI to LVDS Bridge driver
> +
> config DRM_TI_SN65DSI86
> tristate "TI SN65DSI86 DSI to eDP bridge"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 21ea8fe09992..e11415db0a31 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o
> obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o
> obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> +obj-$(CONFIG_DRM_TI_SN65DSI83) += ti-sn65dsi83.o
> obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
> obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> new file mode 100644
> index 000000000000..79c023ba3dce
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -0,0 +1,535 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Marek Vasut <marex@denx.de>
> + *
> + * Based on previous work of:
> + * Valentin Raevsky <valentin@compulab.co.il>
> + * Philippe Schenker <philippe.schenker@toradex.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of_graph.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +/* ID registers */
> +#define REG_ID(n) (0x00 + (n))
> +/* Reset and clock registers */
> +#define REG_RC_RESET 0x09
> +#define REG_RC_RESET_SOFT_RESET BIT(0)
> +#define REG_RC_LVDS_PLL 0x0a
> +#define REG_RC_LVDS_PLL_PLL_EN_STAT BIT(7)
> +#define REG_RC_LVDS_PLL_LVDS_CLK_RANGE(n) (((n) & 0x7) << 1)
> +#define REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY BIT(0)
> +#define REG_RC_DSI_CLK 0x0b
> +#define REG_RC_DSI_CLK_DSI_CLK_DIVIDER(n) (((n) & 0x1f) << 3)
> +#define REG_RC_DSI_CLK_REFCLK_MULTIPLIER(n) ((n) & 0x3)
> +#define REG_RC_PLL_EN 0x0d
> +#define REG_RC_PLL_EN_PLL_EN BIT(0)
> +/* DSI registers */
> +#define REG_DSI_LANE 0x10
> +#define REG_DSI_LANE_RESERVED BIT(5)
> +#define REG_DSI_LANE_CHA_DSI_LANES(n) (((n) & 0x3) << 3)
> +#define REG_DSI_LANE_SOT_ERR_TOL_DIS BIT(0)
> +#define REG_DSI_EQ 0x11
> +#define REG_DSI_EQ_CHA_DSI_DATA_EQ(n) (((n) & 0x3) << 6)
> +#define REG_DSI_EQ_CHA_DSI_CLK_EQ(n) (((n) & 0x3) << 2)
> +#define REG_DSI_CLK 0x12
> +#define REG_DSI_CLK_CHA_DSI_CLK_RANGE(n) ((n) & 0xff)
> +/* LVDS registers */
> +#define REG_LVDS_FMT 0x18
> +#define REG_LVDS_FMT_DE_NEG_POLARITY BIT(7)
> +#define REG_LVDS_FMT_HS_NEG_POLARITY BIT(6)
> +#define REG_LVDS_FMT_VS_NEG_POLARITY BIT(5)
> +#define REG_LVDS_FMT_RESERVED BIT(4) /* Set to 1 */
> +#define REG_LVDS_FMT_CHA_24BPP_MODE BIT(3)
> +#define REG_LVDS_FMT_CHA_24BPP_FORMAT1 BIT(1)
> +#define REG_LVDS_VCOM 0x19
> +#define REG_LVDS_VCOM_CHA_LVDS_VOCM BIT(6)
> +#define REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(n) (((n) & 0x3) << 2)
> +#define REG_LVDS_LANE 0x1a
> +#define REG_LVDS_LANE_CHA_REVERSE_LVDS BIT(5)
> +#define REG_LVDS_LANE_CHA_LVDS_TERM BIT(1)
> +#define REG_LVDS_CM 0x1b
> +#define REG_LVDS_CM_CHA_LVDS_CM_ADJUST(n) ((n) & 0x3)
> +/* Video registers */
> +#define REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW 0x20
> +#define REG_VID_CHA_ACTIVE_LINE_LENGTH_HIGH 0x21
> +#define REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW 0x24
> +#define REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH 0x25
> +#define REG_VID_CHA_SYNC_DELAY_LOW 0x28
> +#define REG_VID_CHA_SYNC_DELAY_HIGH 0x29
> +#define REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW 0x2c
> +#define REG_VID_CHA_HSYNC_PULSE_WIDTH_HIGH 0x2d
> +#define REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW 0x30
> +#define REG_VID_CHA_VSYNC_PULSE_WIDTH_HIGH 0x31
> +#define REG_VID_CHA_HORIZONTAL_BACK_PORCH 0x34
> +#define REG_VID_CHA_VERTICAL_BACK_PORCH 0x36
> +#define REG_VID_CHA_HORIZONTAL_FRONT_PORCH 0x38
> +#define REG_VID_CHA_VERTICAL_FRONT_PORCH 0x3a
> +#define REG_VID_CHA_TEST_PATTERN 0x3c
> +/* IRQ registers */
> +#define REG_IRQ_GLOBAL 0xe0
> +#define REG_IRQ_GLOBAL_IRQ_EN BIT(0)
> +#define REG_IRQ_EN 0xe1
> +#define REG_IRQ_EN_CHA_SYNCH_ERR_EN BIT(7)
> +#define REG_IRQ_EN_CHA_CRC_ERR_EN BIT(6)
> +#define REG_IRQ_EN_CHA_UNC_ECC_ERR_EN BIT(5)
> +#define REG_IRQ_EN_CHA_COR_ECC_ERR_EN BIT(4)
> +#define REG_IRQ_EN_CHA_LLP_ERR_EN BIT(3)
> +#define REG_IRQ_EN_CHA_SOT_BIT_ERR_EN BIT(2)
> +#define REG_IRQ_EN_CHA_PLL_UNLOCK_EN BIT(0)
> +#define REG_IRQ_STAT 0xe5
> +#define REG_IRQ_STAT_CHA_SYNCH_ERR BIT(7)
> +#define REG_IRQ_STAT_CHA_CRC_ERR BIT(6)
> +#define REG_IRQ_STAT_CHA_UNC_ECC_ERR BIT(5)
> +#define REG_IRQ_STAT_CHA_COR_ECC_ERR BIT(4)
> +#define REG_IRQ_STAT_CHA_LLP_ERR BIT(3)
> +#define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2)
> +#define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0)
> +
> +struct sn65dsi83 {
> + struct drm_bridge bridge;
> + struct drm_display_mode mode;
> + struct device *dev;
> + struct regmap *regmap;
> + struct device_node *host_node;
> + struct mipi_dsi_device *dsi;
> + struct drm_bridge *panel_bridge;
> + struct gpio_desc *enable_gpio;
> + int dsi_lanes;
> +};
> +
> +static const struct regmap_range sn65dsi83_readable_ranges[] = {
> + regmap_reg_range(REG_ID(0), REG_ID(8)),
> + regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_DSI_CLK),
> + regmap_reg_range(REG_RC_PLL_EN, REG_RC_PLL_EN),
> + regmap_reg_range(REG_DSI_LANE, REG_DSI_CLK),
> + regmap_reg_range(REG_LVDS_FMT, REG_LVDS_CM),
> + regmap_reg_range(REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
> + REG_VID_CHA_ACTIVE_LINE_LENGTH_HIGH),
> + regmap_reg_range(REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
> + REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH),
> + regmap_reg_range(REG_VID_CHA_SYNC_DELAY_LOW,
> + REG_VID_CHA_SYNC_DELAY_HIGH),
> + regmap_reg_range(REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
> + REG_VID_CHA_HSYNC_PULSE_WIDTH_HIGH),
> + regmap_reg_range(REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
> + REG_VID_CHA_VSYNC_PULSE_WIDTH_HIGH),
> + regmap_reg_range(REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> + REG_VID_CHA_HORIZONTAL_BACK_PORCH),
> + regmap_reg_range(REG_VID_CHA_VERTICAL_BACK_PORCH,
> + REG_VID_CHA_VERTICAL_BACK_PORCH),
> + regmap_reg_range(REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
> + REG_VID_CHA_HORIZONTAL_FRONT_PORCH),
> + regmap_reg_range(REG_VID_CHA_VERTICAL_FRONT_PORCH,
> + REG_VID_CHA_VERTICAL_FRONT_PORCH),
> + regmap_reg_range(REG_VID_CHA_TEST_PATTERN, REG_VID_CHA_TEST_PATTERN),
> + regmap_reg_range(REG_IRQ_GLOBAL, REG_IRQ_EN),
> + regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
> +};
> +
> +static const struct regmap_access_table sn65dsi83_readable_table = {
> + .yes_ranges = sn65dsi83_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(sn65dsi83_readable_ranges),
> +};
> +
> +static const struct regmap_range sn65dsi83_writeable_ranges[] = {
> + regmap_reg_range(REG_RC_RESET, REG_RC_DSI_CLK),
> + regmap_reg_range(REG_RC_PLL_EN, REG_RC_PLL_EN),
> + regmap_reg_range(REG_DSI_LANE, REG_DSI_CLK),
> + regmap_reg_range(REG_LVDS_FMT, REG_LVDS_CM),
> + regmap_reg_range(REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
> + REG_VID_CHA_ACTIVE_LINE_LENGTH_HIGH),
> + regmap_reg_range(REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
> + REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH),
> + regmap_reg_range(REG_VID_CHA_SYNC_DELAY_LOW,
> + REG_VID_CHA_SYNC_DELAY_HIGH),
> + regmap_reg_range(REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
> + REG_VID_CHA_HSYNC_PULSE_WIDTH_HIGH),
> + regmap_reg_range(REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
> + REG_VID_CHA_VSYNC_PULSE_WIDTH_HIGH),
> + regmap_reg_range(REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> + REG_VID_CHA_HORIZONTAL_BACK_PORCH),
> + regmap_reg_range(REG_VID_CHA_VERTICAL_BACK_PORCH,
> + REG_VID_CHA_VERTICAL_BACK_PORCH),
> + regmap_reg_range(REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
> + REG_VID_CHA_HORIZONTAL_FRONT_PORCH),
> + regmap_reg_range(REG_VID_CHA_VERTICAL_FRONT_PORCH,
> + REG_VID_CHA_VERTICAL_FRONT_PORCH),
> + regmap_reg_range(REG_VID_CHA_TEST_PATTERN, REG_VID_CHA_TEST_PATTERN),
> + regmap_reg_range(REG_IRQ_GLOBAL, REG_IRQ_EN),
> + regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
> +};
> +
> +static const struct regmap_access_table sn65dsi83_writeable_table = {
> + .yes_ranges = sn65dsi83_writeable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(sn65dsi83_writeable_ranges),
> +};
> +
> +static const struct regmap_range sn65dsi83_volatile_ranges[] = {
> + regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_LVDS_PLL),
Why is REG_RC_LVDS_PLL volatile?
> + regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
Do you need to list REG_RC_RESET as volatile? Specifically you need
to make sure it's not cached...
> +};
> +
> +static const struct regmap_access_table sn65dsi83_volatile_table = {
> + .yes_ranges = sn65dsi83_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(sn65dsi83_volatile_ranges),
> +};
> +
> +static const struct regmap_config sn65dsi83_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .rd_table = &sn65dsi83_readable_table,
> + .wr_table = &sn65dsi83_writeable_table,
> + .volatile_table = &sn65dsi83_volatile_table,
> + .cache_type = REGCACHE_RBTREE,
> + .max_register = REG_IRQ_STAT,
> +};
I'm curious how much the "readable" and "writable" sections get you.
In theory only the "volatile" should really matter, right?
> +
> +static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct sn65dsi83, bridge);
> +}
> +
> +static int sn65dsi83_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> + struct device *dev = ctx->dev;
> + struct mipi_dsi_device *dsi;
> + struct mipi_dsi_host *host;
> + int ret = 0;
> +
> + const struct mipi_dsi_device_info info = {
> + .type = "sn65dsi83",
> + .channel = 0,
> + .node = NULL,
> + };
> +
> + host = of_find_mipi_dsi_host_by_node(ctx->host_node);
> + if (!host) {
> + dev_err(dev, "failed to find dsi host\n");
I notice that the sn65dsi86 driver usually uses DRM_ERROR instead of dev_err().
> + return -EPROBE_DEFER;
> + }
> +
> + dsi = mipi_dsi_device_register_full(host, &info);
> + if (IS_ERR(dsi)) {
> + ret = PTR_ERR(dsi);
> + dev_err(dev, "failed to create dsi device, ret=%i\n", ret);
> + goto err_dsi_device;
Why not just "return ret" and get rid of the "err_dsi_device" label?
> + }
> +
> + ctx->dsi = dsi;
> +
> + dsi->lanes = ctx->dsi_lanes;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0) {
> + dev_err(dev, "failed to attach dsi to host\n");
> + goto err_dsi_attach;
> + }
> +
> + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
> + &ctx->bridge, flags);
> +
> +err_dsi_attach:
> + mipi_dsi_device_unregister(dsi);
> +err_dsi_device:
> + return ret;
> +}
> +
> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
> +{
> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> + /*
> + * Reset the chip, pull EN line low for t_reset=10ms,
> + * then high for t_en=1ms.
> + */
> + gpiod_set_value(ctx->enable_gpio, 0);
Why not use the "cansleep" version to give some flexibility?
> + usleep_range(10000, 11000);
It seems like it would be worth it to read the enable_gpio first? If
it was already 0 maybe you can skip the 10 ms delay? I would imagine
that most of the time the bridge would already be disabled to start?
> + gpiod_set_value(ctx->enable_gpio, 1);
> + usleep_range(1000, 1100);
This fully resets the chip, right? So you need to invalidate your regmap cache?
> +}
> +
> +static u8 sn65dsi83_get_lvds_range(struct sn65dsi83 *ctx)
> +{
> + /*
> + * The encoding of the LVDS_CLK_RANGE is as follows:
> + * 000 - 25 MHz <= LVDS_CLK < 37.5 MHz
> + * 001 - 37.5 MHz <= LVDS_CLK < 62.5 MHz
> + * 010 - 62.5 MHz <= LVDS_CLK < 87.5 MHz
> + * 011 - 87.5 MHz <= LVDS_CLK < 112.5 MHz
> + * 100 - 112.5 MHz <= LVDS_CLK < 137.5 MHz
> + * 101 - 137.5 MHz <= LVDS_CLK <= 154 MHz
> + * which is a range of 12.5MHz..162.5MHz in 50MHz steps, except that
> + * the ends of the ranges are clamped to the supported range. Since
> + * sn65dsi83_mode_valid() already filters the valid modes and limits
> + * the clock to 25..154 MHz, the range calculation can be simplified
> + * as follows:
> + */
> + return (ctx->mode.clock - 12500) / 25000;
> +}
> +
> +static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx)
> +{
> + /*
> + * The encoding of the CHA_DSI_CLK_RANGE is as follows:
> + * 0x00 through 0x07 - Reserved
> + * 0x08 - 40 <= DSI_CLK < 45 MHz
> + * 0x09 - 45 <= DSI_CLK < 50 MHz
> + * ...
> + * 0x63 - 495 <= DSI_CLK < 500 MHz
> + * 0x64 - 500 MHz
> + * 0x65 through 0xFF - Reserved
> + * which is DSI clock in 5 MHz steps, clamped to 40..500 MHz.
> + * The DSI clock are calculated as:
> + * DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
> + * the 2 is there because the bus is DDR.
> + */
> + return clamp((unsigned int)ctx->mode.clock *
> + mipi_dsi_pixel_format_to_bpp(ctx->dsi->format) /
> + ctx->dsi_lanes / 2, 40000U, 500000U) / 5000U;
> +}
> +
> +static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> +{
> + /* The divider is (DSI_CLK / LVDS_CLK) - 1, which really is: */
> + return (mipi_dsi_pixel_format_to_bpp(ctx->dsi->format) /
> + ctx->dsi_lanes / 2) - 1;
> +}
> +
> +static void sn65dsi83_enable(struct drm_bridge *bridge)
> +{
> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> + u16 val;
> +
> + /* Clear reset, disable PLL */
> + regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
I don't think you need to clear reset, do you? The doc says "This bit
automatically clears when set to 1 and returns 0s when read." Maybe
was needed because you forgot to list this register as volatile?
> + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
Probably you don't need this? It's the default and in pre-enable you
just reset the chip. Maybe it was needed since you don't flush the
cache in pre-enable?
> + /* Reference clock derived from DSI link clock. */
> + regmap_write(ctx->regmap, REG_RC_LVDS_PLL,
> + REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx)) |
> + REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
> + regmap_write(ctx->regmap, REG_DSI_CLK,
> + REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
> + regmap_write(ctx->regmap, REG_RC_DSI_CLK,
> + REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
> +
> + /* Set number of DSI lanes, keep reserved bits. */
> + regmap_write(ctx->regmap, REG_DSI_LANE,
> + REG_DSI_LANE_RESERVED |
> + REG_DSI_LANE_CHA_DSI_LANES(~(ctx->dsi_lanes - 1)));
> + /* No equalization. */
> + regmap_write(ctx->regmap, REG_DSI_EQ, 0x00);
> +
> + /* RGB888 is the only format supported so far. */
> + regmap_write(ctx->regmap, REG_LVDS_FMT,
> + (ctx->mode.flags & DRM_MODE_FLAG_NHSYNC ?
> + REG_LVDS_FMT_HS_NEG_POLARITY : 0) |
> + (ctx->mode.flags & DRM_MODE_FLAG_NVSYNC ?
> + REG_LVDS_FMT_VS_NEG_POLARITY : 0) |
> + REG_LVDS_FMT_RESERVED |
> + REG_LVDS_FMT_CHA_24BPP_MODE);
> + regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
> + regmap_write(ctx->regmap, REG_LVDS_LANE, REG_LVDS_LANE_CHA_LVDS_TERM);
> + regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
> +
> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
> + &ctx->mode.hdisplay, 2);
> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
> + &ctx->mode.vdisplay, 2);
> + val = 32 + 1; /* 32 + 1 pixel clock to ensure proper operation */
> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, &val, 2);
> + val = ctx->mode.hsync_end - ctx->mode.hsync_start;
> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
> + &val, 2);
> + val = ctx->mode.vsync_end - ctx->mode.vsync_start;
> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
> + &val, 2);
> + regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> + ctx->mode.htotal - ctx->mode.hsync_end);
> + regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_BACK_PORCH,
> + ctx->mode.vtotal - ctx->mode.vsync_end);
> + regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
> + ctx->mode.hsync_start - ctx->mode.hdisplay);
> + regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
> + ctx->mode.vsync_start - ctx->mode.vdisplay);
> + regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
> +
> + /* Enable PLL */
> + regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
After you turn on the PLL, I think you should be waiting for
"PLL_EN_STAT" to show that the PLL is actually enabled and then
delaying 3ms, right?
> + /* Trigger reset after CSR register update. */
> + regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
> +}
> +
> +static void sn65dsi83_disable(struct drm_bridge *bridge)
> +{
> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> + /* Clear reset, disable PLL */
> + regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
I don't think you need this--it self-clears.
> + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> +}
> +
> +static void sn65dsi83_post_disable(struct drm_bridge *bridge)
> +{
> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> + /* Put the chip in reset, pull EN line low. */
> + gpiod_set_value(ctx->enable_gpio, 0);
> +}
> +
> +static enum drm_mode_status
> +sn65dsi83_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_info *info,
> + const struct drm_display_mode *mode)
> +{
> + /* LVDS output clock range 25..154 MHz */
> + if (mode->clock < 25000)
> + return MODE_CLOCK_LOW;
> + if (mode->clock > 154000)
> + return MODE_CLOCK_HIGH;
> +
> + return MODE_OK;
> +}
> +
> +static void sn65dsi83_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adj)
> +{
> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> + ctx->mode = *mode;
IIRC you should be using adj, not mode. If something earlier in the
chain tweaked it then adj will be more correct.
> +}
> +
> +static const struct drm_bridge_funcs sn65dsi83_funcs = {
> + .attach = sn65dsi83_attach,
> + .pre_enable = sn65dsi83_pre_enable,
> + .enable = sn65dsi83_enable,
> + .disable = sn65dsi83_disable,
> + .post_disable = sn65dsi83_post_disable,
> + .mode_valid = sn65dsi83_mode_valid,
> + .mode_set = sn65dsi83_mode_set,
> +};
> +
> +static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx)
> +{
> + struct drm_bridge *panel_bridge;
> + struct device *dev = ctx->dev;
> + struct device_node *endpoint;
> + struct drm_panel *panel;
> + int ret;
> +
> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> + ctx->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> + ctx->host_node = of_graph_get_remote_port_parent(endpoint);
> + of_node_put(endpoint);
> + of_node_put(ctx->host_node);
It seems odd that you're storing "ctx->host_node" permanently in your
structures but you're also calling of_node_put() on it. Are you
positive that's correct? I think you need to do the of_node_put() in
the remove function and probably in the error handling of this
function (or use devm to handle it).
> +
> + if (ctx->dsi_lanes < 0 || ctx->dsi_lanes > 4)
> + return -EINVAL;
Should be dsi_lanes <= 0, right?
-Doug
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
2021-02-04 17:15 ` Doug Anderson
@ 2021-02-04 18:09 ` Marek Vasut
-1 siblings, 0 replies; 19+ messages in thread
From: Marek Vasut @ 2021-02-04 18:09 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Laurent Pinchart, Linus Walleij, Sam Ravnborg,
Stephen Boyd,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
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 .
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-02-04 18:09 ` Marek Vasut
0 siblings, 0 replies; 19+ messages in thread
From: Marek Vasut @ 2021-02-04 18:09 UTC (permalink / raw)
To: Doug Anderson
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
dri-devel, Stephen Boyd, Laurent Pinchart, Sam Ravnborg
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 .
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
2021-02-04 18:09 ` Marek Vasut
@ 2021-02-04 18:38 ` Doug Anderson
-1 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2021-02-04 18:38 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Laurent Pinchart, Linus Walleij, Sam Ravnborg,
Stephen Boyd,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-02-04 18:38 ` Doug Anderson
0 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2021-02-04 18:38 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
dri-devel, Stephen Boyd, Laurent Pinchart, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
2021-02-04 17:15 ` [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver Doug Anderson
@ 2021-02-04 18:40 ` Marek Vasut
2021-02-04 21:10 ` Doug Anderson
0 siblings, 1 reply; 19+ messages in thread
From: Marek Vasut @ 2021-02-04 18:40 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Stephen Boyd, Philippe Schenker, Laurent Pinchart,
Valentin Raevsky, Sam Ravnborg
On 2/4/21 6:15 PM, Doug Anderson wrote:
> Hi,
[...]
>> +static const struct regmap_range sn65dsi83_volatile_ranges[] = {
>> + regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_LVDS_PLL),
>
> Why is REG_RC_LVDS_PLL volatile?
See register 0xa bit 7, PLL_EN_STAT .
>> + regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
>
> Do you need to list REG_RC_RESET as volatile? Specifically you need
> to make sure it's not cached...
Isn't volatile table exactly for this purpose -- to make sure the reg is
not cached ?
>> +};
>> +
>> +static const struct regmap_access_table sn65dsi83_volatile_table = {
>> + .yes_ranges = sn65dsi83_volatile_ranges,
>> + .n_yes_ranges = ARRAY_SIZE(sn65dsi83_volatile_ranges),
>> +};
>> +
>> +static const struct regmap_config sn65dsi83_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .rd_table = &sn65dsi83_readable_table,
>> + .wr_table = &sn65dsi83_writeable_table,
>> + .volatile_table = &sn65dsi83_volatile_table,
>> + .cache_type = REGCACHE_RBTREE,
>> + .max_register = REG_IRQ_STAT,
>> +};
>
> I'm curious how much the "readable" and "writable" sections get you.
> In theory only the "volatile" should really matter, right?
They are useful when dumping the regs from debugfs regmap registers .
>> +
>> +static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
>> +{
>> + return container_of(bridge, struct sn65dsi83, bridge);
>> +}
>> +
>> +static int sn65dsi83_attach(struct drm_bridge *bridge,
>> + enum drm_bridge_attach_flags flags)
>> +{
>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> + struct device *dev = ctx->dev;
>> + struct mipi_dsi_device *dsi;
>> + struct mipi_dsi_host *host;
>> + int ret = 0;
>> +
>> + const struct mipi_dsi_device_info info = {
>> + .type = "sn65dsi83",
>> + .channel = 0,
>> + .node = NULL,
>> + };
>> +
>> + host = of_find_mipi_dsi_host_by_node(ctx->host_node);
>> + if (!host) {
>> + dev_err(dev, "failed to find dsi host\n");
>
> I notice that the sn65dsi86 driver usually uses DRM_ERROR instead of dev_err().
I guess you want DRM maintainers to comment on this one. I think simple
dev_err() is clear in what it does, DRM_ERROR seems like another level
of indirection.
>> + return -EPROBE_DEFER;
>> + }
>> +
>> + dsi = mipi_dsi_device_register_full(host, &info);
>> + if (IS_ERR(dsi)) {
>> + ret = PTR_ERR(dsi);
>> + dev_err(dev, "failed to create dsi device, ret=%i\n", ret);
>> + goto err_dsi_device;
>
> Why not just "return ret" and get rid of the "err_dsi_device" label?
Oh, right, or even dev_err_probe().
[...]
>> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
>> +{
>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> +
>> + /*
>> + * Reset the chip, pull EN line low for t_reset=10ms,
>> + * then high for t_en=1ms.
>> + */
>> + gpiod_set_value(ctx->enable_gpio, 0);
>
> Why not use the "cansleep" version to give some flexibility?
Does that make a difference in non-interrupt context ?
>> + usleep_range(10000, 11000);
>
> It seems like it would be worth it to read the enable_gpio first? If
> it was already 0 maybe you can skip the 10 ms delay? I would imagine
> that most of the time the bridge would already be disabled to start?
How do you guarantee the GPIO was LO for 10 mS here? You can sample that
it is LO, but you won't know how long it was LO before this code was
executed.
>> + gpiod_set_value(ctx->enable_gpio, 1);
>> + usleep_range(1000, 1100);
>
> This fully resets the chip, right? So you need to invalidate your regmap cache?
Right
>> +}
>> +
[...]
>> +static void sn65dsi83_enable(struct drm_bridge *bridge)
>> +{
>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> + u16 val;
>> +
>> + /* Clear reset, disable PLL */
>> + regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
>
> I don't think you need to clear reset, do you? The doc says "This bit
> automatically clears when set to 1 and returns 0s when read." Maybe
> was needed because you forgot to list this register as volatile?
I need to check this one.
>> + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
>
> Probably you don't need this? It's the default and in pre-enable you
> just reset the chip. Maybe it was needed since you don't flush the
> cache in pre-enable?
Have a look at the Example Script in the DSI83 datasheet, this PLL part
is needed.
>> + /* Reference clock derived from DSI link clock. */
>> + regmap_write(ctx->regmap, REG_RC_LVDS_PLL,
>> + REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx)) |
>> + REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
>> + regmap_write(ctx->regmap, REG_DSI_CLK,
>> + REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
>> + regmap_write(ctx->regmap, REG_RC_DSI_CLK,
>> + REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
>> +
>> + /* Set number of DSI lanes, keep reserved bits. */
>> + regmap_write(ctx->regmap, REG_DSI_LANE,
>> + REG_DSI_LANE_RESERVED |
>> + REG_DSI_LANE_CHA_DSI_LANES(~(ctx->dsi_lanes - 1)));
>> + /* No equalization. */
>> + regmap_write(ctx->regmap, REG_DSI_EQ, 0x00);
>> +
>> + /* RGB888 is the only format supported so far. */
>> + regmap_write(ctx->regmap, REG_LVDS_FMT,
>> + (ctx->mode.flags & DRM_MODE_FLAG_NHSYNC ?
>> + REG_LVDS_FMT_HS_NEG_POLARITY : 0) |
>> + (ctx->mode.flags & DRM_MODE_FLAG_NVSYNC ?
>> + REG_LVDS_FMT_VS_NEG_POLARITY : 0) |
>> + REG_LVDS_FMT_RESERVED |
>> + REG_LVDS_FMT_CHA_24BPP_MODE);
>> + regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
>> + regmap_write(ctx->regmap, REG_LVDS_LANE, REG_LVDS_LANE_CHA_LVDS_TERM);
>> + regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>> +
>> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
>> + &ctx->mode.hdisplay, 2);
>> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
>> + &ctx->mode.vdisplay, 2);
>> + val = 32 + 1; /* 32 + 1 pixel clock to ensure proper operation */
>> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, &val, 2);
>> + val = ctx->mode.hsync_end - ctx->mode.hsync_start;
>> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
>> + &val, 2);
>> + val = ctx->mode.vsync_end - ctx->mode.vsync_start;
>> + regmap_bulk_write(ctx->regmap, REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
>> + &val, 2);
>> + regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_BACK_PORCH,
>> + ctx->mode.htotal - ctx->mode.hsync_end);
>> + regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_BACK_PORCH,
>> + ctx->mode.vtotal - ctx->mode.vsync_end);
>> + regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
>> + ctx->mode.hsync_start - ctx->mode.hdisplay);
>> + regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
>> + ctx->mode.vsync_start - ctx->mode.vdisplay);
>> + regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
>> +
>> + /* Enable PLL */
>> + regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
>
> After you turn on the PLL, I think you should be waiting for
> "PLL_EN_STAT" to show that the PLL is actually enabled and then
> delaying 3ms, right?
Yes
>> + /* Trigger reset after CSR register update. */
>> + regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
>> +}
>> +
>> +static void sn65dsi83_disable(struct drm_bridge *bridge)
>> +{
>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> +
>> + /* Clear reset, disable PLL */
>> + regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
>
> I don't think you need this--it self-clears.
>
>
>> + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
>> +}
>> +
>> +static void sn65dsi83_post_disable(struct drm_bridge *bridge)
>> +{
>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> +
>> + /* Put the chip in reset, pull EN line low. */
>> + gpiod_set_value(ctx->enable_gpio, 0);
>> +}
>> +
>> +static enum drm_mode_status
>> +sn65dsi83_mode_valid(struct drm_bridge *bridge,
>> + const struct drm_display_info *info,
>> + const struct drm_display_mode *mode)
>> +{
>> + /* LVDS output clock range 25..154 MHz */
>> + if (mode->clock < 25000)
>> + return MODE_CLOCK_LOW;
>> + if (mode->clock > 154000)
>> + return MODE_CLOCK_HIGH;
>> +
>> + return MODE_OK;
>> +}
>> +
>> +static void sn65dsi83_mode_set(struct drm_bridge *bridge,
>> + const struct drm_display_mode *mode,
>> + const struct drm_display_mode *adj)
>> +{
>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> +
>> + ctx->mode = *mode;
>
> IIRC you should be using adj, not mode. If something earlier in the
> chain tweaked it then adj will be more correct.
OK
[...]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
2021-02-04 18:38 ` Doug Anderson
@ 2021-02-04 18:46 ` Marek Vasut
-1 siblings, 0 replies; 19+ messages in thread
From: Marek Vasut @ 2021-02-04 18:46 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Laurent Pinchart, Linus Walleij, Sam Ravnborg,
Stephen Boyd,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
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 ?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-02-04 18:46 ` Marek Vasut
0 siblings, 0 replies; 19+ messages in thread
From: Marek Vasut @ 2021-02-04 18:46 UTC (permalink / raw)
To: Doug Anderson
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
dri-devel, Stephen Boyd, Laurent Pinchart, Sam Ravnborg
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 ?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
2021-02-04 18:40 ` Marek Vasut
@ 2021-02-04 21:10 ` Doug Anderson
2021-02-04 22:05 ` Marek Vasut
0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2021-02-04 21:10 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Stephen Boyd, Philippe Schenker, Laurent Pinchart,
Valentin Raevsky, Sam Ravnborg
Hi,
On Thu, Feb 4, 2021 at 10:41 AM Marek Vasut <marex@denx.de> wrote:
>
> >> +static const struct regmap_range sn65dsi83_volatile_ranges[] = {
> >> + regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_LVDS_PLL),
> >
> > Why is REG_RC_LVDS_PLL volatile?
>
> See register 0xa bit 7, PLL_EN_STAT .
Wow, I looked at it a few times and still didn't see it. OK, fair enough.
> >> + regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
> >
> > Do you need to list REG_RC_RESET as volatile? Specifically you need
> > to make sure it's not cached...
>
> Isn't volatile table exactly for this purpose -- to make sure the reg is
> not cached ?
Sorry, I was unclear I guess. I'm suggesting that you add
REG_RC_RESET to the list of volatile ones since I don't see it there.
> >> +static const struct regmap_config sn65dsi83_regmap_config = {
> >> + .reg_bits = 8,
> >> + .val_bits = 8,
> >> + .rd_table = &sn65dsi83_readable_table,
> >> + .wr_table = &sn65dsi83_writeable_table,
> >> + .volatile_table = &sn65dsi83_volatile_table,
> >> + .cache_type = REGCACHE_RBTREE,
> >> + .max_register = REG_IRQ_STAT,
> >> +};
> >
> > I'm curious how much the "readable" and "writable" sections get you.
> > In theory only the "volatile" should really matter, right?
>
> They are useful when dumping the regs from debugfs regmap registers .
OK, fair enough. When I thought about doing this on sn65dsi86, it
came to be that a better way might be something like:
#define ACC_RO BIT(0)
#define ACC_RW BIT(1)
#define ACC_W1C BIT(2)
#define ACC_WO BIT(3)
u8 reg_acceess[] = {
[0x00] = ACC_RO,
[0x01] = ACC_RO,
...
[0x0a] = ACC_RO | ACC_RW,
[0x0b] = ACC_RW,
[0x0d] = ACC_RW
...
};
The above maps really nicely to the public datasheet and is easy to
validate. Then you can just look up in that array in a constant time
lookup. In other words, "readable" if either RO or RW is set.
"writable" if any of RW, W1C, or WO is set. Everything that's not RW
is volatile (technically you could differentiate between RO things
that are hardcoded and ones that aren't, but you probably don't need
to).
Anyway, feel free to ignore... What you have is fine too.
> >> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
> >> +{
> >> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> >> +
> >> + /*
> >> + * Reset the chip, pull EN line low for t_reset=10ms,
> >> + * then high for t_en=1ms.
> >> + */
> >> + gpiod_set_value(ctx->enable_gpio, 0);
> >
> > Why not use the "cansleep" version to give some flexibility?
>
> Does that make a difference in non-interrupt context ?
As I understand it:
* If a client calls gpiod_set_value() then the underlying GPIO can
only be one that doesn't sleep.
* If a client calls gpiod_set_value_cansleep() then the underlying
GPIO can be either one that does or doesn't sleep.
* A client is only allowed to call gpiod_set_value_cansleep() if it's
not in interrupt context.
You are definitely not in an interrupt context (right?), so calling
the "cansleep" version has no downsides but allows board designers to
hook up an enable that can sleep.
> >> + usleep_range(10000, 11000);
> >
> > It seems like it would be worth it to read the enable_gpio first? If
> > it was already 0 maybe you can skip the 10 ms delay? I would imagine
> > that most of the time the bridge would already be disabled to start?
>
> How do you guarantee the GPIO was LO for 10 mS here? You can sample that
> it is LO, but you won't know how long it was LO before this code was
> executed.
Ah, true. I guess the best we could do would be keep track of the
GPIO ourselves so that if we were the one to last turn it off we could
avoid the delay.
> >> + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> >
> > Probably you don't need this? It's the default and in pre-enable you
> > just reset the chip. Maybe it was needed since you don't flush the
> > cache in pre-enable?
>
> Have a look at the Example Script in the DSI83 datasheet, this PLL part
> is needed.
I think that script is written without the assumption that you have
just reset the chip using the enable GPIO. If you have just reset
with the enable GPIO it shouldn't be needed.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
2021-02-04 21:10 ` Doug Anderson
@ 2021-02-04 22:05 ` Marek Vasut
2021-02-12 8:43 ` Linus Walleij
0 siblings, 1 reply; 19+ messages in thread
From: Marek Vasut @ 2021-02-04 22:05 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Stephen Boyd, Philippe Schenker, Laurent Pinchart,
Valentin Raevsky, Sam Ravnborg
On 2/4/21 10:10 PM, Doug Anderson wrote:
> Hi,
Hi,
[...]
>>>> + regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
>>>
>>> Do you need to list REG_RC_RESET as volatile? Specifically you need
>>> to make sure it's not cached...
>>
>> Isn't volatile table exactly for this purpose -- to make sure the reg is
>> not cached ?
>
> Sorry, I was unclear I guess. I'm suggesting that you add
> REG_RC_RESET to the list of volatile ones since I don't see it there.
Ah, yes, it should.
>>>> +static const struct regmap_config sn65dsi83_regmap_config = {
>>>> + .reg_bits = 8,
>>>> + .val_bits = 8,
>>>> + .rd_table = &sn65dsi83_readable_table,
>>>> + .wr_table = &sn65dsi83_writeable_table,
>>>> + .volatile_table = &sn65dsi83_volatile_table,
>>>> + .cache_type = REGCACHE_RBTREE,
>>>> + .max_register = REG_IRQ_STAT,
>>>> +};
>>>
>>> I'm curious how much the "readable" and "writable" sections get you.
>>> In theory only the "volatile" should really matter, right?
>>
>> They are useful when dumping the regs from debugfs regmap registers .
>
> OK, fair enough. When I thought about doing this on sn65dsi86, it
> came to be that a better way might be something like:
>
> #define ACC_RO BIT(0)
> #define ACC_RW BIT(1)
> #define ACC_W1C BIT(2)
> #define ACC_WO BIT(3)
>
> u8 reg_acceess[] = {
> [0x00] = ACC_RO,
> [0x01] = ACC_RO,
> ...
> [0x0a] = ACC_RO | ACC_RW,
> [0x0b] = ACC_RW,
> [0x0d] = ACC_RW
> ...
> };
>
> The above maps really nicely to the public datasheet and is easy to
> validate. Then you can just look up in that array in a constant time
> lookup. In other words, "readable" if either RO or RW is set.
> "writable" if any of RW, W1C, or WO is set. Everything that's not RW
> is volatile (technically you could differentiate between RO things
> that are hardcoded and ones that aren't, but you probably don't need
> to).
>
> Anyway, feel free to ignore... What you have is fine too.
It might make sense to implement some more generic support for this ^
into the regmap core ? This seems to be a rather common pattern.
>>>> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
>>>> +{
>>>> + struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>>>> +
>>>> + /*
>>>> + * Reset the chip, pull EN line low for t_reset=10ms,
>>>> + * then high for t_en=1ms.
>>>> + */
>>>> + gpiod_set_value(ctx->enable_gpio, 0);
>>>
>>> Why not use the "cansleep" version to give some flexibility?
>>
>> Does that make a difference in non-interrupt context ?
>
> As I understand it:
>
> * If a client calls gpiod_set_value() then the underlying GPIO can
> only be one that doesn't sleep.
>
> * If a client calls gpiod_set_value_cansleep() then the underlying
> GPIO can be either one that does or doesn't sleep.
>
> * A client is only allowed to call gpiod_set_value_cansleep() if it's
> not in interrupt context.
>
> You are definitely not in an interrupt context (right?), so calling
> the "cansleep" version has no downsides but allows board designers to
> hook up an enable that can sleep.
Linus, can you please confirm this ? I find this hard to believe, since
there are plenty of places in the kernel which use gpiod_set_value()
without the _cansleep, are those problematic then ?
>>>> + usleep_range(10000, 11000);
>>>
>>> It seems like it would be worth it to read the enable_gpio first? If
>>> it was already 0 maybe you can skip the 10 ms delay? I would imagine
>>> that most of the time the bridge would already be disabled to start?
>>
>> How do you guarantee the GPIO was LO for 10 mS here? You can sample that
>> it is LO, but you won't know how long it was LO before this code was
>> executed.
>
> Ah, true. I guess the best we could do would be keep track of the
> GPIO ourselves so that if we were the one to last turn it off we could
> avoid the delay.
Does the extra complexity outweigh the benefit of saving those 10mS ?
>>>> + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
>>>
>>> Probably you don't need this? It's the default and in pre-enable you
>>> just reset the chip. Maybe it was needed since you don't flush the
>>> cache in pre-enable?
>>
>> Have a look at the Example Script in the DSI83 datasheet, this PLL part
>> is needed.
>
> I think that script is written without the assumption that you have
> just reset the chip using the enable GPIO. If you have just reset
> with the enable GPIO it shouldn't be needed.
I don't think it hurts anything, so let's keep it.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
2021-01-30 18:10 [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings Marek Vasut
@ 2021-02-09 20:19 ` Rob Herring
2021-02-04 17:15 ` Doug Anderson
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2021-02-09 20:19 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Douglas Anderson, Laurent Pinchart, Linus Walleij,
Sam Ravnborg, Stephen Boyd, devicetree
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
@ 2021-02-09 20:19 ` Rob Herring
0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2021-02-09 20:19 UTC (permalink / raw)
To: Marek Vasut
Cc: devicetree, Douglas Anderson, dri-devel, Stephen Boyd,
Laurent Pinchart, Sam Ravnborg
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
2021-02-04 22:05 ` Marek Vasut
@ 2021-02-12 8:43 ` Linus Walleij
2021-02-12 10:31 ` Marek Vasut
0 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-02-12 8:43 UTC (permalink / raw)
To: Marek Vasut
Cc: Doug Anderson, dri-devel, Stephen Boyd, Philippe Schenker,
Laurent Pinchart, Valentin Raevsky, Sam Ravnborg
On Thu, Feb 4, 2021 at 11:05 PM Marek Vasut <marex@denx.de> wrote:
> On 2/4/21 10:10 PM, Doug Anderson wrote:
> >>>> + /*
> >>>> + * Reset the chip, pull EN line low for t_reset=10ms,
> >>>> + * then high for t_en=1ms.
> >>>> + */
> >>>> + gpiod_set_value(ctx->enable_gpio, 0);
> >>>
> >>> Why not use the "cansleep" version to give some flexibility?
> >>
> >> Does that make a difference in non-interrupt context ?
> >
> > As I understand it:
> >
> > * If a client calls gpiod_set_value() then the underlying GPIO can
> > only be one that doesn't sleep.
> >
> > * If a client calls gpiod_set_value_cansleep() then the underlying
> > GPIO can be either one that does or doesn't sleep.
> >
> > * A client is only allowed to call gpiod_set_value_cansleep() if it's
> > not in interrupt context.
> >
> > You are definitely not in an interrupt context (right?), so calling
> > the "cansleep" version has no downsides but allows board designers to
> > hook up an enable that can sleep.
>
> Linus, can you please confirm this ? I find this hard to believe, since
> there are plenty of places in the kernel which use gpiod_set_value()
> without the _cansleep, are those problematic then ?
The function looks like so:
void gpiod_set_value(struct gpio_desc *desc, int value)
{
VALIDATE_DESC_VOID(desc);
/* Should be using gpiod_set_value_cansleep() */
WARN_ON(desc->gdev->chip->can_sleep);
gpiod_set_value_nocheck(desc, value);
}
So if the chip has set ->can_sleep (i,e, this GPIO is on something
like an I2C bus) then a warning will appear.
The warning only really appear if you have a driver that was
previously only used on a gpiochip with "fast" (write a register)
GPIOs and somebody start to use the same driver with a
GPIO on e.g. an I2C bus, which will define ->can_sleep.
The simple solution to the warning is to switch to the
_cansleep() variant but really it is a sign to check that
this is not being called in atomic context and just check
that the driver overall will survive sleeping context
here.
In a way _cansleep() is just syntactic sugar.
In this driver, you can use _cansleep() if you like but not
using it mostly works too, if used with SoC-type GPIOs.
Whoever uses the driver with a GPIO on an I2C chip
or similar gets to fix it.
Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
2021-02-12 8:43 ` Linus Walleij
@ 2021-02-12 10:31 ` Marek Vasut
0 siblings, 0 replies; 19+ messages in thread
From: Marek Vasut @ 2021-02-12 10:31 UTC (permalink / raw)
To: Linus Walleij
Cc: Doug Anderson, dri-devel, Stephen Boyd, Philippe Schenker,
Laurent Pinchart, Valentin Raevsky, Sam Ravnborg
On 2/12/21 9:43 AM, Linus Walleij wrote:
> On Thu, Feb 4, 2021 at 11:05 PM Marek Vasut <marex@denx.de> wrote:
>> On 2/4/21 10:10 PM, Doug Anderson wrote:
>
>>>>>> + /*
>>>>>> + * Reset the chip, pull EN line low for t_reset=10ms,
>>>>>> + * then high for t_en=1ms.
>>>>>> + */
>>>>>> + gpiod_set_value(ctx->enable_gpio, 0);
>>>>>
>>>>> Why not use the "cansleep" version to give some flexibility?
>>>>
>>>> Does that make a difference in non-interrupt context ?
>>>
>>> As I understand it:
>>>
>>> * If a client calls gpiod_set_value() then the underlying GPIO can
>>> only be one that doesn't sleep.
>>>
>>> * If a client calls gpiod_set_value_cansleep() then the underlying
>>> GPIO can be either one that does or doesn't sleep.
>>>
>>> * A client is only allowed to call gpiod_set_value_cansleep() if it's
>>> not in interrupt context.
>>>
>>> You are definitely not in an interrupt context (right?), so calling
>>> the "cansleep" version has no downsides but allows board designers to
>>> hook up an enable that can sleep.
>>
>> Linus, can you please confirm this ? I find this hard to believe, since
>> there are plenty of places in the kernel which use gpiod_set_value()
>> without the _cansleep, are those problematic then ?
>
> The function looks like so:
>
> void gpiod_set_value(struct gpio_desc *desc, int value)
> {
> VALIDATE_DESC_VOID(desc);
> /* Should be using gpiod_set_value_cansleep() */
> WARN_ON(desc->gdev->chip->can_sleep);
> gpiod_set_value_nocheck(desc, value);
> }
>
> So if the chip has set ->can_sleep (i,e, this GPIO is on something
> like an I2C bus) then a warning will appear.
>
> The warning only really appear if you have a driver that was
> previously only used on a gpiochip with "fast" (write a register)
> GPIOs and somebody start to use the same driver with a
> GPIO on e.g. an I2C bus, which will define ->can_sleep.
>
> The simple solution to the warning is to switch to the
> _cansleep() variant but really it is a sign to check that
> this is not being called in atomic context and just check
> that the driver overall will survive sleeping context
> here.
>
> In a way _cansleep() is just syntactic sugar.
>
> In this driver, you can use _cansleep() if you like but not
> using it mostly works too, if used with SoC-type GPIOs.
> Whoever uses the driver with a GPIO on an I2C chip
> or similar gets to fix it.
Nice, I'm gonna archive this, this explanation is very clear, thanks.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-02-12 10:31 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-30 18:10 [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings Marek Vasut
2021-02-02 19:41 ` Linus Walleij
2021-02-02 19:41 ` Linus Walleij
2021-02-04 17:15 ` Doug Anderson
2021-02-04 17:15 ` Doug Anderson
2021-02-04 18:09 ` Marek Vasut
2021-02-04 18:09 ` Marek Vasut
2021-02-04 18:38 ` Doug Anderson
2021-02-04 18:38 ` Doug Anderson
2021-02-04 18:46 ` Marek Vasut
2021-02-04 18:46 ` Marek Vasut
[not found] ` <20210130181014.161457-2-marex@denx.de>
2021-02-04 17:15 ` [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver Doug Anderson
2021-02-04 18:40 ` Marek Vasut
2021-02-04 21:10 ` Doug Anderson
2021-02-04 22:05 ` Marek Vasut
2021-02-12 8:43 ` Linus Walleij
2021-02-12 10:31 ` Marek Vasut
2021-02-09 20:19 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings Rob Herring
2021-02-09 20:19 ` Rob Herring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.