[PATCH v5 0/9] Add TOSHIBA TC358764 DSI/LVDS bridge driver

[PATCH v5 0/9] Add TOSHIBA TC358764 DSI/LVDS bridge driver

[Andrzej Hajda]

Hi,

This is continuation of Maciej patchset. I took it over since he can't work
on it in near future.

I have removed panel patches as they are already merged (thanks Thierry),
but I have added fix for timings to allow refresh rate 60Hz.
I have addressed all comments from reviewers - thanks Inki, thanks me :)

All changes are described in the patches.

Regards
Andrzej


Andrzej Hajda (6):
  dt-bindings: tc358754: add DT bindings
  drm/bridge: tc358764: Add DSI to LVDS bridge driver
  ARM: dts: exynos5250: add DSI node
  ARM: dts: exynos5250-arndale: add DSI and panel nodes
  drm/panel: simple: fix BOE/HV070WSA-100 timings
  dt-bindings: exynos_dsim: update of graph bindings

Maciej Purski (3):
  drm/exynos: rename bridge_node to in_bridge_node
  drm/exynos: move connector creation to attach callback
  drm/exynos: enable out_bridge in exynos_dsi_enable

 .../display/bridge/toshiba,tc358764.txt       |  35 ++
 .../bindings/display/exynos/exynos_dsim.txt   |  25 +-
 arch/arm/boot/dts/exynos5250-arndale.dts      |  61 +++
 arch/arm/boot/dts/exynos5250.dtsi             |  21 +
 drivers/gpu/drm/bridge/Kconfig                |   8 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/tc358764.c             | 499 ++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_dsi.c       | 104 ++--
 drivers/gpu/drm/panel/panel-simple.c          |  14 +-
 9 files changed, 701 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
 create mode 100644 drivers/gpu/drm/bridge/tc358764.c

-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 1/9] drm/exynos: rename bridge_node to in_bridge_node

[Andrzej Hajda]

From: Maciej Purski <m.purski@samsung.com>

Driver uses bridge_node to refer to bridge on input side of DSI.
Since we want to add support for bridges on output side lets add
"in" prefix to avoid confusion with out bridges.

Changes in v5:
- replace mic_ prefix with in_

Signed-off-by: Maciej Purski <m.purski@samsung.com>
[ a.hajda@samsuung.com: v5 ]
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 7c3030b7e586..351403f9d245 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -279,7 +279,7 @@ struct exynos_dsi {
 	struct list_head transfer_list;
 
 	const struct exynos_dsi_driver_data *driver_data;
-	struct device_node *bridge_node;
+	struct device_node *in_bridge_node;
 };
 
 #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
@@ -1631,7 +1631,7 @@ static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
 	if (ret < 0)
 		return ret;
 
-	dsi->bridge_node = of_graph_get_remote_node(node, DSI_PORT_IN, 0);
+	dsi->in_bridge_node = of_graph_get_remote_node(node, DSI_PORT_IN, 0);
 
 	return 0;
 }
@@ -1642,7 +1642,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 	struct drm_encoder *encoder = dev_get_drvdata(dev);
 	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
 	struct drm_device *drm_dev = data;
-	struct drm_bridge *bridge;
+	struct drm_bridge *in_bridge;
 	int ret;
 
 	drm_encoder_init(drm_dev, encoder, &exynos_dsi_encoder_funcs,
@@ -1661,10 +1661,10 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	if (dsi->bridge_node) {
-		bridge = of_drm_find_bridge(dsi->bridge_node);
-		if (bridge)
-			drm_bridge_attach(encoder, bridge, NULL);
+	if (dsi->in_bridge_node) {
+		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
+		if (in_bridge)
+			drm_bridge_attach(encoder, in_bridge, NULL);
 	}
 
 	return mipi_dsi_host_register(&dsi->dsi_host);
@@ -1783,7 +1783,7 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 {
 	struct exynos_dsi *dsi = platform_get_drvdata(pdev);
 
-	of_node_put(dsi->bridge_node);
+	of_node_put(dsi->in_bridge_node);
 
 	pm_runtime_disable(&pdev->dev);
 
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Andrzej Hajda]

From: Maciej Purski <m.purski@samsung.com>

The current implementation assumes that the only possible peripheral
device for DSIM is a panel. Using an output bridge child device
should also be possible.

If an output bridge is available, don't create a new connector.
Instead, call drm_bridge_attach() and set encoder's bridge to NULL
in order to avoid an out bridge from being visible by the framework, as
the DSI bus needs control on enabling its child output bridge.

Such sequence is required by Toshiba TC358764 bridge, which is a DSI
peripheral bridge device.

changed in v5:
- detach bridge in mipi_dsi detach callback

Signed-off-by: Maciej Purski <m.purski@samsung.com>
[ a.hajda@samsung.com: v5 ]
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 351403f9d245..f5f51f584fa0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -255,6 +255,7 @@ struct exynos_dsi {
 	struct mipi_dsi_host dsi_host;
 	struct drm_connector connector;
 	struct drm_panel *panel;
+	struct drm_bridge *out_bridge;
 	struct device *dev;
 
 	void __iomem *reg_base;
@@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
-	struct drm_device *drm = dsi->connector.dev;
+	struct drm_encoder *encoder = &dsi->encoder;
+	struct drm_device *drm = encoder->dev;
+	struct drm_bridge *out_bridge;
+
+	out_bridge  = of_drm_find_bridge(device->dev.of_node);
+	if (out_bridge) {
+		drm_bridge_attach(encoder, out_bridge, NULL);
+		dsi->out_bridge = out_bridge;
+		encoder->bridge = NULL;
+	} else {
+		int ret = exynos_dsi_create_connector(encoder);
+
+		if (ret) {
+			DRM_ERROR("failed to create connector ret = %d\n", ret);
+			drm_encoder_cleanup(encoder);
+			return ret;
+		}
+
+		dsi->panel = of_drm_find_panel(device->dev.of_node);
+		if (dsi->panel) {
+			drm_panel_attach(dsi->panel, &dsi->connector);
+			dsi->connector.status = connector_status_connected;
+		}
+	}
 
 	/*
 	 * This is a temporary solution and should be made by more generic way.
@@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (dsi->panel) {
-		drm_panel_attach(dsi->panel, &dsi->connector);
-		dsi->connector.status = connector_status_connected;
-	}
 	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
 			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
 
@@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
-	struct drm_device *drm = dsi->connector.dev;
-
-	mutex_lock(&drm->mode_config.mutex);
+	struct drm_device *drm = dsi->encoder.dev;
 
 	if (dsi->panel) {
+		mutex_lock(&drm->mode_config.mutex);
 		exynos_dsi_disable(&dsi->encoder);
 		drm_panel_detach(dsi->panel);
 		dsi->panel = NULL;
 		dsi->connector.status = connector_status_disconnected;
+		mutex_unlock(&drm->mode_config.mutex);
+	} else {
+		if (dsi->out_bridge->funcs->detach)
+			dsi->out_bridge->funcs->detach(dsi->out_bridge);
+		dsi->out_bridge = NULL;
 	}
 
-	mutex_unlock(&drm->mode_config.mutex);
-
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
 
@@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 	if (ret < 0)
 		return ret;
 
-	ret = exynos_dsi_create_connector(encoder);
-	if (ret) {
-		DRM_ERROR("failed to create connector ret = %d\n", ret);
-		drm_encoder_cleanup(encoder);
-		return ret;
-	}
-
 	if (dsi->in_bridge_node) {
 		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
 		if (in_bridge)
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 3/9] drm/exynos: enable out_bridge in exynos_dsi_enable

[Andrzej Hajda]

From: Maciej Purski <m.purski@samsung.com>

As the out bridge will not be enabled directly by the framework,
it should be enabled by DSI. exynos_dsi_enable() should handle a case,
when there is an out_bridge connected as a DSI peripheral.

Changed in v5:
- fixed error path in exynos_dsi_enable

Signed-off-by: Maciej Purski <m.purski@samsung.com>
[ a.hajda@samsung.com: v5 ]
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 38 +++++++++++++++----------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index f5f51f584fa0..54cfcebe7a2c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1383,29 +1383,37 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 		return;
 
 	pm_runtime_get_sync(dsi->dev);
-
 	dsi->state |= DSIM_STATE_ENABLED;
 
-	ret = drm_panel_prepare(dsi->panel);
-	if (ret < 0) {
-		dsi->state &= ~DSIM_STATE_ENABLED;
-		pm_runtime_put_sync(dsi->dev);
-		return;
+	if (dsi->panel) {
+		ret = drm_panel_prepare(dsi->panel);
+		if (ret < 0)
+			goto err_put_sync;
+	} else {
+		drm_bridge_pre_enable(dsi->out_bridge);
 	}
 
 	exynos_dsi_set_display_mode(dsi);
 	exynos_dsi_set_display_enable(dsi, true);
 
-	ret = drm_panel_enable(dsi->panel);
-	if (ret < 0) {
-		dsi->state &= ~DSIM_STATE_ENABLED;
-		exynos_dsi_set_display_enable(dsi, false);
-		drm_panel_unprepare(dsi->panel);
-		pm_runtime_put_sync(dsi->dev);
-		return;
+	if (dsi->panel) {
+		ret = drm_panel_enable(dsi->panel);
+		if (ret < 0)
+			goto err_display_disable;
+	} else {
+		drm_bridge_enable(dsi->out_bridge);
 	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
+	return;
+
+err_display_disable:
+	exynos_dsi_set_display_enable(dsi, false);
+	drm_panel_unprepare(dsi->panel);
+
+err_put_sync:
+	dsi->state &= ~DSIM_STATE_ENABLED;
+	pm_runtime_put(dsi->dev);
 }
 
 static void exynos_dsi_disable(struct drm_encoder *encoder)
@@ -1418,11 +1426,11 @@ static void exynos_dsi_disable(struct drm_encoder *encoder)
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 
 	drm_panel_disable(dsi->panel);
+	drm_bridge_disable(dsi->out_bridge);
 	exynos_dsi_set_display_enable(dsi, false);
 	drm_panel_unprepare(dsi->panel);
-
+	drm_bridge_post_disable(dsi->out_bridge);
 	dsi->state &= ~DSIM_STATE_ENABLED;
-
 	pm_runtime_put_sync(dsi->dev);
 }
 
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 4/9] dt-bindings: tc358754: add DT bindings

[Andrzej Hajda]

The patch adds bindings to Toshiba DSI/LVDS bridge TC358764.
Bindings describe power supplies, reset gpio and video interfaces.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Maciej Purski <m.purski@samsung.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../display/bridge/toshiba,tc358764.txt       | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
new file mode 100644
index 000000000000..8f9abf28a8fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
@@ -0,0 +1,35 @@
+TC358764 MIPI-DSI to LVDS panel bridge
+
+Required properties:
+  - compatible: "toshiba,tc358764"
+  - reg: the virtual channel number of a DSI peripheral
+  - vddc-supply: core voltage supply, 1.2V
+  - vddio-supply: I/O voltage supply, 1.8V or 3.3V
+  - vddlvds-supply: LVDS1/2 voltage supply, 3.3V
+  - reset-gpios: a GPIO spec for the reset pin
+
+The device node can contain following 'port' child nodes,
+according to the OF graph bindings defined in [1]:
+  0: DSI Input, not required, if the bridge is DSI controlled
+  1: LVDS Output, mandatory
+
+[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+	bridge@0 {
+		reg = <0>;
+		compatible = "toshiba,tc358764";
+		vddc-supply = <&vcc_1v2_reg>;
+		vddio-supply = <&vcc_1v8_reg>;
+		vddlvds-supply = <&vcc_3v3_reg>;
+		reset-gpios = <&gpd1 6 GPIO_ACTIVE_LOW>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port@1 {
+			reg = <1>;
+			lvds_ep: endpoint {
+				remote-endpoint = <&panel_ep>;
+			};
+		};
+	};
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 5/9] drm/bridge: tc358764: Add DSI to LVDS bridge driver

[Andrzej Hajda]

Add a drm_bridge driver for the Toshiba TC358764 DSI to LVDS bridge.

Changes in v4:
- removed license blob,
- ordered includes,
- added error handling,
- fixed reset GPIO handling,
- added missing calls to the panel,
- custom OF graph code replaced with helpers,
- removed tc358764_poweroff from remove callback.
v5:
- fixed supply names,
- fixed broken console - added connector to fb_helper,
- added detach callback - unbinding works,
- fixed typo in error checking code,
- removed sparse bridge->encoder check - core does it already.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Maciej Purski <m.purski@samsung.com>
[ a.hajda@samsung.com: v4, v5 ]
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/Kconfig    |   8 +
 drivers/gpu/drm/bridge/Makefile   |   1 +
 drivers/gpu/drm/bridge/tc358764.c | 499 ++++++++++++++++++++++++++++++
 3 files changed, 508 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/tc358764.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index fa2c7997e2fd..f3da8a716833 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -110,6 +110,14 @@ config DRM_THINE_THC63LVD1024
 	---help---
 	  Thine THC63LVD1024 LVDS/parallel converter driver.
 
+config DRM_TOSHIBA_TC358764
+	tristate "TC358764 DSI/LVDS bridge"
+	depends on DRM && DRM_PANEL
+	depends on OF
+	select DRM_MIPI_DSI
+	help
+	  Toshiba TC358764 DSI/LVDS bridge driver.
+
 config DRM_TOSHIBA_TC358767
 	tristate "Toshiba TC358767 eDP bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 35f88d48ec20..bf7c0cecf227 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_SII9234) += sii9234.o
 obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
+obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
new file mode 100644
index 000000000000..779bc5fce22a
--- /dev/null
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Samsung Electronics Co., Ltd
+ *
+ * Authors:
+ *	Andrzej Hajda <a.hajda@samsung.com>
+ *	Maciej Purski <m.purski@samsung.com>
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drmP.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <video/mipi_display.h>
+
+#define FLD_MASK(start, end)    (((1 << ((start) - (end) + 1)) - 1) << (end))
+#define FLD_VAL(val, start, end) (((val) << (end)) & FLD_MASK(start, end))
+
+/* PPI layer registers */
+#define PPI_STARTPPI		0x0104 /* START control bit */
+#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
+#define PPI_LANEENABLE		0x0134 /* Enables each lane */
+#define PPI_TX_RX_TA		0x013C /* BTA timing parameters */
+#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
+#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
+#define PPI_D2S_CLRSIPOCOUNT	0x016C /* Assertion timer for Lane 2 */
+#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
+#define PPI_START_FUNCTION	1
+
+/* DSI layer registers */
+#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
+#define DSI_LANEENABLE		0x0210 /* Enables each lane */
+#define DSI_RX_START		1
+
+/* Video path registers */
+#define VP_CTRL			0x0450 /* Video Path Control */
+#define VP_CTRL_MSF(v)		FLD_VAL(v, 0, 0) /* Magic square in RGB666 */
+#define VP_CTRL_VTGEN(v)	FLD_VAL(v, 4, 4) /* Use chip clock for timing */
+#define VP_CTRL_EVTMODE(v)	FLD_VAL(v, 5, 5) /* Event mode */
+#define VP_CTRL_RGB888(v)	FLD_VAL(v, 8, 8) /* RGB888 mode */
+#define VP_CTRL_VSDELAY(v)	FLD_VAL(v, 31, 20) /* VSYNC delay */
+#define VP_CTRL_HSPOL		BIT(17) /* Polarity of HSYNC signal */
+#define VP_CTRL_DEPOL		BIT(18) /* Polarity of DE signal */
+#define VP_CTRL_VSPOL		BIT(19) /* Polarity of VSYNC signal */
+#define VP_HTIM1		0x0454 /* Horizontal Timing Control 1 */
+#define VP_HTIM1_HBP(v)		FLD_VAL(v, 24, 16)
+#define VP_HTIM1_HSYNC(v)	FLD_VAL(v, 8, 0)
+#define VP_HTIM2		0x0458 /* Horizontal Timing Control 2 */
+#define VP_HTIM2_HFP(v)		FLD_VAL(v, 24, 16)
+#define VP_HTIM2_HACT(v)	FLD_VAL(v, 10, 0)
+#define VP_VTIM1		0x045C /* Vertical Timing Control 1 */
+#define VP_VTIM1_VBP(v)		FLD_VAL(v, 23, 16)
+#define VP_VTIM1_VSYNC(v)	FLD_VAL(v, 7, 0)
+#define VP_VTIM2		0x0460 /* Vertical Timing Control 2 */
+#define VP_VTIM2_VFP(v)		FLD_VAL(v, 23, 16)
+#define VP_VTIM2_VACT(v)	FLD_VAL(v, 10, 0)
+#define VP_VFUEN		0x0464 /* Video Frame Timing Update Enable */
+
+/* LVDS registers */
+#define LV_MX0003		0x0480 /* Mux input bit 0 to 3 */
+#define LV_MX0407		0x0484 /* Mux input bit 4 to 7 */
+#define LV_MX0811		0x0488 /* Mux input bit 8 to 11 */
+#define LV_MX1215		0x048C /* Mux input bit 12 to 15 */
+#define LV_MX1619		0x0490 /* Mux input bit 16 to 19 */
+#define LV_MX2023		0x0494 /* Mux input bit 20 to 23 */
+#define LV_MX2427		0x0498 /* Mux input bit 24 to 27 */
+#define LV_MX(b0, b1, b2, b3)	(FLD_VAL(b0, 4, 0) | FLD_VAL(b1, 12, 8) | \
+				FLD_VAL(b2, 20, 16) | FLD_VAL(b3, 28, 24))
+
+/* Input bit numbers used in mux registers */
+enum {
+	LVI_R0,
+	LVI_R1,
+	LVI_R2,
+	LVI_R3,
+	LVI_R4,
+	LVI_R5,
+	LVI_R6,
+	LVI_R7,
+	LVI_G0,
+	LVI_G1,
+	LVI_G2,
+	LVI_G3,
+	LVI_G4,
+	LVI_G5,
+	LVI_G6,
+	LVI_G7,
+	LVI_B0,
+	LVI_B1,
+	LVI_B2,
+	LVI_B3,
+	LVI_B4,
+	LVI_B5,
+	LVI_B6,
+	LVI_B7,
+	LVI_HS,
+	LVI_VS,
+	LVI_DE,
+	LVI_L0
+};
+
+#define LV_CFG			0x049C /* LVDS Configuration */
+#define LV_PHY0			0x04A0 /* LVDS PHY 0 */
+#define LV_PHY0_RST(v)		FLD_VAL(v, 22, 22) /* PHY reset */
+#define LV_PHY0_IS(v)		FLD_VAL(v, 15, 14)
+#define LV_PHY0_ND(v)		FLD_VAL(v, 4, 0) /* Frequency range select */
+#define LV_PHY0_PRBS_ON(v)	FLD_VAL(v, 20, 16) /* Clock/Data Flag pins */
+
+/* System registers */
+#define SYS_RST			0x0504 /* System Reset */
+#define SYS_ID			0x0580 /* System ID */
+
+#define SYS_RST_I2CS		BIT(0) /* Reset I2C-Slave controller */
+#define SYS_RST_I2CM		BIT(1) /* Reset I2C-Master controller */
+#define SYS_RST_LCD		BIT(2) /* Reset LCD controller */
+#define SYS_RST_BM		BIT(3) /* Reset Bus Management controller */
+#define SYS_RST_DSIRX		BIT(4) /* Reset DSI-RX and App controller */
+#define SYS_RST_REG		BIT(5) /* Reset Register module */
+
+#define LPX_PERIOD		2
+#define TTA_SURE		3
+#define TTA_GET			0x20000
+
+/* Lane enable PPI and DSI register bits */
+#define LANEENABLE_CLEN		BIT(0)
+#define LANEENABLE_L0EN		BIT(1)
+#define LANEENABLE_L1EN		BIT(2)
+#define LANEENABLE_L2EN		BIT(3)
+#define LANEENABLE_L3EN		BIT(4)
+
+/* LVCFG fields */
+#define LV_CFG_LVEN		BIT(0)
+#define LV_CFG_LVDLINK		BIT(1)
+#define LV_CFG_CLKPOL1		BIT(2)
+#define LV_CFG_CLKPOL2		BIT(3)
+
+static const char * const tc358764_supplies[] = {
+	"vddc", "vddio", "vddlvds"
+};
+
+struct tc358764 {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
+	struct gpio_desc *gpio_reset;
+	struct drm_panel *panel;
+	int error;
+};
+
+static int tc358764_clear_error(struct tc358764 *ctx)
+{
+	int ret = ctx->error;
+
+	ctx->error = 0;
+	return ret;
+}
+
+static void tc358764_read(struct tc358764 *ctx, u16 addr, u32 *val)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	ssize_t ret;
+
+	if (ctx->error)
+		return;
+
+	cpu_to_le16s(&addr);
+	ret = mipi_dsi_generic_read(dsi, &addr, sizeof(addr), val, sizeof(*val));
+	if (ret >= 0)
+		le32_to_cpus(val);
+
+	dev_dbg(ctx->dev, "read: %d, addr: %d\n", addr, *val);
+}
+
+static void tc358764_write(struct tc358764 *ctx, u16 addr, u32 val)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	ssize_t ret;
+	u8 data[6];
+
+	if (ctx->error)
+		return;
+
+	data[0] = addr;
+	data[1] = addr >> 8;
+	data[2] = val;
+	data[3] = val >> 8;
+	data[4] = val >> 16;
+	data[5] = val >> 24;
+
+	ret = mipi_dsi_generic_write(dsi, data, sizeof(data));
+	if (ret < 0)
+		ctx->error = ret;
+}
+
+static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct tc358764, bridge);
+}
+
+static inline
+struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
+{
+	return container_of(connector, struct tc358764, connector);
+}
+
+static int tc358764_init(struct tc358764 *ctx)
+{
+	u32 v = 0;
+
+	tc358764_read(ctx, SYS_ID, &v);
+	if (ctx->error)
+		return tc358764_clear_error(ctx);
+	dev_info(ctx->dev, "ID: %#x\n", v);
+
+	/* configure PPI counters */
+	tc358764_write(ctx, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
+	tc358764_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
+	tc358764_write(ctx, PPI_D0S_CLRSIPOCOUNT, 5);
+	tc358764_write(ctx, PPI_D1S_CLRSIPOCOUNT, 5);
+	tc358764_write(ctx, PPI_D2S_CLRSIPOCOUNT, 5);
+	tc358764_write(ctx, PPI_D3S_CLRSIPOCOUNT, 5);
+
+	/* enable four data lanes and clock lane */
+	tc358764_write(ctx, PPI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
+		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
+	tc358764_write(ctx, DSI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
+		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
+
+	/* start */
+	tc358764_write(ctx, PPI_STARTPPI, PPI_START_FUNCTION);
+	tc358764_write(ctx, DSI_STARTDSI, DSI_RX_START);
+
+	/* configure video path */
+	tc358764_write(ctx, VP_CTRL, VP_CTRL_VSDELAY(15) | VP_CTRL_RGB888(1) |
+		       VP_CTRL_EVTMODE(1) | VP_CTRL_HSPOL | VP_CTRL_VSPOL);
+
+	/* reset PHY */
+	tc358764_write(ctx, LV_PHY0, LV_PHY0_RST(1) |
+		       LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) | LV_PHY0_ND(6));
+	tc358764_write(ctx, LV_PHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) |
+		       LV_PHY0_ND(6));
+
+	/* reset bridge */
+	tc358764_write(ctx, SYS_RST, SYS_RST_LCD);
+
+	/* set bit order */
+	tc358764_write(ctx, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
+	tc358764_write(ctx, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
+	tc358764_write(ctx, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7));
+	tc358764_write(ctx, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
+	tc358764_write(ctx, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
+	tc358764_write(ctx, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
+	tc358764_write(ctx, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
+	tc358764_write(ctx, LV_CFG, LV_CFG_CLKPOL2 | LV_CFG_CLKPOL1 |
+		       LV_CFG_LVEN);
+
+	return tc358764_clear_error(ctx);
+}
+
+static void tc358764_reset(struct tc358764 *ctx)
+{
+	gpiod_set_value(ctx->gpio_reset, 1);
+	usleep_range(1000, 2000);
+	gpiod_set_value(ctx->gpio_reset, 0);
+	usleep_range(1000, 2000);
+}
+
+static int tc358764_get_modes(struct drm_connector *connector)
+{
+	struct tc358764 *ctx = connector_to_tc358764(connector);
+
+	return drm_panel_get_modes(ctx->panel);
+}
+
+static const
+struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
+	.get_modes = tc358764_get_modes,
+};
+
+static const struct drm_connector_funcs tc358764_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static void tc358764_disable(struct drm_bridge *bridge)
+{
+	struct tc358764 *ctx = bridge_to_tc358764(bridge);
+	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
+
+	if (ret < 0)
+		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
+}
+
+static void tc358764_post_disable(struct drm_bridge *bridge)
+{
+	struct tc358764 *ctx = bridge_to_tc358764(bridge);
+	int ret;
+
+	ret = drm_panel_unprepare(ctx->panel);
+	if (ret < 0)
+		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
+	tc358764_reset(ctx);
+	usleep_range(10000, 15000);
+	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret < 0)
+		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
+}
+
+static void tc358764_pre_enable(struct drm_bridge *bridge)
+{
+	struct tc358764 *ctx = bridge_to_tc358764(bridge);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret < 0)
+		dev_err(ctx->dev, "error enabling regulators (%d)\n", ret);
+	usleep_range(10000, 15000);
+	tc358764_reset(ctx);
+	ret = tc358764_init(ctx);
+	if (ret < 0)
+		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
+	ret = drm_panel_prepare(ctx->panel);
+	if (ret < 0)
+		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
+}
+
+static void tc358764_enable(struct drm_bridge *bridge)
+{
+	struct tc358764 *ctx = bridge_to_tc358764(bridge);
+	int ret = drm_panel_enable(ctx->panel);
+
+	if (ret < 0)
+		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
+}
+
+static int tc358764_attach(struct drm_bridge *bridge)
+{
+	struct tc358764 *ctx = bridge_to_tc358764(bridge);
+	struct drm_device *drm = bridge->dev;
+	int ret;
+
+	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
+	ret = drm_connector_init(drm, &ctx->connector,
+				 &tc358764_connector_funcs,
+				 DRM_MODE_CONNECTOR_LVDS);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&ctx->connector,
+				 &tc358764_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&ctx->connector, bridge->encoder);
+	drm_panel_attach(ctx->panel, &ctx->connector);
+	ctx->connector.funcs->reset(&ctx->connector);
+	drm_fb_helper_add_one_connector(drm->fb_helper, &ctx->connector);
+	drm_connector_register(&ctx->connector);
+
+	return 0;
+}
+
+static void tc358764_detach(struct drm_bridge *bridge)
+{
+	struct tc358764 *ctx = bridge_to_tc358764(bridge);
+	struct drm_device *drm = bridge->dev;
+
+	drm_connector_unregister(&ctx->connector);
+	drm_fb_helper_remove_one_connector(drm->fb_helper, &ctx->connector);
+	drm_panel_detach(ctx->panel);
+	ctx->panel = NULL;
+	drm_connector_unreference(&ctx->connector);
+}
+
+static const struct drm_bridge_funcs tc358764_bridge_funcs = {
+	.disable = tc358764_disable,
+	.post_disable = tc358764_post_disable,
+	.enable = tc358764_enable,
+	.pre_enable = tc358764_pre_enable,
+	.attach = tc358764_attach,
+	.detach = tc358764_detach,
+};
+
+static int tc358764_parse_dt(struct tc358764 *ctx)
+{
+	struct device *dev = ctx->dev;
+	int ret;
+
+	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->gpio_reset)) {
+		dev_err(dev, "no reset GPIO pin provided\n");
+		return PTR_ERR(ctx->gpio_reset);
+	}
+
+	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
+					  NULL);
+	if (ret && ret != -EPROBE_DEFER)
+		dev_err(dev, "cannot find panel (%d)\n", ret);
+
+	return ret;
+}
+
+static int tc358764_configure_regulators(struct tc358764 *ctx)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(ctx->supplies); ++i)
+		ctx->supplies[i].supply = tc358764_supplies[i];
+
+	ret = devm_regulator_bulk_get(ctx->dev, ARRAY_SIZE(ctx->supplies),
+				      ctx->supplies);
+	if (ret < 0)
+		dev_err(ctx->dev, "failed to get regulators: %d\n", ret);
+
+	return ret;
+}
+
+static int tc358764_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct tc358764 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(struct tc358764), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	ctx->dev = dev;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
+		| MIPI_DSI_MODE_VIDEO_AUTO_VERT | MIPI_DSI_MODE_LPM;
+
+	ret = tc358764_parse_dt(ctx);
+	if (ret < 0)
+		return ret;
+
+	ret = tc358764_configure_regulators(ctx);
+	if (ret < 0)
+		return ret;
+
+	ctx->bridge.funcs = &tc358764_bridge_funcs;
+	ctx->bridge.of_node = dev->of_node;
+
+	drm_bridge_add(&ctx->bridge);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		drm_bridge_remove(&ctx->bridge);
+		dev_err(dev, "failed to attach dsi\n");
+	}
+
+	return ret;
+}
+
+static int tc358764_remove(struct mipi_dsi_device *dsi)
+{
+	struct tc358764 *ctx = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_bridge_remove(&ctx->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id tc358764_of_match[] = {
+	{ .compatible = "toshiba,tc358764" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tc358764_of_match);
+
+static struct mipi_dsi_driver tc358764_driver = {
+	.probe = tc358764_probe,
+	.remove = tc358764_remove,
+	.driver = {
+		.name = "tc358764",
+		.owner = THIS_MODULE,
+		.of_match_table = tc358764_of_match,
+	},
+};
+module_mipi_dsi_driver(tc358764_driver);
+
+MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
+MODULE_AUTHOR("Maciej Purski <m.purski@samsung.com>");
+MODULE_DESCRIPTION("MIPI-DSI based Driver for TC358764 DSI/LVDS Bridge");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 6/9] ARM: dts: exynos5250: add DSI node

[Andrzej Hajda]

The patch adds common part of DSI node for Exynos5250 platforms
and a required mipi-phy node.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 arch/arm/boot/dts/exynos5250.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 2daf505b3d08..9965eca94a31 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -733,6 +733,27 @@
 			#phy-cells = <0>;
 		};
 
+		mipi_phy: video-phy@10040710 {
+			compatible = "samsung,s5pv210-mipi-video-phy";
+			reg = <0x10040710 0x100>;
+			#phy-cells = <1>;
+			syscon = <&pmu_system_controller>;
+		};
+
+		dsi_0: dsi@14500000 {
+			compatible = "samsung,exynos4210-mipi-dsi";
+			reg = <0x14500000 0x10000>;
+			interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,power-domain = <&pd_disp1>;
+			phys = <&mipi_phy 3>;
+			phy-names = "dsim";
+			clocks = <&clock CLK_DSIM0>, <&clock CLK_SCLK_MIPI1>;
+			clock-names = "bus_clk", "sclk_mipi";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		adc: adc@12d10000 {
 			compatible = "samsung,exynos-adc-v1";
 			reg = <0x12D10000 0x100>;
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 7/9] ARM: dts: exynos5250-arndale: add DSI and panel nodes

[Andrzej Hajda]

The patch adds bridge and panel nodes.
It adds also DSI properties specific for arndale board and
regulators required by the bridge.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 arch/arm/boot/dts/exynos5250-arndale.dts | 61 ++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index 7a8a5c55701a..816d89d4cefd 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -71,6 +71,17 @@
 		};
 	};
 
+	panel: panel {
+		compatible = "boe,hv070wsa-100";
+		power-supply = <&vcc_3v3_reg>;
+		enable-gpios = <&gpd1 3 GPIO_ACTIVE_HIGH>;
+		port {
+			panel_ep: endpoint {
+				remote-endpoint = <&bridge_out_ep>;
+			};
+		};
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -97,6 +108,30 @@
 			reg = <2>;
 			regulator-name = "hdmi-en";
 		};
+
+		vcc_1v2_reg: regulator@3 {
+			compatible = "regulator-fixed";
+			reg = <3>;
+			regulator-name = "VCC_1V2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+		};
+
+		vcc_1v8_reg: regulator@4 {
+			compatible = "regulator-fixed";
+			reg = <4>;
+			regulator-name = "VCC_1V8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+		};
+
+		vcc_3v3_reg: regulator@5 {
+			compatible = "regulator-fixed";
+			reg = <5>;
+			regulator-name = "VCC_3V3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
 	};
 
 	fixed-rate-clocks {
@@ -119,6 +154,32 @@
 	cpu0-supply = <&buck2_reg>;
 };
 
+&dsi_0 {
+	vddcore-supply = <&ldo8_reg>;
+	vddio-supply = <&ldo10_reg>;
+	samsung,pll-clock-frequency = <24000000>;
+	samsung,burst-clock-frequency = <320000000>;
+	samsung,esc-clock-frequency = <10000000>;
+	status = "okay";
+
+	bridge@0 {
+		reg = <0>;
+		compatible = "toshiba,tc358764";
+		vddc-supply = <&vcc_1v2_reg>;
+		vddio-supply = <&vcc_1v8_reg>;
+		vddlvds-supply = <&vcc_3v3_reg>;
+		reset-gpios = <&gpd1 6 GPIO_ACTIVE_LOW>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port@1 {
+			reg = <1>;
+			bridge_out_ep: endpoint {
+				remote-endpoint = <&panel_ep>;
+			};
+		};
+	};
+};
+
 &dp {
 	status = "okay";
 	samsung,color-space = <0>;
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 8/9] drm/panel: simple: fix BOE/HV070WSA-100 timings

[Andrzej Hajda]

Panel timings were taken from vendor code and are not fully correct - refresh
rate is about 50Hz instead of 60Hz. The patch fixes it.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index d5da58d5c9b1..a2226a3d2887 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -746,15 +746,15 @@ static const struct panel_desc avic_tm070ddh03 = {
 };
 
 static const struct drm_display_mode boe_hv070wsa_mode = {
-	.clock = 40800,
+	.clock = 42105,
 	.hdisplay = 1024,
-	.hsync_start = 1024 + 90,
-	.hsync_end = 1024 + 90 + 90,
-	.htotal = 1024 + 90 + 90 + 90,
+	.hsync_start = 1024 + 30,
+	.hsync_end = 1024 + 30 + 30,
+	.htotal = 1024 + 30 + 30 + 30,
 	.vdisplay = 600,
-	.vsync_start = 600 + 3,
-	.vsync_end = 600 + 3 + 4,
-	.vtotal = 600 + 3 + 4 + 3,
+	.vsync_start = 600 + 10,
+	.vsync_end = 600 + 10 + 10,
+	.vtotal = 600 + 10 + 10 + 10,
 	.vrefresh = 60,
 };
 
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 9/9] dt-bindings: exynos_dsim: update of graph bindings

[Andrzej Hajda]

Of-graph bindings should describe ports present in the device, not the
devices it can be connected to. The patch replaces verbose description
with shorter but more precise one. While at it clock related properties
are moved to the main node as it is their actual location.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 .../bindings/display/exynos/exynos_dsim.txt   | 25 +++++--------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt
index 2fff8b406f4c..be377786e8cd 100644
--- a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt
+++ b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt
@@ -21,6 +21,9 @@ Required properties:
   - samsung,pll-clock-frequency: specifies frequency of the oscillator clock
   - #address-cells, #size-cells: should be set respectively to <1> and <0>
     according to DSI host bindings (see MIPI DSI bindings [1])
+  - samsung,burst-clock-frequency: specifies DSI frequency in high-speed burst
+    mode
+  - samsung,esc-clock-frequency: specifies DSI frequency in escape mode
 
 Optional properties:
   - power-domains: a phandle to DSIM power domain node
@@ -29,25 +32,9 @@ Child nodes:
   Should contain DSI peripheral nodes (see MIPI DSI bindings [1]).
 
 Video interfaces:
-  Device node can contain video interface port nodes according to [2].
-  The following are properties specific to those nodes:
-
-  port node inbound:
-    - reg: (required) must be 0.
-  port node outbound:
-    - reg: (required) must be 1.
-
-  endpoint node connected from mic node (reg = 0):
-    - remote-endpoint: specifies the endpoint in mic node. This node is required
-		       for Exynos5433 mipi dsi. So mic can access to panel node
-		       throughout this dsi node.
-  endpoint node connected to panel node (reg = 1):
-    - remote-endpoint: specifies the endpoint in panel node. This node is
-		       required in all kinds of exynos mipi dsi to represent
-		       the connection between mipi dsi and panel.
-    - samsung,burst-clock-frequency: specifies DSI frequency in high-speed burst
-      mode
-    - samsung,esc-clock-frequency: specifies DSI frequency in escape mode
+  Device node can contain following video interface port nodes according to [2]:
+  0: RGB input,
+  1: DSI output
 
 [1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
 [2]: Documentation/devicetree/bindings/media/video-interfaces.txt
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 5/9] drm/bridge: tc358764: Add DSI to LVDS bridge driver

[Archit Taneja]

On Wednesday 25 July 2018 09:16 PM, Andrzej Hajda wrote:
> Add a drm_bridge driver for the Toshiba TC358764 DSI to LVDS bridge.
> 
> Changes in v4:
> - removed license blob,
> - ordered includes,
> - added error handling,
> - fixed reset GPIO handling,
> - added missing calls to the panel,
> - custom OF graph code replaced with helpers,
> - removed tc358764_poweroff from remove callback.
> v5:
> - fixed supply names,
> - fixed broken console - added connector to fb_helper,
> - added detach callback - unbinding works,
> - fixed typo in error checking code,
> - removed sparse bridge->encoder check - core does it already.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> [ a.hajda@samsung.com: v4, v5 ]
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>   drivers/gpu/drm/bridge/Kconfig    |   8 +
>   drivers/gpu/drm/bridge/Makefile   |   1 +
>   drivers/gpu/drm/bridge/tc358764.c | 499 ++++++++++++++++++++++++++++++
>   3 files changed, 508 insertions(+)
>   create mode 100644 drivers/gpu/drm/bridge/tc358764.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index fa2c7997e2fd..f3da8a716833 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -110,6 +110,14 @@ config DRM_THINE_THC63LVD1024
>   	---help---
>   	  Thine THC63LVD1024 LVDS/parallel converter driver.
>   
> +config DRM_TOSHIBA_TC358764
> +	tristate "TC358764 DSI/LVDS bridge"
> +	depends on DRM && DRM_PANEL
> +	depends on OF
> +	select DRM_MIPI_DSI
> +	help
> +	  Toshiba TC358764 DSI/LVDS bridge driver.
> +
>   config DRM_TOSHIBA_TC358767
>   	tristate "Toshiba TC358767 eDP bridge"
>   	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 35f88d48ec20..bf7c0cecf227 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>   obj-$(CONFIG_DRM_SII902X) += sii902x.o
>   obj-$(CONFIG_DRM_SII9234) += sii9234.o
>   obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
> +obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
>   obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
> new file mode 100644
> index 000000000000..779bc5fce22a
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/tc358764.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Samsung Electronics Co., Ltd
> + *
> + * Authors:
> + *	Andrzej Hajda <a.hajda@samsung.com>
> + *	Maciej Purski <m.purski@samsung.com>
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drmP.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +#include <video/mipi_display.h>
> +
> +#define FLD_MASK(start, end)    (((1 << ((start) - (end) + 1)) - 1) << (end))
> +#define FLD_VAL(val, start, end) (((val) << (end)) & FLD_MASK(start, end))
> +
> +/* PPI layer registers */
> +#define PPI_STARTPPI		0x0104 /* START control bit */
> +#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
> +#define PPI_LANEENABLE		0x0134 /* Enables each lane */
> +#define PPI_TX_RX_TA		0x013C /* BTA timing parameters */
> +#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
> +#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
> +#define PPI_D2S_CLRSIPOCOUNT	0x016C /* Assertion timer for Lane 2 */
> +#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
> +#define PPI_START_FUNCTION	1
> +
> +/* DSI layer registers */
> +#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
> +#define DSI_LANEENABLE		0x0210 /* Enables each lane */
> +#define DSI_RX_START		1
> +
> +/* Video path registers */
> +#define VP_CTRL			0x0450 /* Video Path Control */
> +#define VP_CTRL_MSF(v)		FLD_VAL(v, 0, 0) /* Magic square in RGB666 */
> +#define VP_CTRL_VTGEN(v)	FLD_VAL(v, 4, 4) /* Use chip clock for timing */
> +#define VP_CTRL_EVTMODE(v)	FLD_VAL(v, 5, 5) /* Event mode */
> +#define VP_CTRL_RGB888(v)	FLD_VAL(v, 8, 8) /* RGB888 mode */
> +#define VP_CTRL_VSDELAY(v)	FLD_VAL(v, 31, 20) /* VSYNC delay */
> +#define VP_CTRL_HSPOL		BIT(17) /* Polarity of HSYNC signal */
> +#define VP_CTRL_DEPOL		BIT(18) /* Polarity of DE signal */
> +#define VP_CTRL_VSPOL		BIT(19) /* Polarity of VSYNC signal */
> +#define VP_HTIM1		0x0454 /* Horizontal Timing Control 1 */
> +#define VP_HTIM1_HBP(v)		FLD_VAL(v, 24, 16)
> +#define VP_HTIM1_HSYNC(v)	FLD_VAL(v, 8, 0)
> +#define VP_HTIM2		0x0458 /* Horizontal Timing Control 2 */
> +#define VP_HTIM2_HFP(v)		FLD_VAL(v, 24, 16)
> +#define VP_HTIM2_HACT(v)	FLD_VAL(v, 10, 0)
> +#define VP_VTIM1		0x045C /* Vertical Timing Control 1 */
> +#define VP_VTIM1_VBP(v)		FLD_VAL(v, 23, 16)
> +#define VP_VTIM1_VSYNC(v)	FLD_VAL(v, 7, 0)
> +#define VP_VTIM2		0x0460 /* Vertical Timing Control 2 */
> +#define VP_VTIM2_VFP(v)		FLD_VAL(v, 23, 16)
> +#define VP_VTIM2_VACT(v)	FLD_VAL(v, 10, 0)
> +#define VP_VFUEN		0x0464 /* Video Frame Timing Update Enable */
> +
> +/* LVDS registers */
> +#define LV_MX0003		0x0480 /* Mux input bit 0 to 3 */
> +#define LV_MX0407		0x0484 /* Mux input bit 4 to 7 */
> +#define LV_MX0811		0x0488 /* Mux input bit 8 to 11 */
> +#define LV_MX1215		0x048C /* Mux input bit 12 to 15 */
> +#define LV_MX1619		0x0490 /* Mux input bit 16 to 19 */
> +#define LV_MX2023		0x0494 /* Mux input bit 20 to 23 */
> +#define LV_MX2427		0x0498 /* Mux input bit 24 to 27 */
> +#define LV_MX(b0, b1, b2, b3)	(FLD_VAL(b0, 4, 0) | FLD_VAL(b1, 12, 8) | \
> +				FLD_VAL(b2, 20, 16) | FLD_VAL(b3, 28, 24))
> +
> +/* Input bit numbers used in mux registers */
> +enum {
> +	LVI_R0,
> +	LVI_R1,
> +	LVI_R2,
> +	LVI_R3,
> +	LVI_R4,
> +	LVI_R5,
> +	LVI_R6,
> +	LVI_R7,
> +	LVI_G0,
> +	LVI_G1,
> +	LVI_G2,
> +	LVI_G3,
> +	LVI_G4,
> +	LVI_G5,
> +	LVI_G6,
> +	LVI_G7,
> +	LVI_B0,
> +	LVI_B1,
> +	LVI_B2,
> +	LVI_B3,
> +	LVI_B4,
> +	LVI_B5,
> +	LVI_B6,
> +	LVI_B7,
> +	LVI_HS,
> +	LVI_VS,
> +	LVI_DE,
> +	LVI_L0
> +};
> +
> +#define LV_CFG			0x049C /* LVDS Configuration */
> +#define LV_PHY0			0x04A0 /* LVDS PHY 0 */
> +#define LV_PHY0_RST(v)		FLD_VAL(v, 22, 22) /* PHY reset */
> +#define LV_PHY0_IS(v)		FLD_VAL(v, 15, 14)
> +#define LV_PHY0_ND(v)		FLD_VAL(v, 4, 0) /* Frequency range select */
> +#define LV_PHY0_PRBS_ON(v)	FLD_VAL(v, 20, 16) /* Clock/Data Flag pins */
> +
> +/* System registers */
> +#define SYS_RST			0x0504 /* System Reset */
> +#define SYS_ID			0x0580 /* System ID */
> +
> +#define SYS_RST_I2CS		BIT(0) /* Reset I2C-Slave controller */
> +#define SYS_RST_I2CM		BIT(1) /* Reset I2C-Master controller */
> +#define SYS_RST_LCD		BIT(2) /* Reset LCD controller */
> +#define SYS_RST_BM		BIT(3) /* Reset Bus Management controller */
> +#define SYS_RST_DSIRX		BIT(4) /* Reset DSI-RX and App controller */
> +#define SYS_RST_REG		BIT(5) /* Reset Register module */
> +
> +#define LPX_PERIOD		2
> +#define TTA_SURE		3
> +#define TTA_GET			0x20000
> +
> +/* Lane enable PPI and DSI register bits */
> +#define LANEENABLE_CLEN		BIT(0)
> +#define LANEENABLE_L0EN		BIT(1)
> +#define LANEENABLE_L1EN		BIT(2)
> +#define LANEENABLE_L2EN		BIT(3)
> +#define LANEENABLE_L3EN		BIT(4)
> +
> +/* LVCFG fields */
> +#define LV_CFG_LVEN		BIT(0)
> +#define LV_CFG_LVDLINK		BIT(1)
> +#define LV_CFG_CLKPOL1		BIT(2)
> +#define LV_CFG_CLKPOL2		BIT(3)
> +
> +static const char * const tc358764_supplies[] = {
> +	"vddc", "vddio", "vddlvds"
> +};
> +
> +struct tc358764 {
> +	struct device *dev;
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
> +	struct gpio_desc *gpio_reset;
> +	struct drm_panel *panel;
> +	int error;
> +};
> +
> +static int tc358764_clear_error(struct tc358764 *ctx)
> +{
> +	int ret = ctx->error;
> +
> +	ctx->error = 0;
> +	return ret;
> +}
> +
> +static void tc358764_read(struct tc358764 *ctx, u16 addr, u32 *val)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	ssize_t ret;
> +
> +	if (ctx->error)
> +		return;
> +
> +	cpu_to_le16s(&addr);
> +	ret = mipi_dsi_generic_read(dsi, &addr, sizeof(addr), val, sizeof(*val));
> +	if (ret >= 0)
> +		le32_to_cpus(val);
> +
> +	dev_dbg(ctx->dev, "read: %d, addr: %d\n", addr, *val);
> +}
> +
> +static void tc358764_write(struct tc358764 *ctx, u16 addr, u32 val)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	ssize_t ret;
> +	u8 data[6];
> +
> +	if (ctx->error)
> +		return;
> +
> +	data[0] = addr;
> +	data[1] = addr >> 8;
> +	data[2] = val;
> +	data[3] = val >> 8;
> +	data[4] = val >> 16;
> +	data[5] = val >> 24;
> +
> +	ret = mipi_dsi_generic_write(dsi, data, sizeof(data));
> +	if (ret < 0)
> +		ctx->error = ret;
> +}
> +
> +static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct tc358764, bridge);
> +}
> +
> +static inline
> +struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct tc358764, connector);
> +}
> +
> +static int tc358764_init(struct tc358764 *ctx)
> +{
> +	u32 v = 0;
> +
> +	tc358764_read(ctx, SYS_ID, &v);
> +	if (ctx->error)
> +		return tc358764_clear_error(ctx);
> +	dev_info(ctx->dev, "ID: %#x\n", v);
> +
> +	/* configure PPI counters */
> +	tc358764_write(ctx, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
> +	tc358764_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
> +	tc358764_write(ctx, PPI_D0S_CLRSIPOCOUNT, 5);
> +	tc358764_write(ctx, PPI_D1S_CLRSIPOCOUNT, 5);
> +	tc358764_write(ctx, PPI_D2S_CLRSIPOCOUNT, 5);
> +	tc358764_write(ctx, PPI_D3S_CLRSIPOCOUNT, 5);
> +
> +	/* enable four data lanes and clock lane */
> +	tc358764_write(ctx, PPI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
> +	tc358764_write(ctx, DSI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
> +
> +	/* start */
> +	tc358764_write(ctx, PPI_STARTPPI, PPI_START_FUNCTION);
> +	tc358764_write(ctx, DSI_STARTDSI, DSI_RX_START);
> +
> +	/* configure video path */
> +	tc358764_write(ctx, VP_CTRL, VP_CTRL_VSDELAY(15) | VP_CTRL_RGB888(1) |
> +		       VP_CTRL_EVTMODE(1) | VP_CTRL_HSPOL | VP_CTRL_VSPOL);
> +
> +	/* reset PHY */
> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_RST(1) |
> +		       LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) | LV_PHY0_ND(6));
> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) |
> +		       LV_PHY0_ND(6));
> +
> +	/* reset bridge */
> +	tc358764_write(ctx, SYS_RST, SYS_RST_LCD);
> +
> +	/* set bit order */
> +	tc358764_write(ctx, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> +	tc358764_write(ctx, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> +	tc358764_write(ctx, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7));
> +	tc358764_write(ctx, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
> +	tc358764_write(ctx, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
> +	tc358764_write(ctx, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
> +	tc358764_write(ctx, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
> +	tc358764_write(ctx, LV_CFG, LV_CFG_CLKPOL2 | LV_CFG_CLKPOL1 |
> +		       LV_CFG_LVEN);
> +
> +	return tc358764_clear_error(ctx);
> +}
> +
> +static void tc358764_reset(struct tc358764 *ctx)
> +{
> +	gpiod_set_value(ctx->gpio_reset, 1);
> +	usleep_range(1000, 2000);
> +	gpiod_set_value(ctx->gpio_reset, 0);
> +	usleep_range(1000, 2000);
> +}
> +
> +static int tc358764_get_modes(struct drm_connector *connector)
> +{
> +	struct tc358764 *ctx = connector_to_tc358764(connector);
> +
> +	return drm_panel_get_modes(ctx->panel);
> +}
> +
> +static const
> +struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
> +	.get_modes = tc358764_get_modes,
> +};
> +
> +static const struct drm_connector_funcs tc358764_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static void tc358764_disable(struct drm_bridge *bridge)
> +{
> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> +	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
> +
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
> +}
> +
> +static void tc358764_post_disable(struct drm_bridge *bridge)
> +{
> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> +	int ret;
> +
> +	ret = drm_panel_unprepare(ctx->panel);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
> +	tc358764_reset(ctx);
> +	usleep_range(10000, 15000);
> +	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
> +}
> +
> +static void tc358764_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error enabling regulators (%d)\n", ret);
> +	usleep_range(10000, 15000);
> +	tc358764_reset(ctx);
> +	ret = tc358764_init(ctx);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> +	ret = drm_panel_prepare(ctx->panel);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
> +}
> +
> +static void tc358764_enable(struct drm_bridge *bridge)
> +{
> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> +	int ret = drm_panel_enable(ctx->panel);
> +
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
> +}
> +
> +static int tc358764_attach(struct drm_bridge *bridge)
> +{
> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> +	struct drm_device *drm = bridge->dev;
> +	int ret;
> +
> +	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +	ret = drm_connector_init(drm, &ctx->connector,
> +				 &tc358764_connector_funcs,
> +				 DRM_MODE_CONNECTOR_LVDS);
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector\n");
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(&ctx->connector,
> +				 &tc358764_connector_helper_funcs);
> +	drm_mode_connector_attach_encoder(&ctx->connector, bridge->encoder);
> +	drm_panel_attach(ctx->panel, &ctx->connector);
> +	ctx->connector.funcs->reset(&ctx->connector);
> +	drm_fb_helper_add_one_connector(drm->fb_helper, &ctx->connector);
> +	drm_connector_register(&ctx->connector);

Aren't drm_connector_register()/unregister calls managed by the drm
core?

Otherwise:

Reviewed-by: Archit Taneja <architt@codeaurora.org>

Thanks,
Archit

> +
> +	return 0;
> +}
> +
> +static void tc358764_detach(struct drm_bridge *bridge)
> +{
> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> +	struct drm_device *drm = bridge->dev;
> +
> +	drm_connector_unregister(&ctx->connector);
> +	drm_fb_helper_remove_one_connector(drm->fb_helper, &ctx->connector);
> +	drm_panel_detach(ctx->panel);
> +	ctx->panel = NULL;
> +	drm_connector_unreference(&ctx->connector);
> +}
> +
> +static const struct drm_bridge_funcs tc358764_bridge_funcs = {
> +	.disable = tc358764_disable,
> +	.post_disable = tc358764_post_disable,
> +	.enable = tc358764_enable,
> +	.pre_enable = tc358764_pre_enable,
> +	.attach = tc358764_attach,
> +	.detach = tc358764_detach,
> +};
> +
> +static int tc358764_parse_dt(struct tc358764 *ctx)
> +{
> +	struct device *dev = ctx->dev;
> +	int ret;
> +
> +	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->gpio_reset)) {
> +		dev_err(dev, "no reset GPIO pin provided\n");
> +		return PTR_ERR(ctx->gpio_reset);
> +	}
> +
> +	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
> +					  NULL);
> +	if (ret && ret != -EPROBE_DEFER)
> +		dev_err(dev, "cannot find panel (%d)\n", ret);
> +
> +	return ret;
> +}
> +
> +static int tc358764_configure_regulators(struct tc358764 *ctx)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); ++i)
> +		ctx->supplies[i].supply = tc358764_supplies[i];
> +
> +	ret = devm_regulator_bulk_get(ctx->dev, ARRAY_SIZE(ctx->supplies),
> +				      ctx->supplies);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "failed to get regulators: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int tc358764_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct tc358764 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct tc358764), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +
> +	ctx->dev = dev;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
> +		| MIPI_DSI_MODE_VIDEO_AUTO_VERT | MIPI_DSI_MODE_LPM;
> +
> +	ret = tc358764_parse_dt(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tc358764_configure_regulators(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctx->bridge.funcs = &tc358764_bridge_funcs;
> +	ctx->bridge.of_node = dev->of_node;
> +
> +	drm_bridge_add(&ctx->bridge);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		drm_bridge_remove(&ctx->bridge);
> +		dev_err(dev, "failed to attach dsi\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static int tc358764_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct tc358764 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_bridge_remove(&ctx->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tc358764_of_match[] = {
> +	{ .compatible = "toshiba,tc358764" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tc358764_of_match);
> +
> +static struct mipi_dsi_driver tc358764_driver = {
> +	.probe = tc358764_probe,
> +	.remove = tc358764_remove,
> +	.driver = {
> +		.name = "tc358764",
> +		.owner = THIS_MODULE,
> +		.of_match_table = tc358764_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(tc358764_driver);
> +
> +MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
> +MODULE_AUTHOR("Maciej Purski <m.purski@samsung.com>");
> +MODULE_DESCRIPTION("MIPI-DSI based Driver for TC358764 DSI/LVDS Bridge");
> +MODULE_LICENSE("GPL v2");
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/9] ARM: dts: exynos5250: add DSI node

[Krzysztof Kozlowski]

On 25 July 2018 at 17:46, Andrzej Hajda <a.hajda@samsung.com> wrote:
> The patch adds common part of DSI node for Exynos5250 platforms
> and a required mipi-phy node.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250.dtsi | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

I'll take it after merge window so for v4.20.

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 7/9] ARM: dts: exynos5250-arndale: add DSI and panel nodes

[Krzysztof Kozlowski]

On 25 July 2018 at 17:46, Andrzej Hajda <a.hajda@samsung.com> wrote:
> The patch adds bridge and panel nodes.
> It adds also DSI properties specific for arndale board and
> regulators required by the bridge.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250-arndale.dts | 61 ++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)

I'll take it after merge window so for v4.20.

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 5/9] drm/bridge: tc358764: Add DSI to LVDS bridge driver

[Andrzej Hajda]

On 26.07.2018 09:36, Archit Taneja wrote:
>
> On Wednesday 25 July 2018 09:16 PM, Andrzej Hajda wrote:
>> Add a drm_bridge driver for the Toshiba TC358764 DSI to LVDS bridge.
>>
>> Changes in v4:
>> - removed license blob,
>> - ordered includes,
>> - added error handling,
>> - fixed reset GPIO handling,
>> - added missing calls to the panel,
>> - custom OF graph code replaced with helpers,
>> - removed tc358764_poweroff from remove callback.
>> v5:
>> - fixed supply names,
>> - fixed broken console - added connector to fb_helper,
>> - added detach callback - unbinding works,
>> - fixed typo in error checking code,
>> - removed sparse bridge->encoder check - core does it already.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> [ a.hajda@samsung.com: v4, v5 ]
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>   drivers/gpu/drm/bridge/Kconfig    |   8 +
>>   drivers/gpu/drm/bridge/Makefile   |   1 +
>>   drivers/gpu/drm/bridge/tc358764.c | 499 ++++++++++++++++++++++++++++++
>>   3 files changed, 508 insertions(+)
>>   create mode 100644 drivers/gpu/drm/bridge/tc358764.c
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index fa2c7997e2fd..f3da8a716833 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -110,6 +110,14 @@ config DRM_THINE_THC63LVD1024
>>   	---help---
>>   	  Thine THC63LVD1024 LVDS/parallel converter driver.
>>   
>> +config DRM_TOSHIBA_TC358764
>> +	tristate "TC358764 DSI/LVDS bridge"
>> +	depends on DRM && DRM_PANEL
>> +	depends on OF
>> +	select DRM_MIPI_DSI
>> +	help
>> +	  Toshiba TC358764 DSI/LVDS bridge driver.
>> +
>>   config DRM_TOSHIBA_TC358767
>>   	tristate "Toshiba TC358767 eDP bridge"
>>   	depends on OF
>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>> index 35f88d48ec20..bf7c0cecf227 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>   obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>   obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>   obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>> +obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
>>   obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
>> new file mode 100644
>> index 000000000000..779bc5fce22a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/tc358764.c
>> @@ -0,0 +1,499 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Samsung Electronics Co., Ltd
>> + *
>> + * Authors:
>> + *	Andrzej Hajda <a.hajda@samsung.com>
>> + *	Maciej Purski <m.purski@samsung.com>
>> + */
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drmP.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <video/mipi_display.h>
>> +
>> +#define FLD_MASK(start, end)    (((1 << ((start) - (end) + 1)) - 1) << (end))
>> +#define FLD_VAL(val, start, end) (((val) << (end)) & FLD_MASK(start, end))
>> +
>> +/* PPI layer registers */
>> +#define PPI_STARTPPI		0x0104 /* START control bit */
>> +#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
>> +#define PPI_LANEENABLE		0x0134 /* Enables each lane */
>> +#define PPI_TX_RX_TA		0x013C /* BTA timing parameters */
>> +#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
>> +#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
>> +#define PPI_D2S_CLRSIPOCOUNT	0x016C /* Assertion timer for Lane 2 */
>> +#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
>> +#define PPI_START_FUNCTION	1
>> +
>> +/* DSI layer registers */
>> +#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
>> +#define DSI_LANEENABLE		0x0210 /* Enables each lane */
>> +#define DSI_RX_START		1
>> +
>> +/* Video path registers */
>> +#define VP_CTRL			0x0450 /* Video Path Control */
>> +#define VP_CTRL_MSF(v)		FLD_VAL(v, 0, 0) /* Magic square in RGB666 */
>> +#define VP_CTRL_VTGEN(v)	FLD_VAL(v, 4, 4) /* Use chip clock for timing */
>> +#define VP_CTRL_EVTMODE(v)	FLD_VAL(v, 5, 5) /* Event mode */
>> +#define VP_CTRL_RGB888(v)	FLD_VAL(v, 8, 8) /* RGB888 mode */
>> +#define VP_CTRL_VSDELAY(v)	FLD_VAL(v, 31, 20) /* VSYNC delay */
>> +#define VP_CTRL_HSPOL		BIT(17) /* Polarity of HSYNC signal */
>> +#define VP_CTRL_DEPOL		BIT(18) /* Polarity of DE signal */
>> +#define VP_CTRL_VSPOL		BIT(19) /* Polarity of VSYNC signal */
>> +#define VP_HTIM1		0x0454 /* Horizontal Timing Control 1 */
>> +#define VP_HTIM1_HBP(v)		FLD_VAL(v, 24, 16)
>> +#define VP_HTIM1_HSYNC(v)	FLD_VAL(v, 8, 0)
>> +#define VP_HTIM2		0x0458 /* Horizontal Timing Control 2 */
>> +#define VP_HTIM2_HFP(v)		FLD_VAL(v, 24, 16)
>> +#define VP_HTIM2_HACT(v)	FLD_VAL(v, 10, 0)
>> +#define VP_VTIM1		0x045C /* Vertical Timing Control 1 */
>> +#define VP_VTIM1_VBP(v)		FLD_VAL(v, 23, 16)
>> +#define VP_VTIM1_VSYNC(v)	FLD_VAL(v, 7, 0)
>> +#define VP_VTIM2		0x0460 /* Vertical Timing Control 2 */
>> +#define VP_VTIM2_VFP(v)		FLD_VAL(v, 23, 16)
>> +#define VP_VTIM2_VACT(v)	FLD_VAL(v, 10, 0)
>> +#define VP_VFUEN		0x0464 /* Video Frame Timing Update Enable */
>> +
>> +/* LVDS registers */
>> +#define LV_MX0003		0x0480 /* Mux input bit 0 to 3 */
>> +#define LV_MX0407		0x0484 /* Mux input bit 4 to 7 */
>> +#define LV_MX0811		0x0488 /* Mux input bit 8 to 11 */
>> +#define LV_MX1215		0x048C /* Mux input bit 12 to 15 */
>> +#define LV_MX1619		0x0490 /* Mux input bit 16 to 19 */
>> +#define LV_MX2023		0x0494 /* Mux input bit 20 to 23 */
>> +#define LV_MX2427		0x0498 /* Mux input bit 24 to 27 */
>> +#define LV_MX(b0, b1, b2, b3)	(FLD_VAL(b0, 4, 0) | FLD_VAL(b1, 12, 8) | \
>> +				FLD_VAL(b2, 20, 16) | FLD_VAL(b3, 28, 24))
>> +
>> +/* Input bit numbers used in mux registers */
>> +enum {
>> +	LVI_R0,
>> +	LVI_R1,
>> +	LVI_R2,
>> +	LVI_R3,
>> +	LVI_R4,
>> +	LVI_R5,
>> +	LVI_R6,
>> +	LVI_R7,
>> +	LVI_G0,
>> +	LVI_G1,
>> +	LVI_G2,
>> +	LVI_G3,
>> +	LVI_G4,
>> +	LVI_G5,
>> +	LVI_G6,
>> +	LVI_G7,
>> +	LVI_B0,
>> +	LVI_B1,
>> +	LVI_B2,
>> +	LVI_B3,
>> +	LVI_B4,
>> +	LVI_B5,
>> +	LVI_B6,
>> +	LVI_B7,
>> +	LVI_HS,
>> +	LVI_VS,
>> +	LVI_DE,
>> +	LVI_L0
>> +};
>> +
>> +#define LV_CFG			0x049C /* LVDS Configuration */
>> +#define LV_PHY0			0x04A0 /* LVDS PHY 0 */
>> +#define LV_PHY0_RST(v)		FLD_VAL(v, 22, 22) /* PHY reset */
>> +#define LV_PHY0_IS(v)		FLD_VAL(v, 15, 14)
>> +#define LV_PHY0_ND(v)		FLD_VAL(v, 4, 0) /* Frequency range select */
>> +#define LV_PHY0_PRBS_ON(v)	FLD_VAL(v, 20, 16) /* Clock/Data Flag pins */
>> +
>> +/* System registers */
>> +#define SYS_RST			0x0504 /* System Reset */
>> +#define SYS_ID			0x0580 /* System ID */
>> +
>> +#define SYS_RST_I2CS		BIT(0) /* Reset I2C-Slave controller */
>> +#define SYS_RST_I2CM		BIT(1) /* Reset I2C-Master controller */
>> +#define SYS_RST_LCD		BIT(2) /* Reset LCD controller */
>> +#define SYS_RST_BM		BIT(3) /* Reset Bus Management controller */
>> +#define SYS_RST_DSIRX		BIT(4) /* Reset DSI-RX and App controller */
>> +#define SYS_RST_REG		BIT(5) /* Reset Register module */
>> +
>> +#define LPX_PERIOD		2
>> +#define TTA_SURE		3
>> +#define TTA_GET			0x20000
>> +
>> +/* Lane enable PPI and DSI register bits */
>> +#define LANEENABLE_CLEN		BIT(0)
>> +#define LANEENABLE_L0EN		BIT(1)
>> +#define LANEENABLE_L1EN		BIT(2)
>> +#define LANEENABLE_L2EN		BIT(3)
>> +#define LANEENABLE_L3EN		BIT(4)
>> +
>> +/* LVCFG fields */
>> +#define LV_CFG_LVEN		BIT(0)
>> +#define LV_CFG_LVDLINK		BIT(1)
>> +#define LV_CFG_CLKPOL1		BIT(2)
>> +#define LV_CFG_CLKPOL2		BIT(3)
>> +
>> +static const char * const tc358764_supplies[] = {
>> +	"vddc", "vddio", "vddlvds"
>> +};
>> +
>> +struct tc358764 {
>> +	struct device *dev;
>> +	struct drm_bridge bridge;
>> +	struct drm_connector connector;
>> +	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
>> +	struct gpio_desc *gpio_reset;
>> +	struct drm_panel *panel;
>> +	int error;
>> +};
>> +
>> +static int tc358764_clear_error(struct tc358764 *ctx)
>> +{
>> +	int ret = ctx->error;
>> +
>> +	ctx->error = 0;
>> +	return ret;
>> +}
>> +
>> +static void tc358764_read(struct tc358764 *ctx, u16 addr, u32 *val)
>> +{
>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> +	ssize_t ret;
>> +
>> +	if (ctx->error)
>> +		return;
>> +
>> +	cpu_to_le16s(&addr);
>> +	ret = mipi_dsi_generic_read(dsi, &addr, sizeof(addr), val, sizeof(*val));
>> +	if (ret >= 0)
>> +		le32_to_cpus(val);
>> +
>> +	dev_dbg(ctx->dev, "read: %d, addr: %d\n", addr, *val);
>> +}
>> +
>> +static void tc358764_write(struct tc358764 *ctx, u16 addr, u32 val)
>> +{
>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> +	ssize_t ret;
>> +	u8 data[6];
>> +
>> +	if (ctx->error)
>> +		return;
>> +
>> +	data[0] = addr;
>> +	data[1] = addr >> 8;
>> +	data[2] = val;
>> +	data[3] = val >> 8;
>> +	data[4] = val >> 16;
>> +	data[5] = val >> 24;
>> +
>> +	ret = mipi_dsi_generic_write(dsi, data, sizeof(data));
>> +	if (ret < 0)
>> +		ctx->error = ret;
>> +}
>> +
>> +static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
>> +{
>> +	return container_of(bridge, struct tc358764, bridge);
>> +}
>> +
>> +static inline
>> +struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
>> +{
>> +	return container_of(connector, struct tc358764, connector);
>> +}
>> +
>> +static int tc358764_init(struct tc358764 *ctx)
>> +{
>> +	u32 v = 0;
>> +
>> +	tc358764_read(ctx, SYS_ID, &v);
>> +	if (ctx->error)
>> +		return tc358764_clear_error(ctx);
>> +	dev_info(ctx->dev, "ID: %#x\n", v);
>> +
>> +	/* configure PPI counters */
>> +	tc358764_write(ctx, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
>> +	tc358764_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
>> +	tc358764_write(ctx, PPI_D0S_CLRSIPOCOUNT, 5);
>> +	tc358764_write(ctx, PPI_D1S_CLRSIPOCOUNT, 5);
>> +	tc358764_write(ctx, PPI_D2S_CLRSIPOCOUNT, 5);
>> +	tc358764_write(ctx, PPI_D3S_CLRSIPOCOUNT, 5);
>> +
>> +	/* enable four data lanes and clock lane */
>> +	tc358764_write(ctx, PPI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
>> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
>> +	tc358764_write(ctx, DSI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
>> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
>> +
>> +	/* start */
>> +	tc358764_write(ctx, PPI_STARTPPI, PPI_START_FUNCTION);
>> +	tc358764_write(ctx, DSI_STARTDSI, DSI_RX_START);
>> +
>> +	/* configure video path */
>> +	tc358764_write(ctx, VP_CTRL, VP_CTRL_VSDELAY(15) | VP_CTRL_RGB888(1) |
>> +		       VP_CTRL_EVTMODE(1) | VP_CTRL_HSPOL | VP_CTRL_VSPOL);
>> +
>> +	/* reset PHY */
>> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_RST(1) |
>> +		       LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) | LV_PHY0_ND(6));
>> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) |
>> +		       LV_PHY0_ND(6));
>> +
>> +	/* reset bridge */
>> +	tc358764_write(ctx, SYS_RST, SYS_RST_LCD);
>> +
>> +	/* set bit order */
>> +	tc358764_write(ctx, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
>> +	tc358764_write(ctx, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
>> +	tc358764_write(ctx, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7));
>> +	tc358764_write(ctx, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
>> +	tc358764_write(ctx, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
>> +	tc358764_write(ctx, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
>> +	tc358764_write(ctx, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
>> +	tc358764_write(ctx, LV_CFG, LV_CFG_CLKPOL2 | LV_CFG_CLKPOL1 |
>> +		       LV_CFG_LVEN);
>> +
>> +	return tc358764_clear_error(ctx);
>> +}
>> +
>> +static void tc358764_reset(struct tc358764 *ctx)
>> +{
>> +	gpiod_set_value(ctx->gpio_reset, 1);
>> +	usleep_range(1000, 2000);
>> +	gpiod_set_value(ctx->gpio_reset, 0);
>> +	usleep_range(1000, 2000);
>> +}
>> +
>> +static int tc358764_get_modes(struct drm_connector *connector)
>> +{
>> +	struct tc358764 *ctx = connector_to_tc358764(connector);
>> +
>> +	return drm_panel_get_modes(ctx->panel);
>> +}
>> +
>> +static const
>> +struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
>> +	.get_modes = tc358764_get_modes,
>> +};
>> +
>> +static const struct drm_connector_funcs tc358764_connector_funcs = {
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.destroy = drm_connector_cleanup,
>> +	.reset = drm_atomic_helper_connector_reset,
>> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static void tc358764_disable(struct drm_bridge *bridge)
>> +{
>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>> +	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
>> +
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
>> +}
>> +
>> +static void tc358764_post_disable(struct drm_bridge *bridge)
>> +{
>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>> +	int ret;
>> +
>> +	ret = drm_panel_unprepare(ctx->panel);
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
>> +	tc358764_reset(ctx);
>> +	usleep_range(10000, 15000);
>> +	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
>> +}
>> +
>> +static void tc358764_pre_enable(struct drm_bridge *bridge)
>> +{
>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>> +	int ret;
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error enabling regulators (%d)\n", ret);
>> +	usleep_range(10000, 15000);
>> +	tc358764_reset(ctx);
>> +	ret = tc358764_init(ctx);
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
>> +	ret = drm_panel_prepare(ctx->panel);
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
>> +}
>> +
>> +static void tc358764_enable(struct drm_bridge *bridge)
>> +{
>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>> +	int ret = drm_panel_enable(ctx->panel);
>> +
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
>> +}
>> +
>> +static int tc358764_attach(struct drm_bridge *bridge)
>> +{
>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>> +	struct drm_device *drm = bridge->dev;
>> +	int ret;
>> +
>> +	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
>> +	ret = drm_connector_init(drm, &ctx->connector,
>> +				 &tc358764_connector_funcs,
>> +				 DRM_MODE_CONNECTOR_LVDS);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to initialize connector\n");
>> +		return ret;
>> +	}
>> +
>> +	drm_connector_helper_add(&ctx->connector,
>> +				 &tc358764_connector_helper_funcs);
>> +	drm_mode_connector_attach_encoder(&ctx->connector, bridge->encoder);
>> +	drm_panel_attach(ctx->panel, &ctx->connector);
>> +	ctx->connector.funcs->reset(&ctx->connector);
>> +	drm_fb_helper_add_one_connector(drm->fb_helper, &ctx->connector);
>> +	drm_connector_register(&ctx->connector);
> Aren't drm_connector_register()/unregister calls managed by the drm
> core?

drm core registers connectors during initialization but tc358764_attach
can be called after bridge probe, also after drm initialization, in such
case connector should be dynamically allocated/de-allocated.

> Otherwise:
> Reviewed-by: Archit Taneja <architt@codeaurora.org>

Thanks for the review, queued to drm-misc-next.

Regards
Andrzej


>
> Thanks,
> Archit
>
>> +
>> +	return 0;
>> +}
>> +
>> +static void tc358764_detach(struct drm_bridge *bridge)
>> +{
>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>> +	struct drm_device *drm = bridge->dev;
>> +
>> +	drm_connector_unregister(&ctx->connector);
>> +	drm_fb_helper_remove_one_connector(drm->fb_helper, &ctx->connector);
>> +	drm_panel_detach(ctx->panel);
>> +	ctx->panel = NULL;
>> +	drm_connector_unreference(&ctx->connector);
>> +}
>> +
>> +static const struct drm_bridge_funcs tc358764_bridge_funcs = {
>> +	.disable = tc358764_disable,
>> +	.post_disable = tc358764_post_disable,
>> +	.enable = tc358764_enable,
>> +	.pre_enable = tc358764_pre_enable,
>> +	.attach = tc358764_attach,
>> +	.detach = tc358764_detach,
>> +};
>> +
>> +static int tc358764_parse_dt(struct tc358764 *ctx)
>> +{
>> +	struct device *dev = ctx->dev;
>> +	int ret;
>> +
>> +	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->gpio_reset)) {
>> +		dev_err(dev, "no reset GPIO pin provided\n");
>> +		return PTR_ERR(ctx->gpio_reset);
>> +	}
>> +
>> +	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
>> +					  NULL);
>> +	if (ret && ret != -EPROBE_DEFER)
>> +		dev_err(dev, "cannot find panel (%d)\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int tc358764_configure_regulators(struct tc358764 *ctx)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); ++i)
>> +		ctx->supplies[i].supply = tc358764_supplies[i];
>> +
>> +	ret = devm_regulator_bulk_get(ctx->dev, ARRAY_SIZE(ctx->supplies),
>> +				      ctx->supplies);
>> +	if (ret < 0)
>> +		dev_err(ctx->dev, "failed to get regulators: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int tc358764_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device *dev = &dsi->dev;
>> +	struct tc358764 *ctx;
>> +	int ret;
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(struct tc358764), GFP_KERNEL);
>> +	if (!ctx)
>> +		return -ENOMEM;
>> +
>> +	mipi_dsi_set_drvdata(dsi, ctx);
>> +
>> +	ctx->dev = dev;
>> +
>> +	dsi->lanes = 4;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
>> +		| MIPI_DSI_MODE_VIDEO_AUTO_VERT | MIPI_DSI_MODE_LPM;
>> +
>> +	ret = tc358764_parse_dt(ctx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = tc358764_configure_regulators(ctx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ctx->bridge.funcs = &tc358764_bridge_funcs;
>> +	ctx->bridge.of_node = dev->of_node;
>> +
>> +	drm_bridge_add(&ctx->bridge);
>> +
>> +	ret = mipi_dsi_attach(dsi);
>> +	if (ret < 0) {
>> +		drm_bridge_remove(&ctx->bridge);
>> +		dev_err(dev, "failed to attach dsi\n");
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int tc358764_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct tc358764 *ctx = mipi_dsi_get_drvdata(dsi);
>> +
>> +	mipi_dsi_detach(dsi);
>> +	drm_bridge_remove(&ctx->bridge);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id tc358764_of_match[] = {
>> +	{ .compatible = "toshiba,tc358764" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, tc358764_of_match);
>> +
>> +static struct mipi_dsi_driver tc358764_driver = {
>> +	.probe = tc358764_probe,
>> +	.remove = tc358764_remove,
>> +	.driver = {
>> +		.name = "tc358764",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = tc358764_of_match,
>> +	},
>> +};
>> +module_mipi_dsi_driver(tc358764_driver);
>> +
>> +MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
>> +MODULE_AUTHOR("Maciej Purski <m.purski@samsung.com>");
>> +MODULE_DESCRIPTION("MIPI-DSI based Driver for TC358764 DSI/LVDS Bridge");
>> +MODULE_LICENSE("GPL v2");
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 5/9] drm/bridge: tc358764: Add DSI to LVDS bridge driver

[Laurent Pinchart]

Hi Andrzej,

On Friday, 27 July 2018 10:17:50 EEST Andrzej Hajda wrote:
> On 26.07.2018 09:36, Archit Taneja wrote:
> > On Wednesday 25 July 2018 09:16 PM, Andrzej Hajda wrote:
> >> Add a drm_bridge driver for the Toshiba TC358764 DSI to LVDS bridge.
> >> 
> >> Changes in v4:
> >> - removed license blob,
> >> - ordered includes,
> >> - added error handling,
> >> - fixed reset GPIO handling,
> >> - added missing calls to the panel,
> >> - custom OF graph code replaced with helpers,
> >> - removed tc358764_poweroff from remove callback.
> >> v5:
> >> - fixed supply names,
> >> - fixed broken console - added connector to fb_helper,
> >> - added detach callback - unbinding works,
> >> - fixed typo in error checking code,
> >> - removed sparse bridge->encoder check - core does it already.
> >> 
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >> [ a.hajda@samsung.com: v4, v5 ]
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> ---
> >> 
> >>   drivers/gpu/drm/bridge/Kconfig    |   8 +
> >>   drivers/gpu/drm/bridge/Makefile   |   1 +
> >>   drivers/gpu/drm/bridge/tc358764.c | 499 ++++++++++++++++++++++++++++++
> >>   3 files changed, 508 insertions(+)
> >>   create mode 100644 drivers/gpu/drm/bridge/tc358764.c
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/Kconfig
> >> b/drivers/gpu/drm/bridge/Kconfig index fa2c7997e2fd..f3da8a716833 100644
> >> --- a/drivers/gpu/drm/bridge/Kconfig
> >> +++ b/drivers/gpu/drm/bridge/Kconfig
> >> @@ -110,6 +110,14 @@ config DRM_THINE_THC63LVD1024
> >> 
> >>   	---help---
> >>   	
> >>   	  Thine THC63LVD1024 LVDS/parallel converter driver.
> >> 
> >> +config DRM_TOSHIBA_TC358764
> >> +	tristate "TC358764 DSI/LVDS bridge"
> >> +	depends on DRM && DRM_PANEL
> >> +	depends on OF
> >> +	select DRM_MIPI_DSI
> >> +	help
> >> +	  Toshiba TC358764 DSI/LVDS bridge driver.
> >> +
> >> 
> >>   config DRM_TOSHIBA_TC358767
> >>   
> >>   	tristate "Toshiba TC358767 eDP bridge"
> >>   	depends on OF
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/Makefile
> >> b/drivers/gpu/drm/bridge/Makefile index 35f88d48ec20..bf7c0cecf227
> >> 100644
> >> --- a/drivers/gpu/drm/bridge/Makefile
> >> +++ b/drivers/gpu/drm/bridge/Makefile
> >> @@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> >> 
> >>   obj-$(CONFIG_DRM_SII902X) += sii902x.o
> >>   obj-$(CONFIG_DRM_SII9234) += sii9234.o
> >>   obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
> >> 
> >> +obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
> >> 
> >>   obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> >>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> >>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/tc358764.c
> >> b/drivers/gpu/drm/bridge/tc358764.c new file mode 100644
> >> index 000000000000..779bc5fce22a
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/bridge/tc358764.c
> >> @@ -0,0 +1,499 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2018 Samsung Electronics Co., Ltd
> >> + *
> >> + * Authors:
> >> + *	Andrzej Hajda <a.hajda@samsung.com>
> >> + *	Maciej Purski <m.purski@samsung.com>
> >> + */
> >> +
> >> +#include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_crtc.h>
> >> +#include <drm/drm_crtc_helper.h>
> >> +#include <drm/drm_fb_helper.h>
> >> +#include <drm/drm_mipi_dsi.h>
> >> +#include <drm/drm_of.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drmP.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/of_graph.h>
> >> +#include <linux/regulator/consumer.h>
> >> +#include <video/mipi_display.h>
> >> +
> >> +#define FLD_MASK(start, end)    (((1 << ((start) - (end) + 1)) - 1) <<
> >> (end)) +#define FLD_VAL(val, start, end) (((val) << (end)) &
> >> FLD_MASK(start, end)) +
> >> +/* PPI layer registers */
> >> +#define PPI_STARTPPI		0x0104 /* START control bit */
> >> +#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
> >> +#define PPI_LANEENABLE		0x0134 /* Enables each lane */
> >> +#define PPI_TX_RX_TA		0x013C /* BTA timing parameters */
> >> +#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
> >> +#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
> >> +#define PPI_D2S_CLRSIPOCOUNT	0x016C /* Assertion timer for Lane 2 */
> >> +#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
> >> +#define PPI_START_FUNCTION	1
> >> +
> >> +/* DSI layer registers */
> >> +#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
> >> +#define DSI_LANEENABLE		0x0210 /* Enables each lane */
> >> +#define DSI_RX_START		1
> >> +
> >> +/* Video path registers */
> >> +#define VP_CTRL			0x0450 /* Video Path Control */
> >> +#define VP_CTRL_MSF(v)		FLD_VAL(v, 0, 0) /* Magic square in RGB666 
*/
> >> +#define VP_CTRL_VTGEN(v)	FLD_VAL(v, 4, 4) /* Use chip clock for timing
> >> */
> >> +#define VP_CTRL_EVTMODE(v)	FLD_VAL(v, 5, 5) /* Event mode */
> >> +#define VP_CTRL_RGB888(v)	FLD_VAL(v, 8, 8) /* RGB888 mode */
> >> +#define VP_CTRL_VSDELAY(v)	FLD_VAL(v, 31, 20) /* VSYNC delay */
> >> +#define VP_CTRL_HSPOL		BIT(17) /* Polarity of HSYNC signal */
> >> +#define VP_CTRL_DEPOL		BIT(18) /* Polarity of DE signal */
> >> +#define VP_CTRL_VSPOL		BIT(19) /* Polarity of VSYNC signal */
> >> +#define VP_HTIM1		0x0454 /* Horizontal Timing Control 1 */
> >> +#define VP_HTIM1_HBP(v)		FLD_VAL(v, 24, 16)
> >> +#define VP_HTIM1_HSYNC(v)	FLD_VAL(v, 8, 0)
> >> +#define VP_HTIM2		0x0458 /* Horizontal Timing Control 2 */
> >> +#define VP_HTIM2_HFP(v)		FLD_VAL(v, 24, 16)
> >> +#define VP_HTIM2_HACT(v)	FLD_VAL(v, 10, 0)
> >> +#define VP_VTIM1		0x045C /* Vertical Timing Control 1 */
> >> +#define VP_VTIM1_VBP(v)		FLD_VAL(v, 23, 16)
> >> +#define VP_VTIM1_VSYNC(v)	FLD_VAL(v, 7, 0)
> >> +#define VP_VTIM2		0x0460 /* Vertical Timing Control 2 */
> >> +#define VP_VTIM2_VFP(v)		FLD_VAL(v, 23, 16)
> >> +#define VP_VTIM2_VACT(v)	FLD_VAL(v, 10, 0)
> >> +#define VP_VFUEN		0x0464 /* Video Frame Timing Update Enable */
> >> +
> >> +/* LVDS registers */
> >> +#define LV_MX0003		0x0480 /* Mux input bit 0 to 3 */
> >> +#define LV_MX0407		0x0484 /* Mux input bit 4 to 7 */
> >> +#define LV_MX0811		0x0488 /* Mux input bit 8 to 11 */
> >> +#define LV_MX1215		0x048C /* Mux input bit 12 to 15 */
> >> +#define LV_MX1619		0x0490 /* Mux input bit 16 to 19 */
> >> +#define LV_MX2023		0x0494 /* Mux input bit 20 to 23 */
> >> +#define LV_MX2427		0x0498 /* Mux input bit 24 to 27 */
> >> +#define LV_MX(b0, b1, b2, b3)	(FLD_VAL(b0, 4, 0) | FLD_VAL(b1, 12, 8) 
|
> >> \
> >> +				FLD_VAL(b2, 20, 16) | FLD_VAL(b3, 28, 24))
> >> +
> >> +/* Input bit numbers used in mux registers */
> >> +enum {
> >> +	LVI_R0,
> >> +	LVI_R1,
> >> +	LVI_R2,
> >> +	LVI_R3,
> >> +	LVI_R4,
> >> +	LVI_R5,
> >> +	LVI_R6,
> >> +	LVI_R7,
> >> +	LVI_G0,
> >> +	LVI_G1,
> >> +	LVI_G2,
> >> +	LVI_G3,
> >> +	LVI_G4,
> >> +	LVI_G5,
> >> +	LVI_G6,
> >> +	LVI_G7,
> >> +	LVI_B0,
> >> +	LVI_B1,
> >> +	LVI_B2,
> >> +	LVI_B3,
> >> +	LVI_B4,
> >> +	LVI_B5,
> >> +	LVI_B6,
> >> +	LVI_B7,
> >> +	LVI_HS,
> >> +	LVI_VS,
> >> +	LVI_DE,
> >> +	LVI_L0
> >> +};
> >> +
> >> +#define LV_CFG			0x049C /* LVDS Configuration */
> >> +#define LV_PHY0			0x04A0 /* LVDS PHY 0 */
> >> +#define LV_PHY0_RST(v)		FLD_VAL(v, 22, 22) /* PHY reset */
> >> +#define LV_PHY0_IS(v)		FLD_VAL(v, 15, 14)
> >> +#define LV_PHY0_ND(v)		FLD_VAL(v, 4, 0) /* Frequency range select */
> >> +#define LV_PHY0_PRBS_ON(v)	FLD_VAL(v, 20, 16) /* Clock/Data Flag pins 
*/
> >> +
> >> +/* System registers */
> >> +#define SYS_RST			0x0504 /* System Reset */
> >> +#define SYS_ID			0x0580 /* System ID */
> >> +
> >> +#define SYS_RST_I2CS		BIT(0) /* Reset I2C-Slave controller */
> >> +#define SYS_RST_I2CM		BIT(1) /* Reset I2C-Master controller */
> >> +#define SYS_RST_LCD		BIT(2) /* Reset LCD controller */
> >> +#define SYS_RST_BM		BIT(3) /* Reset Bus Management controller */
> >> +#define SYS_RST_DSIRX		BIT(4) /* Reset DSI-RX and App controller */
> >> +#define SYS_RST_REG		BIT(5) /* Reset Register module */
> >> +
> >> +#define LPX_PERIOD		2
> >> +#define TTA_SURE		3
> >> +#define TTA_GET			0x20000
> >> +
> >> +/* Lane enable PPI and DSI register bits */
> >> +#define LANEENABLE_CLEN		BIT(0)
> >> +#define LANEENABLE_L0EN		BIT(1)
> >> +#define LANEENABLE_L1EN		BIT(2)
> >> +#define LANEENABLE_L2EN		BIT(3)
> >> +#define LANEENABLE_L3EN		BIT(4)
> >> +
> >> +/* LVCFG fields */
> >> +#define LV_CFG_LVEN		BIT(0)
> >> +#define LV_CFG_LVDLINK		BIT(1)
> >> +#define LV_CFG_CLKPOL1		BIT(2)
> >> +#define LV_CFG_CLKPOL2		BIT(3)
> >> +
> >> +static const char * const tc358764_supplies[] = {
> >> +	"vddc", "vddio", "vddlvds"
> >> +};
> >> +
> >> +struct tc358764 {
> >> +	struct device *dev;
> >> +	struct drm_bridge bridge;
> >> +	struct drm_connector connector;
> >> +	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
> >> +	struct gpio_desc *gpio_reset;
> >> +	struct drm_panel *panel;
> >> +	int error;
> >> +};
> >> +
> >> +static int tc358764_clear_error(struct tc358764 *ctx)
> >> +{
> >> +	int ret = ctx->error;
> >> +
> >> +	ctx->error = 0;
> >> +	return ret;
> >> +}
> >> +
> >> +static void tc358764_read(struct tc358764 *ctx, u16 addr, u32 *val)
> >> +{
> >> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> >> +	ssize_t ret;
> >> +
> >> +	if (ctx->error)
> >> +		return;
> >> +
> >> +	cpu_to_le16s(&addr);
> >> +	ret = mipi_dsi_generic_read(dsi, &addr, sizeof(addr), val,
> >> sizeof(*val));
> >> +	if (ret >= 0)
> >> +		le32_to_cpus(val);
> >> +
> >> +	dev_dbg(ctx->dev, "read: %d, addr: %d\n", addr, *val);
> >> +}
> >> +
> >> +static void tc358764_write(struct tc358764 *ctx, u16 addr, u32 val)
> >> +{
> >> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> >> +	ssize_t ret;
> >> +	u8 data[6];
> >> +
> >> +	if (ctx->error)
> >> +		return;
> >> +
> >> +	data[0] = addr;
> >> +	data[1] = addr >> 8;
> >> +	data[2] = val;
> >> +	data[3] = val >> 8;
> >> +	data[4] = val >> 16;
> >> +	data[5] = val >> 24;
> >> +
> >> +	ret = mipi_dsi_generic_write(dsi, data, sizeof(data));
> >> +	if (ret < 0)
> >> +		ctx->error = ret;
> >> +}
> >> +
> >> +static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge
> >> *bridge) +{
> >> +	return container_of(bridge, struct tc358764, bridge);
> >> +}
> >> +
> >> +static inline
> >> +struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
> >> +{
> >> +	return container_of(connector, struct tc358764, connector);
> >> +}
> >> +
> >> +static int tc358764_init(struct tc358764 *ctx)
> >> +{
> >> +	u32 v = 0;
> >> +
> >> +	tc358764_read(ctx, SYS_ID, &v);
> >> +	if (ctx->error)
> >> +		return tc358764_clear_error(ctx);
> >> +	dev_info(ctx->dev, "ID: %#x\n", v);
> >> +
> >> +	/* configure PPI counters */
> >> +	tc358764_write(ctx, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
> >> +	tc358764_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
> >> +	tc358764_write(ctx, PPI_D0S_CLRSIPOCOUNT, 5);
> >> +	tc358764_write(ctx, PPI_D1S_CLRSIPOCOUNT, 5);
> >> +	tc358764_write(ctx, PPI_D2S_CLRSIPOCOUNT, 5);
> >> +	tc358764_write(ctx, PPI_D3S_CLRSIPOCOUNT, 5);
> >> +
> >> +	/* enable four data lanes and clock lane */
> >> +	tc358764_write(ctx, PPI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
> >> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
> >> +	tc358764_write(ctx, DSI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
> >> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
> >> +
> >> +	/* start */
> >> +	tc358764_write(ctx, PPI_STARTPPI, PPI_START_FUNCTION);
> >> +	tc358764_write(ctx, DSI_STARTDSI, DSI_RX_START);
> >> +
> >> +	/* configure video path */
> >> +	tc358764_write(ctx, VP_CTRL, VP_CTRL_VSDELAY(15) | VP_CTRL_RGB888(1) |
> >> +		       VP_CTRL_EVTMODE(1) | VP_CTRL_HSPOL | VP_CTRL_VSPOL);
> >> +
> >> +	/* reset PHY */
> >> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_RST(1) |
> >> +		       LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) | LV_PHY0_ND(6));
> >> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) |
> >> +		       LV_PHY0_ND(6));
> >> +
> >> +	/* reset bridge */
> >> +	tc358764_write(ctx, SYS_RST, SYS_RST_LCD);
> >> +
> >> +	/* set bit order */
> >> +	tc358764_write(ctx, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> >> +	tc358764_write(ctx, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> >> +	tc358764_write(ctx, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7));
> >> +	tc358764_write(ctx, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
> >> +	tc358764_write(ctx, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
> >> +	tc358764_write(ctx, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
> >> +	tc358764_write(ctx, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
> >> +	tc358764_write(ctx, LV_CFG, LV_CFG_CLKPOL2 | LV_CFG_CLKPOL1 |
> >> +		       LV_CFG_LVEN);
> >> +
> >> +	return tc358764_clear_error(ctx);
> >> +}
> >> +
> >> +static void tc358764_reset(struct tc358764 *ctx)
> >> +{
> >> +	gpiod_set_value(ctx->gpio_reset, 1);
> >> +	usleep_range(1000, 2000);
> >> +	gpiod_set_value(ctx->gpio_reset, 0);
> >> +	usleep_range(1000, 2000);
> >> +}
> >> +
> >> +static int tc358764_get_modes(struct drm_connector *connector)
> >> +{
> >> +	struct tc358764 *ctx = connector_to_tc358764(connector);
> >> +
> >> +	return drm_panel_get_modes(ctx->panel);
> >> +}
> >> +
> >> +static const
> >> +struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
> >> +	.get_modes = tc358764_get_modes,
> >> +};
> >> +
> >> +static const struct drm_connector_funcs tc358764_connector_funcs = {
> >> +	.fill_modes = drm_helper_probe_single_connector_modes,
> >> +	.destroy = drm_connector_cleanup,
> >> +	.reset = drm_atomic_helper_connector_reset,
> >> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> >> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >> +};
> >> +
> >> +static void tc358764_disable(struct drm_bridge *bridge)
> >> +{
> >> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >> +	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
> >> +
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
> >> +}
> >> +
> >> +static void tc358764_post_disable(struct drm_bridge *bridge)
> >> +{
> >> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >> +	int ret;
> >> +
> >> +	ret = drm_panel_unprepare(ctx->panel);
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
> >> +	tc358764_reset(ctx);
> >> +	usleep_range(10000, 15000);
> >> +	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
> >> +}
> >> +
> >> +static void tc358764_pre_enable(struct drm_bridge *bridge)
> >> +{
> >> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >> +	int ret;
> >> +
> >> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error enabling regulators (%d)\n", ret);
> >> +	usleep_range(10000, 15000);
> >> +	tc358764_reset(ctx);
> >> +	ret = tc358764_init(ctx);
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> >> +	ret = drm_panel_prepare(ctx->panel);
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
> >> +}
> >> +
> >> +static void tc358764_enable(struct drm_bridge *bridge)
> >> +{
> >> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >> +	int ret = drm_panel_enable(ctx->panel);
> >> +
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
> >> +}
> >> +
> >> +static int tc358764_attach(struct drm_bridge *bridge)
> >> +{
> >> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >> +	struct drm_device *drm = bridge->dev;
> >> +	int ret;
> >> +
> >> +	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
> >> +	ret = drm_connector_init(drm, &ctx->connector,
> >> +				 &tc358764_connector_funcs,
> >> +				 DRM_MODE_CONNECTOR_LVDS);
> >> +	if (ret) {
> >> +		DRM_ERROR("Failed to initialize connector\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	drm_connector_helper_add(&ctx->connector,
> >> +				 &tc358764_connector_helper_funcs);
> >> +	drm_mode_connector_attach_encoder(&ctx->connector, bridge->encoder);
> >> +	drm_panel_attach(ctx->panel, &ctx->connector);
> >> +	ctx->connector.funcs->reset(&ctx->connector);
> >> +	drm_fb_helper_add_one_connector(drm->fb_helper, &ctx->connector);
> >> +	drm_connector_register(&ctx->connector);
> > 
> > Aren't drm_connector_register()/unregister calls managed by the drm
> > core?
> 
> drm core registers connectors during initialization but tc358764_attach
> can be called after bridge probe, also after drm initialization, in such
> case connector should be dynamically allocated/de-allocated.

This really, really, really calls for *NOT* creating drm_connector instances 
in bridge drivers. This has been an issue since the beginning and we need to 
fix it. Connectors should be created by display drivers, and connector 
operations implemented using operations provided by bridges.

Futhermore, registering and unregistering connectors after probe time isn't 
supported in the DRM core, this will lead to nasty races with userspace. This 
is also something that we should fix, but it will be much more difficult to 
tackle.

> > Otherwise:
> > Reviewed-by: Archit Taneja <architt@codeaurora.org>
> 
> Thanks for the review, queued to drm-misc-next.

With the above issues ? :-(

> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void tc358764_detach(struct drm_bridge *bridge)
> >> +{
> >> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >> +	struct drm_device *drm = bridge->dev;
> >> +
> >> +	drm_connector_unregister(&ctx->connector);
> >> +	drm_fb_helper_remove_one_connector(drm->fb_helper, &ctx->connector);
> >> +	drm_panel_detach(ctx->panel);
> >> +	ctx->panel = NULL;
> >> +	drm_connector_unreference(&ctx->connector);
> >> +}
> >> +
> >> +static const struct drm_bridge_funcs tc358764_bridge_funcs = {
> >> +	.disable = tc358764_disable,
> >> +	.post_disable = tc358764_post_disable,
> >> +	.enable = tc358764_enable,
> >> +	.pre_enable = tc358764_pre_enable,
> >> +	.attach = tc358764_attach,
> >> +	.detach = tc358764_detach,
> >> +};
> >> +
> >> +static int tc358764_parse_dt(struct tc358764 *ctx)
> >> +{
> >> +	struct device *dev = ctx->dev;
> >> +	int ret;
> >> +
> >> +	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >> +	if (IS_ERR(ctx->gpio_reset)) {
> >> +		dev_err(dev, "no reset GPIO pin provided\n");
> >> +		return PTR_ERR(ctx->gpio_reset);
> >> +	}
> >> +
> >> +	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
> >> +					  NULL);
> >> +	if (ret && ret != -EPROBE_DEFER)
> >> +		dev_err(dev, "cannot find panel (%d)\n", ret);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int tc358764_configure_regulators(struct tc358764 *ctx)
> >> +{
> >> +	int i, ret;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); ++i)
> >> +		ctx->supplies[i].supply = tc358764_supplies[i];
> >> +
> >> +	ret = devm_regulator_bulk_get(ctx->dev, ARRAY_SIZE(ctx->supplies),
> >> +				      ctx->supplies);
> >> +	if (ret < 0)
> >> +		dev_err(ctx->dev, "failed to get regulators: %d\n", ret);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int tc358764_probe(struct mipi_dsi_device *dsi)
> >> +{
> >> +	struct device *dev = &dsi->dev;
> >> +	struct tc358764 *ctx;
> >> +	int ret;
> >> +
> >> +	ctx = devm_kzalloc(dev, sizeof(struct tc358764), GFP_KERNEL);
> >> +	if (!ctx)
> >> +		return -ENOMEM;
> >> +
> >> +	mipi_dsi_set_drvdata(dsi, ctx);
> >> +
> >> +	ctx->dev = dev;
> >> +
> >> +	dsi->lanes = 4;
> >> +	dsi->format = MIPI_DSI_FMT_RGB888;
> >> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
> >> +		| MIPI_DSI_MODE_VIDEO_AUTO_VERT | MIPI_DSI_MODE_LPM;
> >> +
> >> +	ret = tc358764_parse_dt(ctx);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	ret = tc358764_configure_regulators(ctx);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	ctx->bridge.funcs = &tc358764_bridge_funcs;
> >> +	ctx->bridge.of_node = dev->of_node;
> >> +
> >> +	drm_bridge_add(&ctx->bridge);
> >> +
> >> +	ret = mipi_dsi_attach(dsi);
> >> +	if (ret < 0) {
> >> +		drm_bridge_remove(&ctx->bridge);
> >> +		dev_err(dev, "failed to attach dsi\n");
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int tc358764_remove(struct mipi_dsi_device *dsi)
> >> +{
> >> +	struct tc358764 *ctx = mipi_dsi_get_drvdata(dsi);
> >> +
> >> +	mipi_dsi_detach(dsi);
> >> +	drm_bridge_remove(&ctx->bridge);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct of_device_id tc358764_of_match[] = {
> >> +	{ .compatible = "toshiba,tc358764" },
> >> +	{ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, tc358764_of_match);
> >> +
> >> +static struct mipi_dsi_driver tc358764_driver = {
> >> +	.probe = tc358764_probe,
> >> +	.remove = tc358764_remove,
> >> +	.driver = {
> >> +		.name = "tc358764",
> >> +		.owner = THIS_MODULE,
> >> +		.of_match_table = tc358764_of_match,
> >> +	},
> >> +};
> >> +module_mipi_dsi_driver(tc358764_driver);
> >> +
> >> +MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
> >> +MODULE_AUTHOR("Maciej Purski <m.purski@samsung.com>");
> >> +MODULE_DESCRIPTION("MIPI-DSI based Driver for TC358764 DSI/LVDS
> >> Bridge");
> >> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 5/9] drm/bridge: tc358764: Add DSI to LVDS bridge driver

[Andrzej Hajda]

On 27.07.2018 12:30, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Friday, 27 July 2018 10:17:50 EEST Andrzej Hajda wrote:
>> On 26.07.2018 09:36, Archit Taneja wrote:
>>> On Wednesday 25 July 2018 09:16 PM, Andrzej Hajda wrote:
>>>> Add a drm_bridge driver for the Toshiba TC358764 DSI to LVDS bridge.
>>>>
>>>> Changes in v4:
>>>> - removed license blob,
>>>> - ordered includes,
>>>> - added error handling,
>>>> - fixed reset GPIO handling,
>>>> - added missing calls to the panel,
>>>> - custom OF graph code replaced with helpers,
>>>> - removed tc358764_poweroff from remove callback.
>>>> v5:
>>>> - fixed supply names,
>>>> - fixed broken console - added connector to fb_helper,
>>>> - added detach callback - unbinding works,
>>>> - fixed typo in error checking code,
>>>> - removed sparse bridge->encoder check - core does it already.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>> [ a.hajda@samsung.com: v4, v5 ]
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>>
>>>>   drivers/gpu/drm/bridge/Kconfig    |   8 +
>>>>   drivers/gpu/drm/bridge/Makefile   |   1 +
>>>>   drivers/gpu/drm/bridge/tc358764.c | 499 ++++++++++++++++++++++++++++++
>>>>   3 files changed, 508 insertions(+)
>>>>   create mode 100644 drivers/gpu/drm/bridge/tc358764.c
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>>>> b/drivers/gpu/drm/bridge/Kconfig index fa2c7997e2fd..f3da8a716833 100644
>>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>>> @@ -110,6 +110,14 @@ config DRM_THINE_THC63LVD1024
>>>>
>>>>   	---help---
>>>>   	
>>>>   	  Thine THC63LVD1024 LVDS/parallel converter driver.
>>>>
>>>> +config DRM_TOSHIBA_TC358764
>>>> +	tristate "TC358764 DSI/LVDS bridge"
>>>> +	depends on DRM && DRM_PANEL
>>>> +	depends on OF
>>>> +	select DRM_MIPI_DSI
>>>> +	help
>>>> +	  Toshiba TC358764 DSI/LVDS bridge driver.
>>>> +
>>>>
>>>>   config DRM_TOSHIBA_TC358767
>>>>   
>>>>   	tristate "Toshiba TC358767 eDP bridge"
>>>>   	depends on OF
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/Makefile
>>>> b/drivers/gpu/drm/bridge/Makefile index 35f88d48ec20..bf7c0cecf227
>>>> 100644
>>>> --- a/drivers/gpu/drm/bridge/Makefile
>>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>>> @@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>>
>>>>   obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>>   obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>>>   obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>>>
>>>> +obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
>>>>
>>>>   obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358764.c
>>>> b/drivers/gpu/drm/bridge/tc358764.c new file mode 100644
>>>> index 000000000000..779bc5fce22a
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/bridge/tc358764.c
>>>> @@ -0,0 +1,499 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) 2018 Samsung Electronics Co., Ltd
>>>> + *
>>>> + * Authors:
>>>> + *	Andrzej Hajda <a.hajda@samsung.com>
>>>> + *	Maciej Purski <m.purski@samsung.com>
>>>> + */
>>>> +
>>>> +#include <drm/drm_atomic_helper.h>
>>>> +#include <drm/drm_crtc.h>
>>>> +#include <drm/drm_crtc_helper.h>
>>>> +#include <drm/drm_fb_helper.h>
>>>> +#include <drm/drm_mipi_dsi.h>
>>>> +#include <drm/drm_of.h>
>>>> +#include <drm/drm_panel.h>
>>>> +#include <drm/drmP.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <video/mipi_display.h>
>>>> +
>>>> +#define FLD_MASK(start, end)    (((1 << ((start) - (end) + 1)) - 1) <<
>>>> (end)) +#define FLD_VAL(val, start, end) (((val) << (end)) &
>>>> FLD_MASK(start, end)) +
>>>> +/* PPI layer registers */
>>>> +#define PPI_STARTPPI		0x0104 /* START control bit */
>>>> +#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
>>>> +#define PPI_LANEENABLE		0x0134 /* Enables each lane */
>>>> +#define PPI_TX_RX_TA		0x013C /* BTA timing parameters */
>>>> +#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
>>>> +#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
>>>> +#define PPI_D2S_CLRSIPOCOUNT	0x016C /* Assertion timer for Lane 2 */
>>>> +#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
>>>> +#define PPI_START_FUNCTION	1
>>>> +
>>>> +/* DSI layer registers */
>>>> +#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
>>>> +#define DSI_LANEENABLE		0x0210 /* Enables each lane */
>>>> +#define DSI_RX_START		1
>>>> +
>>>> +/* Video path registers */
>>>> +#define VP_CTRL			0x0450 /* Video Path Control */
>>>> +#define VP_CTRL_MSF(v)		FLD_VAL(v, 0, 0) /* Magic square in RGB666 
> */
>>>> +#define VP_CTRL_VTGEN(v)	FLD_VAL(v, 4, 4) /* Use chip clock for timing
>>>> */
>>>> +#define VP_CTRL_EVTMODE(v)	FLD_VAL(v, 5, 5) /* Event mode */
>>>> +#define VP_CTRL_RGB888(v)	FLD_VAL(v, 8, 8) /* RGB888 mode */
>>>> +#define VP_CTRL_VSDELAY(v)	FLD_VAL(v, 31, 20) /* VSYNC delay */
>>>> +#define VP_CTRL_HSPOL		BIT(17) /* Polarity of HSYNC signal */
>>>> +#define VP_CTRL_DEPOL		BIT(18) /* Polarity of DE signal */
>>>> +#define VP_CTRL_VSPOL		BIT(19) /* Polarity of VSYNC signal */
>>>> +#define VP_HTIM1		0x0454 /* Horizontal Timing Control 1 */
>>>> +#define VP_HTIM1_HBP(v)		FLD_VAL(v, 24, 16)
>>>> +#define VP_HTIM1_HSYNC(v)	FLD_VAL(v, 8, 0)
>>>> +#define VP_HTIM2		0x0458 /* Horizontal Timing Control 2 */
>>>> +#define VP_HTIM2_HFP(v)		FLD_VAL(v, 24, 16)
>>>> +#define VP_HTIM2_HACT(v)	FLD_VAL(v, 10, 0)
>>>> +#define VP_VTIM1		0x045C /* Vertical Timing Control 1 */
>>>> +#define VP_VTIM1_VBP(v)		FLD_VAL(v, 23, 16)
>>>> +#define VP_VTIM1_VSYNC(v)	FLD_VAL(v, 7, 0)
>>>> +#define VP_VTIM2		0x0460 /* Vertical Timing Control 2 */
>>>> +#define VP_VTIM2_VFP(v)		FLD_VAL(v, 23, 16)
>>>> +#define VP_VTIM2_VACT(v)	FLD_VAL(v, 10, 0)
>>>> +#define VP_VFUEN		0x0464 /* Video Frame Timing Update Enable */
>>>> +
>>>> +/* LVDS registers */
>>>> +#define LV_MX0003		0x0480 /* Mux input bit 0 to 3 */
>>>> +#define LV_MX0407		0x0484 /* Mux input bit 4 to 7 */
>>>> +#define LV_MX0811		0x0488 /* Mux input bit 8 to 11 */
>>>> +#define LV_MX1215		0x048C /* Mux input bit 12 to 15 */
>>>> +#define LV_MX1619		0x0490 /* Mux input bit 16 to 19 */
>>>> +#define LV_MX2023		0x0494 /* Mux input bit 20 to 23 */
>>>> +#define LV_MX2427		0x0498 /* Mux input bit 24 to 27 */
>>>> +#define LV_MX(b0, b1, b2, b3)	(FLD_VAL(b0, 4, 0) | FLD_VAL(b1, 12, 8) 
> |
>>>> \
>>>> +				FLD_VAL(b2, 20, 16) | FLD_VAL(b3, 28, 24))
>>>> +
>>>> +/* Input bit numbers used in mux registers */
>>>> +enum {
>>>> +	LVI_R0,
>>>> +	LVI_R1,
>>>> +	LVI_R2,
>>>> +	LVI_R3,
>>>> +	LVI_R4,
>>>> +	LVI_R5,
>>>> +	LVI_R6,
>>>> +	LVI_R7,
>>>> +	LVI_G0,
>>>> +	LVI_G1,
>>>> +	LVI_G2,
>>>> +	LVI_G3,
>>>> +	LVI_G4,
>>>> +	LVI_G5,
>>>> +	LVI_G6,
>>>> +	LVI_G7,
>>>> +	LVI_B0,
>>>> +	LVI_B1,
>>>> +	LVI_B2,
>>>> +	LVI_B3,
>>>> +	LVI_B4,
>>>> +	LVI_B5,
>>>> +	LVI_B6,
>>>> +	LVI_B7,
>>>> +	LVI_HS,
>>>> +	LVI_VS,
>>>> +	LVI_DE,
>>>> +	LVI_L0
>>>> +};
>>>> +
>>>> +#define LV_CFG			0x049C /* LVDS Configuration */
>>>> +#define LV_PHY0			0x04A0 /* LVDS PHY 0 */
>>>> +#define LV_PHY0_RST(v)		FLD_VAL(v, 22, 22) /* PHY reset */
>>>> +#define LV_PHY0_IS(v)		FLD_VAL(v, 15, 14)
>>>> +#define LV_PHY0_ND(v)		FLD_VAL(v, 4, 0) /* Frequency range select */
>>>> +#define LV_PHY0_PRBS_ON(v)	FLD_VAL(v, 20, 16) /* Clock/Data Flag pins 
> */
>>>> +
>>>> +/* System registers */
>>>> +#define SYS_RST			0x0504 /* System Reset */
>>>> +#define SYS_ID			0x0580 /* System ID */
>>>> +
>>>> +#define SYS_RST_I2CS		BIT(0) /* Reset I2C-Slave controller */
>>>> +#define SYS_RST_I2CM		BIT(1) /* Reset I2C-Master controller */
>>>> +#define SYS_RST_LCD		BIT(2) /* Reset LCD controller */
>>>> +#define SYS_RST_BM		BIT(3) /* Reset Bus Management controller */
>>>> +#define SYS_RST_DSIRX		BIT(4) /* Reset DSI-RX and App controller */
>>>> +#define SYS_RST_REG		BIT(5) /* Reset Register module */
>>>> +
>>>> +#define LPX_PERIOD		2
>>>> +#define TTA_SURE		3
>>>> +#define TTA_GET			0x20000
>>>> +
>>>> +/* Lane enable PPI and DSI register bits */
>>>> +#define LANEENABLE_CLEN		BIT(0)
>>>> +#define LANEENABLE_L0EN		BIT(1)
>>>> +#define LANEENABLE_L1EN		BIT(2)
>>>> +#define LANEENABLE_L2EN		BIT(3)
>>>> +#define LANEENABLE_L3EN		BIT(4)
>>>> +
>>>> +/* LVCFG fields */
>>>> +#define LV_CFG_LVEN		BIT(0)
>>>> +#define LV_CFG_LVDLINK		BIT(1)
>>>> +#define LV_CFG_CLKPOL1		BIT(2)
>>>> +#define LV_CFG_CLKPOL2		BIT(3)
>>>> +
>>>> +static const char * const tc358764_supplies[] = {
>>>> +	"vddc", "vddio", "vddlvds"
>>>> +};
>>>> +
>>>> +struct tc358764 {
>>>> +	struct device *dev;
>>>> +	struct drm_bridge bridge;
>>>> +	struct drm_connector connector;
>>>> +	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
>>>> +	struct gpio_desc *gpio_reset;
>>>> +	struct drm_panel *panel;
>>>> +	int error;
>>>> +};
>>>> +
>>>> +static int tc358764_clear_error(struct tc358764 *ctx)
>>>> +{
>>>> +	int ret = ctx->error;
>>>> +
>>>> +	ctx->error = 0;
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void tc358764_read(struct tc358764 *ctx, u16 addr, u32 *val)
>>>> +{
>>>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>>> +	ssize_t ret;
>>>> +
>>>> +	if (ctx->error)
>>>> +		return;
>>>> +
>>>> +	cpu_to_le16s(&addr);
>>>> +	ret = mipi_dsi_generic_read(dsi, &addr, sizeof(addr), val,
>>>> sizeof(*val));
>>>> +	if (ret >= 0)
>>>> +		le32_to_cpus(val);
>>>> +
>>>> +	dev_dbg(ctx->dev, "read: %d, addr: %d\n", addr, *val);
>>>> +}
>>>> +
>>>> +static void tc358764_write(struct tc358764 *ctx, u16 addr, u32 val)
>>>> +{
>>>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>>> +	ssize_t ret;
>>>> +	u8 data[6];
>>>> +
>>>> +	if (ctx->error)
>>>> +		return;
>>>> +
>>>> +	data[0] = addr;
>>>> +	data[1] = addr >> 8;
>>>> +	data[2] = val;
>>>> +	data[3] = val >> 8;
>>>> +	data[4] = val >> 16;
>>>> +	data[5] = val >> 24;
>>>> +
>>>> +	ret = mipi_dsi_generic_write(dsi, data, sizeof(data));
>>>> +	if (ret < 0)
>>>> +		ctx->error = ret;
>>>> +}
>>>> +
>>>> +static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge
>>>> *bridge) +{
>>>> +	return container_of(bridge, struct tc358764, bridge);
>>>> +}
>>>> +
>>>> +static inline
>>>> +struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
>>>> +{
>>>> +	return container_of(connector, struct tc358764, connector);
>>>> +}
>>>> +
>>>> +static int tc358764_init(struct tc358764 *ctx)
>>>> +{
>>>> +	u32 v = 0;
>>>> +
>>>> +	tc358764_read(ctx, SYS_ID, &v);
>>>> +	if (ctx->error)
>>>> +		return tc358764_clear_error(ctx);
>>>> +	dev_info(ctx->dev, "ID: %#x\n", v);
>>>> +
>>>> +	/* configure PPI counters */
>>>> +	tc358764_write(ctx, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
>>>> +	tc358764_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
>>>> +	tc358764_write(ctx, PPI_D0S_CLRSIPOCOUNT, 5);
>>>> +	tc358764_write(ctx, PPI_D1S_CLRSIPOCOUNT, 5);
>>>> +	tc358764_write(ctx, PPI_D2S_CLRSIPOCOUNT, 5);
>>>> +	tc358764_write(ctx, PPI_D3S_CLRSIPOCOUNT, 5);
>>>> +
>>>> +	/* enable four data lanes and clock lane */
>>>> +	tc358764_write(ctx, PPI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
>>>> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
>>>> +	tc358764_write(ctx, DSI_LANEENABLE, LANEENABLE_L3EN | LANEENABLE_L2EN |
>>>> +		       LANEENABLE_L1EN | LANEENABLE_L0EN | LANEENABLE_CLEN);
>>>> +
>>>> +	/* start */
>>>> +	tc358764_write(ctx, PPI_STARTPPI, PPI_START_FUNCTION);
>>>> +	tc358764_write(ctx, DSI_STARTDSI, DSI_RX_START);
>>>> +
>>>> +	/* configure video path */
>>>> +	tc358764_write(ctx, VP_CTRL, VP_CTRL_VSDELAY(15) | VP_CTRL_RGB888(1) |
>>>> +		       VP_CTRL_EVTMODE(1) | VP_CTRL_HSPOL | VP_CTRL_VSPOL);
>>>> +
>>>> +	/* reset PHY */
>>>> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_RST(1) |
>>>> +		       LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) | LV_PHY0_ND(6));
>>>> +	tc358764_write(ctx, LV_PHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_IS(2) |
>>>> +		       LV_PHY0_ND(6));
>>>> +
>>>> +	/* reset bridge */
>>>> +	tc358764_write(ctx, SYS_RST, SYS_RST_LCD);
>>>> +
>>>> +	/* set bit order */
>>>> +	tc358764_write(ctx, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
>>>> +	tc358764_write(ctx, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
>>>> +	tc358764_write(ctx, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7));
>>>> +	tc358764_write(ctx, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
>>>> +	tc358764_write(ctx, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
>>>> +	tc358764_write(ctx, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
>>>> +	tc358764_write(ctx, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
>>>> +	tc358764_write(ctx, LV_CFG, LV_CFG_CLKPOL2 | LV_CFG_CLKPOL1 |
>>>> +		       LV_CFG_LVEN);
>>>> +
>>>> +	return tc358764_clear_error(ctx);
>>>> +}
>>>> +
>>>> +static void tc358764_reset(struct tc358764 *ctx)
>>>> +{
>>>> +	gpiod_set_value(ctx->gpio_reset, 1);
>>>> +	usleep_range(1000, 2000);
>>>> +	gpiod_set_value(ctx->gpio_reset, 0);
>>>> +	usleep_range(1000, 2000);
>>>> +}
>>>> +
>>>> +static int tc358764_get_modes(struct drm_connector *connector)
>>>> +{
>>>> +	struct tc358764 *ctx = connector_to_tc358764(connector);
>>>> +
>>>> +	return drm_panel_get_modes(ctx->panel);
>>>> +}
>>>> +
>>>> +static const
>>>> +struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
>>>> +	.get_modes = tc358764_get_modes,
>>>> +};
>>>> +
>>>> +static const struct drm_connector_funcs tc358764_connector_funcs = {
>>>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>>>> +	.destroy = drm_connector_cleanup,
>>>> +	.reset = drm_atomic_helper_connector_reset,
>>>> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>> +};
>>>> +
>>>> +static void tc358764_disable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>> +	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
>>>> +
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
>>>> +}
>>>> +
>>>> +static void tc358764_post_disable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_panel_unprepare(ctx->panel);
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
>>>> +	tc358764_reset(ctx);
>>>> +	usleep_range(10000, 15000);
>>>> +	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
>>>> +}
>>>> +
>>>> +static void tc358764_pre_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>> +	int ret;
>>>> +
>>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error enabling regulators (%d)\n", ret);
>>>> +	usleep_range(10000, 15000);
>>>> +	tc358764_reset(ctx);
>>>> +	ret = tc358764_init(ctx);
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
>>>> +	ret = drm_panel_prepare(ctx->panel);
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
>>>> +}
>>>> +
>>>> +static void tc358764_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>> +	int ret = drm_panel_enable(ctx->panel);
>>>> +
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
>>>> +}
>>>> +
>>>> +static int tc358764_attach(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>> +	struct drm_device *drm = bridge->dev;
>>>> +	int ret;
>>>> +
>>>> +	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
>>>> +	ret = drm_connector_init(drm, &ctx->connector,
>>>> +				 &tc358764_connector_funcs,
>>>> +				 DRM_MODE_CONNECTOR_LVDS);
>>>> +	if (ret) {
>>>> +		DRM_ERROR("Failed to initialize connector\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	drm_connector_helper_add(&ctx->connector,
>>>> +				 &tc358764_connector_helper_funcs);
>>>> +	drm_mode_connector_attach_encoder(&ctx->connector, bridge->encoder);
>>>> +	drm_panel_attach(ctx->panel, &ctx->connector);
>>>> +	ctx->connector.funcs->reset(&ctx->connector);
>>>> +	drm_fb_helper_add_one_connector(drm->fb_helper, &ctx->connector);
>>>> +	drm_connector_register(&ctx->connector);
>>> Aren't drm_connector_register()/unregister calls managed by the drm
>>> core?
>> drm core registers connectors during initialization but tc358764_attach
>> can be called after bridge probe, also after drm initialization, in such
>> case connector should be dynamically allocated/de-allocated.
> This really, really, really calls for *NOT* creating drm_connector instances 
> in bridge drivers. This has been an issue since the beginning and we need to 
> fix it. Connectors should be created by display drivers, and connector 
> operations implemented using operations provided by bridges.

I fully agree, and I have raised this issue already few times, but I
have not seen acceptance for this approach.

>
> Futhermore, registering and unregistering connectors after probe time isn't 
> supported in the DRM core, this will lead to nasty races with userspace. This 
> is also something that we should fix, but it will be much more difficult to 
> tackle.

I think it is not true, drm_connector can be created/destroyed
dynamically [1].

[1]:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?#connector-abstraction

Regards
Andrzej

>
>>> Otherwise:
>>> Reviewed-by: Archit Taneja <architt@codeaurora.org>
>> Thanks for the review, queued to drm-misc-next.
> With the above issues ? :-(
>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void tc358764_detach(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>> +	struct drm_device *drm = bridge->dev;
>>>> +
>>>> +	drm_connector_unregister(&ctx->connector);
>>>> +	drm_fb_helper_remove_one_connector(drm->fb_helper, &ctx->connector);
>>>> +	drm_panel_detach(ctx->panel);
>>>> +	ctx->panel = NULL;
>>>> +	drm_connector_unreference(&ctx->connector);
>>>> +}
>>>> +
>>>> +static const struct drm_bridge_funcs tc358764_bridge_funcs = {
>>>> +	.disable = tc358764_disable,
>>>> +	.post_disable = tc358764_post_disable,
>>>> +	.enable = tc358764_enable,
>>>> +	.pre_enable = tc358764_pre_enable,
>>>> +	.attach = tc358764_attach,
>>>> +	.detach = tc358764_detach,
>>>> +};
>>>> +
>>>> +static int tc358764_parse_dt(struct tc358764 *ctx)
>>>> +{
>>>> +	struct device *dev = ctx->dev;
>>>> +	int ret;
>>>> +
>>>> +	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>>>> +	if (IS_ERR(ctx->gpio_reset)) {
>>>> +		dev_err(dev, "no reset GPIO pin provided\n");
>>>> +		return PTR_ERR(ctx->gpio_reset);
>>>> +	}
>>>> +
>>>> +	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
>>>> +					  NULL);
>>>> +	if (ret && ret != -EPROBE_DEFER)
>>>> +		dev_err(dev, "cannot find panel (%d)\n", ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int tc358764_configure_regulators(struct tc358764 *ctx)
>>>> +{
>>>> +	int i, ret;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); ++i)
>>>> +		ctx->supplies[i].supply = tc358764_supplies[i];
>>>> +
>>>> +	ret = devm_regulator_bulk_get(ctx->dev, ARRAY_SIZE(ctx->supplies),
>>>> +				      ctx->supplies);
>>>> +	if (ret < 0)
>>>> +		dev_err(ctx->dev, "failed to get regulators: %d\n", ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int tc358764_probe(struct mipi_dsi_device *dsi)
>>>> +{
>>>> +	struct device *dev = &dsi->dev;
>>>> +	struct tc358764 *ctx;
>>>> +	int ret;
>>>> +
>>>> +	ctx = devm_kzalloc(dev, sizeof(struct tc358764), GFP_KERNEL);
>>>> +	if (!ctx)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	mipi_dsi_set_drvdata(dsi, ctx);
>>>> +
>>>> +	ctx->dev = dev;
>>>> +
>>>> +	dsi->lanes = 4;
>>>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>>>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
>>>> +		| MIPI_DSI_MODE_VIDEO_AUTO_VERT | MIPI_DSI_MODE_LPM;
>>>> +
>>>> +	ret = tc358764_parse_dt(ctx);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ret = tc358764_configure_regulators(ctx);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ctx->bridge.funcs = &tc358764_bridge_funcs;
>>>> +	ctx->bridge.of_node = dev->of_node;
>>>> +
>>>> +	drm_bridge_add(&ctx->bridge);
>>>> +
>>>> +	ret = mipi_dsi_attach(dsi);
>>>> +	if (ret < 0) {
>>>> +		drm_bridge_remove(&ctx->bridge);
>>>> +		dev_err(dev, "failed to attach dsi\n");
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int tc358764_remove(struct mipi_dsi_device *dsi)
>>>> +{
>>>> +	struct tc358764 *ctx = mipi_dsi_get_drvdata(dsi);
>>>> +
>>>> +	mipi_dsi_detach(dsi);
>>>> +	drm_bridge_remove(&ctx->bridge);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id tc358764_of_match[] = {
>>>> +	{ .compatible = "toshiba,tc358764" },
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, tc358764_of_match);
>>>> +
>>>> +static struct mipi_dsi_driver tc358764_driver = {
>>>> +	.probe = tc358764_probe,
>>>> +	.remove = tc358764_remove,
>>>> +	.driver = {
>>>> +		.name = "tc358764",
>>>> +		.owner = THIS_MODULE,
>>>> +		.of_match_table = tc358764_of_match,
>>>> +	},
>>>> +};
>>>> +module_mipi_dsi_driver(tc358764_driver);
>>>> +
>>>> +MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
>>>> +MODULE_AUTHOR("Maciej Purski <m.purski@samsung.com>");
>>>> +MODULE_DESCRIPTION("MIPI-DSI based Driver for TC358764 DSI/LVDS
>>>> Bridge");
>>>> +MODULE_LICENSE("GPL v2");


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Inki Dae]

2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
> From: Maciej Purski <m.purski@samsung.com>
> 
> The current implementation assumes that the only possible peripheral
> device for DSIM is a panel. Using an output bridge child device
> should also be possible.
> 
> If an output bridge is available, don't create a new connector.
> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
> in order to avoid an out bridge from being visible by the framework, as
> the DSI bus needs control on enabling its child output bridge.
> 
> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
> peripheral bridge device.
> 
> changed in v5:
> - detach bridge in mipi_dsi detach callback
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> [ a.hajda@samsung.com: v5 ]
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 351403f9d245..f5f51f584fa0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -255,6 +255,7 @@ struct exynos_dsi {
>  	struct mipi_dsi_host dsi_host;
>  	struct drm_connector connector;
>  	struct drm_panel *panel;
> +	struct drm_bridge *out_bridge;
>  	struct device *dev;
>  
>  	void __iomem *reg_base;
> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  				  struct mipi_dsi_device *device)
>  {
>  	struct exynos_dsi *dsi = host_to_dsi(host);
> -	struct drm_device *drm = dsi->connector.dev;
> +	struct drm_encoder *encoder = &dsi->encoder;
> +	struct drm_device *drm = encoder->dev;
> +	struct drm_bridge *out_bridge;
> +
> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);

Is there any reason to find out_bridge without considering device tree graph binding?

Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this,
Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt

[1] https://lkml.org/lkml/2018/6/19/237

Thanks,
Inki Dae

> +	if (out_bridge) {
> +		drm_bridge_attach(encoder, out_bridge, NULL);
> +		dsi->out_bridge = out_bridge;
> +		encoder->bridge = NULL;
> +	} else {
> +		int ret = exynos_dsi_create_connector(encoder);
> +
> +		if (ret) {
> +			DRM_ERROR("failed to create connector ret = %d\n", ret);
> +			drm_encoder_cleanup(encoder);
> +			return ret;
> +		}
> +
> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
> +		if (dsi->panel) {
> +			drm_panel_attach(dsi->panel, &dsi->connector);
> +			dsi->connector.status = connector_status_connected;
> +		}
> +	}
>  
>  	/*
>  	 * This is a temporary solution and should be made by more generic way.
> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  	dsi->lanes = device->lanes;
>  	dsi->format = device->format;
>  	dsi->mode_flags = device->mode_flags;
> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
> -	if (dsi->panel) {
> -		drm_panel_attach(dsi->panel, &dsi->connector);
> -		dsi->connector.status = connector_status_connected;
> -	}
>  	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
>  			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
>  
> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>  				  struct mipi_dsi_device *device)
>  {
>  	struct exynos_dsi *dsi = host_to_dsi(host);
> -	struct drm_device *drm = dsi->connector.dev;
> -
> -	mutex_lock(&drm->mode_config.mutex);
> +	struct drm_device *drm = dsi->encoder.dev;
>  
>  	if (dsi->panel) {
> +		mutex_lock(&drm->mode_config.mutex);
>  		exynos_dsi_disable(&dsi->encoder);
>  		drm_panel_detach(dsi->panel);
>  		dsi->panel = NULL;
>  		dsi->connector.status = connector_status_disconnected;
> +		mutex_unlock(&drm->mode_config.mutex);
> +	} else {
> +		if (dsi->out_bridge->funcs->detach)
> +			dsi->out_bridge->funcs->detach(dsi->out_bridge);
> +		dsi->out_bridge = NULL;
>  	}
>  
> -	mutex_unlock(&drm->mode_config.mutex);
> -
>  	if (drm->mode_config.poll_enabled)
>  		drm_kms_helper_hotplug_event(drm);
>  
> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = exynos_dsi_create_connector(encoder);
> -	if (ret) {
> -		DRM_ERROR("failed to create connector ret = %d\n", ret);
> -		drm_encoder_cleanup(encoder);
> -		return ret;
> -	}
> -
>  	if (dsi->in_bridge_node) {
>  		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
>  		if (in_bridge)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Andrzej Hajda]

On 07.08.2018 10:53, Inki Dae wrote:
>
> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>> From: Maciej Purski <m.purski@samsung.com>
>>
>> The current implementation assumes that the only possible peripheral
>> device for DSIM is a panel. Using an output bridge child device
>> should also be possible.
>>
>> If an output bridge is available, don't create a new connector.
>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>> in order to avoid an out bridge from being visible by the framework, as
>> the DSI bus needs control on enabling its child output bridge.
>>
>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>> peripheral bridge device.
>>
>> changed in v5:
>> - detach bridge in mipi_dsi detach callback
>>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> [ a.hajda@samsung.com: v5 ]
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> index 351403f9d245..f5f51f584fa0 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>  	struct mipi_dsi_host dsi_host;
>>  	struct drm_connector connector;
>>  	struct drm_panel *panel;
>> +	struct drm_bridge *out_bridge;
>>  	struct device *dev;
>>  
>>  	void __iomem *reg_base;
>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>  				  struct mipi_dsi_device *device)
>>  {
>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>> -	struct drm_device *drm = dsi->connector.dev;
>> +	struct drm_encoder *encoder = &dsi->encoder;
>> +	struct drm_device *drm = encoder->dev;
>> +	struct drm_bridge *out_bridge;
>> +
>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
> Is there any reason to find out_bridge without considering device tree graph binding?

If the sink is a child MIPI-DSI device, the bindings can be omitted, as
in case of all DSI panels in Exynos platforms.
In case bindings are not present you cannot use graph functions.

>
> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this,
> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt

But this document says about child nodes, and Toshiba bridge is MIPI-DSI
child [1].

Regards
Andrzej

>
> [1] https://lkml.org/lkml/2018/6/19/237
>
> Thanks,
> Inki Dae
>
>> +	if (out_bridge) {
>> +		drm_bridge_attach(encoder, out_bridge, NULL);
>> +		dsi->out_bridge = out_bridge;
>> +		encoder->bridge = NULL;
>> +	} else {
>> +		int ret = exynos_dsi_create_connector(encoder);
>> +
>> +		if (ret) {
>> +			DRM_ERROR("failed to create connector ret = %d\n", ret);
>> +			drm_encoder_cleanup(encoder);
>> +			return ret;
>> +		}
>> +
>> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
>> +		if (dsi->panel) {
>> +			drm_panel_attach(dsi->panel, &dsi->connector);
>> +			dsi->connector.status = connector_status_connected;
>> +		}
>> +	}
>>  
>>  	/*
>>  	 * This is a temporary solution and should be made by more generic way.
>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>  	dsi->lanes = device->lanes;
>>  	dsi->format = device->format;
>>  	dsi->mode_flags = device->mode_flags;
>> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
>> -	if (dsi->panel) {
>> -		drm_panel_attach(dsi->panel, &dsi->connector);
>> -		dsi->connector.status = connector_status_connected;
>> -	}
>>  	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
>>  			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
>>  
>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>  				  struct mipi_dsi_device *device)
>>  {
>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>> -	struct drm_device *drm = dsi->connector.dev;
>> -
>> -	mutex_lock(&drm->mode_config.mutex);
>> +	struct drm_device *drm = dsi->encoder.dev;
>>  
>>  	if (dsi->panel) {
>> +		mutex_lock(&drm->mode_config.mutex);
>>  		exynos_dsi_disable(&dsi->encoder);
>>  		drm_panel_detach(dsi->panel);
>>  		dsi->panel = NULL;
>>  		dsi->connector.status = connector_status_disconnected;
>> +		mutex_unlock(&drm->mode_config.mutex);
>> +	} else {
>> +		if (dsi->out_bridge->funcs->detach)
>> +			dsi->out_bridge->funcs->detach(dsi->out_bridge);
>> +		dsi->out_bridge = NULL;
>>  	}
>>  
>> -	mutex_unlock(&drm->mode_config.mutex);
>> -
>>  	if (drm->mode_config.poll_enabled)
>>  		drm_kms_helper_hotplug_event(drm);
>>  
>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	ret = exynos_dsi_create_connector(encoder);
>> -	if (ret) {
>> -		DRM_ERROR("failed to create connector ret = %d\n", ret);
>> -		drm_encoder_cleanup(encoder);
>> -		return ret;
>> -	}
>> -
>>  	if (dsi->in_bridge_node) {
>>  		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
>>  		if (in_bridge)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Inki Dae]

2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글:
> On 07.08.2018 10:53, Inki Dae wrote:
>>
>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>>> From: Maciej Purski <m.purski@samsung.com>
>>>
>>> The current implementation assumes that the only possible peripheral
>>> device for DSIM is a panel. Using an output bridge child device
>>> should also be possible.
>>>
>>> If an output bridge is available, don't create a new connector.
>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>>> in order to avoid an out bridge from being visible by the framework, as
>>> the DSI bus needs control on enabling its child output bridge.
>>>
>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>>> peripheral bridge device.
>>>
>>> changed in v5:
>>> - detach bridge in mipi_dsi detach callback
>>>
>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>> [ a.hajda@samsung.com: v5 ]
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 351403f9d245..f5f51f584fa0 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>>  	struct mipi_dsi_host dsi_host;
>>>  	struct drm_connector connector;
>>>  	struct drm_panel *panel;
>>> +	struct drm_bridge *out_bridge;
>>>  	struct device *dev;
>>>  
>>>  	void __iomem *reg_base;
>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>  				  struct mipi_dsi_device *device)
>>>  {
>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>> -	struct drm_device *drm = dsi->connector.dev;
>>> +	struct drm_encoder *encoder = &dsi->encoder;
>>> +	struct drm_device *drm = encoder->dev;
>>> +	struct drm_bridge *out_bridge;
>>> +
>>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>> Is there any reason to find out_bridge without considering device tree graph binding?
> 
> If the sink is a child MIPI-DSI device, the bindings can be omitted, as
> in case of all DSI panels in Exynos platforms.
> In case bindings are not present you cannot use graph functions.

In other words, this means that this patch doesn't allow to use the device tree graph binding.
I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device.

So I think it would be better to allow to use both of them, as a child device and graph binding.

How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way?

And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding.
Is there any document I can refer to?

Thanks,
Inki Dae

> 
>>
>> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this,
>> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt
> 
> But this document says about child nodes, and Toshiba bridge is MIPI-DSI
> child [1].
> 
> Regards
> Andrzej
> 
>>
>> [1] https://lkml.org/lkml/2018/6/19/237
>>
>> Thanks,
>> Inki Dae
>>
>>> +	if (out_bridge) {
>>> +		drm_bridge_attach(encoder, out_bridge, NULL);
>>> +		dsi->out_bridge = out_bridge;
>>> +		encoder->bridge = NULL;
>>> +	} else {
>>> +		int ret = exynos_dsi_create_connector(encoder);
>>> +
>>> +		if (ret) {
>>> +			DRM_ERROR("failed to create connector ret = %d\n", ret);
>>> +			drm_encoder_cleanup(encoder);
>>> +			return ret;
>>> +		}
>>> +
>>> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
>>> +		if (dsi->panel) {
>>> +			drm_panel_attach(dsi->panel, &dsi->connector);
>>> +			dsi->connector.status = connector_status_connected;
>>> +		}
>>> +	}
>>>  
>>>  	/*
>>>  	 * This is a temporary solution and should be made by more generic way.
>>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>  	dsi->lanes = device->lanes;
>>>  	dsi->format = device->format;
>>>  	dsi->mode_flags = device->mode_flags;
>>> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
>>> -	if (dsi->panel) {
>>> -		drm_panel_attach(dsi->panel, &dsi->connector);
>>> -		dsi->connector.status = connector_status_connected;
>>> -	}
>>>  	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
>>>  			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
>>>  
>>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>  				  struct mipi_dsi_device *device)
>>>  {
>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>> -	struct drm_device *drm = dsi->connector.dev;
>>> -
>>> -	mutex_lock(&drm->mode_config.mutex);
>>> +	struct drm_device *drm = dsi->encoder.dev;
>>>  
>>>  	if (dsi->panel) {
>>> +		mutex_lock(&drm->mode_config.mutex);
>>>  		exynos_dsi_disable(&dsi->encoder);
>>>  		drm_panel_detach(dsi->panel);
>>>  		dsi->panel = NULL;
>>>  		dsi->connector.status = connector_status_disconnected;
>>> +		mutex_unlock(&drm->mode_config.mutex);
>>> +	} else {
>>> +		if (dsi->out_bridge->funcs->detach)
>>> +			dsi->out_bridge->funcs->detach(dsi->out_bridge);
>>> +		dsi->out_bridge = NULL;
>>>  	}
>>>  
>>> -	mutex_unlock(&drm->mode_config.mutex);
>>> -
>>>  	if (drm->mode_config.poll_enabled)
>>>  		drm_kms_helper_hotplug_event(drm);
>>>  
>>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> -	ret = exynos_dsi_create_connector(encoder);
>>> -	if (ret) {
>>> -		DRM_ERROR("failed to create connector ret = %d\n", ret);
>>> -		drm_encoder_cleanup(encoder);
>>> -		return ret;
>>> -	}
>>> -
>>>  	if (dsi->in_bridge_node) {
>>>  		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
>>>  		if (in_bridge)
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Andrzej Hajda]

On 17.08.2018 03:56, Inki Dae wrote:
>
> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글:
>> On 07.08.2018 10:53, Inki Dae wrote:
>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>>>> From: Maciej Purski <m.purski@samsung.com>
>>>>
>>>> The current implementation assumes that the only possible peripheral
>>>> device for DSIM is a panel. Using an output bridge child device
>>>> should also be possible.
>>>>
>>>> If an output bridge is available, don't create a new connector.
>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>>>> in order to avoid an out bridge from being visible by the framework, as
>>>> the DSI bus needs control on enabling its child output bridge.
>>>>
>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>>>> peripheral bridge device.
>>>>
>>>> changed in v5:
>>>> - detach bridge in mipi_dsi detach callback
>>>>
>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>> [ a.hajda@samsung.com: v5 ]
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> index 351403f9d245..f5f51f584fa0 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>>>  	struct mipi_dsi_host dsi_host;
>>>>  	struct drm_connector connector;
>>>>  	struct drm_panel *panel;
>>>> +	struct drm_bridge *out_bridge;
>>>>  	struct device *dev;
>>>>  
>>>>  	void __iomem *reg_base;
>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>  				  struct mipi_dsi_device *device)
>>>>  {
>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>> +	struct drm_encoder *encoder = &dsi->encoder;
>>>> +	struct drm_device *drm = encoder->dev;
>>>> +	struct drm_bridge *out_bridge;
>>>> +
>>>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>>> Is there any reason to find out_bridge without considering device tree graph binding?
>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as
>> in case of all DSI panels in Exynos platforms.
>> In case bindings are not present you cannot use graph functions.
> In other words, this means that this patch doesn't allow to use the device tree graph binding.
> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device.
>
> So I think it would be better to allow to use both of them, as a child device and graph binding.
>
> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way?

It could be done this way, but it should be done for panels and for
bridges, and it should be rather generic helper, not Exynos specific. So
it is something which require additional investigation, discussion and
separate patchset.
On the other side this patch is enough to correctly handle all DSI
bridges in existing Exynos boards.

>
> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding.
> Is there any document I can refer to?

DSI devices (peripherals) should be described as child nodes of DSI host
node in DT bindings, it is described in [1]. And you can find all dsi
panels in exynos based boards are modeled this way.
And the graph documentation [2] says that graphs should be used for
connections that "can not be inferred from device tree parent-child
relationships", it was emphasised multiple times by Rob in discussions
regarding bindings of DSI devices.

[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
[2]: Documentation/devicetree/bindings/graph.txt

Regards
Andrzej

>
> Thanks,
> Inki Dae
>
>>> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this,
>>> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt
>> But this document says about child nodes, and Toshiba bridge is MIPI-DSI
>> child [1].
>>
>> Regards
>> Andrzej
>>
>>> [1] https://lkml.org/lkml/2018/6/19/237
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> +	if (out_bridge) {
>>>> +		drm_bridge_attach(encoder, out_bridge, NULL);
>>>> +		dsi->out_bridge = out_bridge;
>>>> +		encoder->bridge = NULL;
>>>> +	} else {
>>>> +		int ret = exynos_dsi_create_connector(encoder);
>>>> +
>>>> +		if (ret) {
>>>> +			DRM_ERROR("failed to create connector ret = %d\n", ret);
>>>> +			drm_encoder_cleanup(encoder);
>>>> +			return ret;
>>>> +		}
>>>> +
>>>> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
>>>> +		if (dsi->panel) {
>>>> +			drm_panel_attach(dsi->panel, &dsi->connector);
>>>> +			dsi->connector.status = connector_status_connected;
>>>> +		}
>>>> +	}
>>>>  
>>>>  	/*
>>>>  	 * This is a temporary solution and should be made by more generic way.
>>>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>  	dsi->lanes = device->lanes;
>>>>  	dsi->format = device->format;
>>>>  	dsi->mode_flags = device->mode_flags;
>>>> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
>>>> -	if (dsi->panel) {
>>>> -		drm_panel_attach(dsi->panel, &dsi->connector);
>>>> -		dsi->connector.status = connector_status_connected;
>>>> -	}
>>>>  	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
>>>>  			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
>>>>  
>>>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>>  				  struct mipi_dsi_device *device)
>>>>  {
>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>> -
>>>> -	mutex_lock(&drm->mode_config.mutex);
>>>> +	struct drm_device *drm = dsi->encoder.dev;
>>>>  
>>>>  	if (dsi->panel) {
>>>> +		mutex_lock(&drm->mode_config.mutex);
>>>>  		exynos_dsi_disable(&dsi->encoder);
>>>>  		drm_panel_detach(dsi->panel);
>>>>  		dsi->panel = NULL;
>>>>  		dsi->connector.status = connector_status_disconnected;
>>>> +		mutex_unlock(&drm->mode_config.mutex);
>>>> +	} else {
>>>> +		if (dsi->out_bridge->funcs->detach)
>>>> +			dsi->out_bridge->funcs->detach(dsi->out_bridge);
>>>> +		dsi->out_bridge = NULL;
>>>>  	}
>>>>  
>>>> -	mutex_unlock(&drm->mode_config.mutex);
>>>> -
>>>>  	if (drm->mode_config.poll_enabled)
>>>>  		drm_kms_helper_hotplug_event(drm);
>>>>  
>>>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> -	ret = exynos_dsi_create_connector(encoder);
>>>> -	if (ret) {
>>>> -		DRM_ERROR("failed to create connector ret = %d\n", ret);
>>>> -		drm_encoder_cleanup(encoder);
>>>> -		return ret;
>>>> -	}
>>>> -
>>>>  	if (dsi->in_bridge_node) {
>>>>  		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
>>>>  		if (in_bridge)
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Inki Dae]

2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글:
> On 17.08.2018 03:56, Inki Dae wrote:
>>
>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글:
>>> On 07.08.2018 10:53, Inki Dae wrote:
>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>>>>> From: Maciej Purski <m.purski@samsung.com>
>>>>>
>>>>> The current implementation assumes that the only possible peripheral
>>>>> device for DSIM is a panel. Using an output bridge child device
>>>>> should also be possible.
>>>>>
>>>>> If an output bridge is available, don't create a new connector.
>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>>>>> in order to avoid an out bridge from being visible by the framework, as
>>>>> the DSI bus needs control on enabling its child output bridge.
>>>>>
>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>>>>> peripheral bridge device.
>>>>>
>>>>> changed in v5:
>>>>> - detach bridge in mipi_dsi detach callback
>>>>>
>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>>> [ a.hajda@samsung.com: v5 ]
>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>> ---
>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>>>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>> index 351403f9d245..f5f51f584fa0 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>>>>  	struct mipi_dsi_host dsi_host;
>>>>>  	struct drm_connector connector;
>>>>>  	struct drm_panel *panel;
>>>>> +	struct drm_bridge *out_bridge;
>>>>>  	struct device *dev;
>>>>>  
>>>>>  	void __iomem *reg_base;
>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>>  				  struct mipi_dsi_device *device)
>>>>>  {
>>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>>> +	struct drm_encoder *encoder = &dsi->encoder;
>>>>> +	struct drm_device *drm = encoder->dev;
>>>>> +	struct drm_bridge *out_bridge;
>>>>> +
>>>>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>>>> Is there any reason to find out_bridge without considering device tree graph binding?
>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as
>>> in case of all DSI panels in Exynos platforms.
>>> In case bindings are not present you cannot use graph functions.
>> In other words, this means that this patch doesn't allow to use the device tree graph binding.
>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device.
>>
>> So I think it would be better to allow to use both of them, as a child device and graph binding.
>>
>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way?
> 
> It could be done this way, but it should be done for panels and for
> bridges, and it should be rather generic helper, not Exynos specific. So

As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments.


> it is something which require additional investigation, discussion and
> separate patchset.

So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way.
Of course, as a separate patch, we could consider using this function for panel device later.

> On the other side this patch is enough to correctly handle all DSI
> bridges in existing Exynos boards.
> 
>>
>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding.
>> Is there any document I can refer to?
> 
> DSI devices (peripherals) should be described as child nodes of DSI host
> node in DT bindings, it is described in [1]. And you can find all dsi
> panels in exynos based boards are modeled this way.
> And the graph documentation [2] says that graphs should be used for
> connections that "can not be inferred from device tree parent-child
> relationships", it was emphasised multiple times by Rob in discussions
> regarding bindings of DSI devices.

Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes.
And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion.

[1]
commit 1f2db3034c9f16ed918e34809167546f21d0fcb4
Author: Rob Herring <robh@kernel.org>
Date:   Wed Mar 22 08:26:05 2017 -0500

    drm: of: introduce drm_of_find_panel_or_bridge
    
    Many drivers have a common pattern of searching the OF graph for either an
    attached panel or bridge and then finding the DRM struct for the panel
    or bridge. Also, most drivers need to handle deferred probing when the
    DRM device is not yet instantiated. Create a common function,
    drm_of_find_panel_or_bridge, to find the connected node and the
    associated DRM panel or bridge device.
    
    Signed-off-by: Rob Herring <robh@kernel.org>
    Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
    [seanpaul dropped extern from drm_of.h]
    Signed-off-by: Sean Paul <seanpaul@chromium.org>


Thanks,
Inki Dae

> 
> [1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> [2]: Documentation/devicetree/bindings/graph.txt
> 
> Regards
> Andrzej
> 
>>
>> Thanks,
>> Inki Dae
>>
>>>> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this,
>>>> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt
>>> But this document says about child nodes, and Toshiba bridge is MIPI-DSI
>>> child [1].
>>>
>>> Regards
>>> Andrzej
>>>
>>>> [1] https://lkml.org/lkml/2018/6/19/237
>>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> +	if (out_bridge) {
>>>>> +		drm_bridge_attach(encoder, out_bridge, NULL);
>>>>> +		dsi->out_bridge = out_bridge;
>>>>> +		encoder->bridge = NULL;
>>>>> +	} else {
>>>>> +		int ret = exynos_dsi_create_connector(encoder);
>>>>> +
>>>>> +		if (ret) {
>>>>> +			DRM_ERROR("failed to create connector ret = %d\n", ret);
>>>>> +			drm_encoder_cleanup(encoder);
>>>>> +			return ret;
>>>>> +		}
>>>>> +
>>>>> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
>>>>> +		if (dsi->panel) {
>>>>> +			drm_panel_attach(dsi->panel, &dsi->connector);
>>>>> +			dsi->connector.status = connector_status_connected;
>>>>> +		}
>>>>> +	}
>>>>>  
>>>>>  	/*
>>>>>  	 * This is a temporary solution and should be made by more generic way.
>>>>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>>  	dsi->lanes = device->lanes;
>>>>>  	dsi->format = device->format;
>>>>>  	dsi->mode_flags = device->mode_flags;
>>>>> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
>>>>> -	if (dsi->panel) {
>>>>> -		drm_panel_attach(dsi->panel, &dsi->connector);
>>>>> -		dsi->connector.status = connector_status_connected;
>>>>> -	}
>>>>>  	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
>>>>>  			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
>>>>>  
>>>>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>>>  				  struct mipi_dsi_device *device)
>>>>>  {
>>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>>> -
>>>>> -	mutex_lock(&drm->mode_config.mutex);
>>>>> +	struct drm_device *drm = dsi->encoder.dev;
>>>>>  
>>>>>  	if (dsi->panel) {
>>>>> +		mutex_lock(&drm->mode_config.mutex);
>>>>>  		exynos_dsi_disable(&dsi->encoder);
>>>>>  		drm_panel_detach(dsi->panel);
>>>>>  		dsi->panel = NULL;
>>>>>  		dsi->connector.status = connector_status_disconnected;
>>>>> +		mutex_unlock(&drm->mode_config.mutex);
>>>>> +	} else {
>>>>> +		if (dsi->out_bridge->funcs->detach)
>>>>> +			dsi->out_bridge->funcs->detach(dsi->out_bridge);
>>>>> +		dsi->out_bridge = NULL;
>>>>>  	}
>>>>>  
>>>>> -	mutex_unlock(&drm->mode_config.mutex);
>>>>> -
>>>>>  	if (drm->mode_config.poll_enabled)
>>>>>  		drm_kms_helper_hotplug_event(drm);
>>>>>  
>>>>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>>>>>  	if (ret < 0)
>>>>>  		return ret;
>>>>>  
>>>>> -	ret = exynos_dsi_create_connector(encoder);
>>>>> -	if (ret) {
>>>>> -		DRM_ERROR("failed to create connector ret = %d\n", ret);
>>>>> -		drm_encoder_cleanup(encoder);
>>>>> -		return ret;
>>>>> -	}
>>>>> -
>>>>>  	if (dsi->in_bridge_node) {
>>>>>  		in_bridge = of_drm_find_bridge(dsi->in_bridge_node);
>>>>>  		if (in_bridge)
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>
>>>
>>
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Andrzej Hajda]

On 21.08.2018 07:27, Inki Dae wrote:
>
> 2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글:
>> On 17.08.2018 03:56, Inki Dae wrote:
>>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글:
>>>> On 07.08.2018 10:53, Inki Dae wrote:
>>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>>>>>> From: Maciej Purski <m.purski@samsung.com>
>>>>>>
>>>>>> The current implementation assumes that the only possible peripheral
>>>>>> device for DSIM is a panel. Using an output bridge child device
>>>>>> should also be possible.
>>>>>>
>>>>>> If an output bridge is available, don't create a new connector.
>>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>>>>>> in order to avoid an out bridge from being visible by the framework, as
>>>>>> the DSI bus needs control on enabling its child output bridge.
>>>>>>
>>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>>>>>> peripheral bridge device.
>>>>>>
>>>>>> changed in v5:
>>>>>> - detach bridge in mipi_dsi detach callback
>>>>>>
>>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>>>> [ a.hajda@samsung.com: v5 ]
>>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>>>>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> index 351403f9d245..f5f51f584fa0 100644
>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>>>>>  	struct mipi_dsi_host dsi_host;
>>>>>>  	struct drm_connector connector;
>>>>>>  	struct drm_panel *panel;
>>>>>> +	struct drm_bridge *out_bridge;
>>>>>>  	struct device *dev;
>>>>>>  
>>>>>>  	void __iomem *reg_base;
>>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>>>  				  struct mipi_dsi_device *device)
>>>>>>  {
>>>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>>>> +	struct drm_encoder *encoder = &dsi->encoder;
>>>>>> +	struct drm_device *drm = encoder->dev;
>>>>>> +	struct drm_bridge *out_bridge;
>>>>>> +
>>>>>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>>>>> Is there any reason to find out_bridge without considering device tree graph binding?
>>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as
>>>> in case of all DSI panels in Exynos platforms.
>>>> In case bindings are not present you cannot use graph functions.
>>> In other words, this means that this patch doesn't allow to use the device tree graph binding.
>>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device.
>>>
>>> So I think it would be better to allow to use both of them, as a child device and graph binding.
>>>
>>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way?
>> It could be done this way, but it should be done for panels and for
>> bridges, and it should be rather generic helper, not Exynos specific. So
> As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments.
>
>
>> it is something which require additional investigation, discussion and
>> separate patchset.
> So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way.

But drm_of_find_panel_or_bridge is only for looking for non-dsi devices,
or more specifically for looking for devices connected in DTS via
DT-graph. This is not case of panels and bridges used in Exynos boards.
So this function is currently useless with our boards. Maybe some day we
will have bridge/panel controlled via I2C and then it will become
useful, but for now it serves for nothing.

> Of course, as a separate patch, we could consider using this function for panel device later.

As you said drm_of_find_panel_or_bridge looks for panel and/or bridge,
and it was created to merge similar code in panel and bridge lookup,
using it only for bridges and not for panels seems against it's purpose.

>
>> On the other side this patch is enough to correctly handle all DSI
>> bridges in existing Exynos boards.
>>
>>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding.
>>> Is there any document I can refer to?
>> DSI devices (peripherals) should be described as child nodes of DSI host
>> node in DT bindings, it is described in [1]. And you can find all dsi
>> panels in exynos based boards are modeled this way.
>> And the graph documentation [2] says that graphs should be used for
>> connections that "can not be inferred from device tree parent-child
>> relationships", it was emphasised multiple times by Rob in discussions
>> regarding bindings of DSI devices.
> Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes.
> And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion.

Because this function was created for i2c/spi controlled bridges/panels
and probably these boards use only such devices.

Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Inki Dae]

2018년 08월 21일 20:21에 Andrzej Hajda 이(가) 쓴 글:
> On 21.08.2018 07:27, Inki Dae wrote:
>>
>> 2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글:
>>> On 17.08.2018 03:56, Inki Dae wrote:
>>>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글:
>>>>> On 07.08.2018 10:53, Inki Dae wrote:
>>>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>>>>>>> From: Maciej Purski <m.purski@samsung.com>
>>>>>>>
>>>>>>> The current implementation assumes that the only possible peripheral
>>>>>>> device for DSIM is a panel. Using an output bridge child device
>>>>>>> should also be possible.
>>>>>>>
>>>>>>> If an output bridge is available, don't create a new connector.
>>>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>>>>>>> in order to avoid an out bridge from being visible by the framework, as
>>>>>>> the DSI bus needs control on enabling its child output bridge.
>>>>>>>
>>>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>>>>>>> peripheral bridge device.
>>>>>>>
>>>>>>> changed in v5:
>>>>>>> - detach bridge in mipi_dsi detach callback
>>>>>>>
>>>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>>>>> [ a.hajda@samsung.com: v5 ]
>>>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>>>>>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>>> index 351403f9d245..f5f51f584fa0 100644
>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>>>>>>  	struct mipi_dsi_host dsi_host;
>>>>>>>  	struct drm_connector connector;
>>>>>>>  	struct drm_panel *panel;
>>>>>>> +	struct drm_bridge *out_bridge;
>>>>>>>  	struct device *dev;
>>>>>>>  
>>>>>>>  	void __iomem *reg_base;
>>>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>>>>  				  struct mipi_dsi_device *device)
>>>>>>>  {
>>>>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>>>>> +	struct drm_encoder *encoder = &dsi->encoder;
>>>>>>> +	struct drm_device *drm = encoder->dev;
>>>>>>> +	struct drm_bridge *out_bridge;
>>>>>>> +
>>>>>>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>>>>>> Is there any reason to find out_bridge without considering device tree graph binding?
>>>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as
>>>>> in case of all DSI panels in Exynos platforms.
>>>>> In case bindings are not present you cannot use graph functions.
>>>> In other words, this means that this patch doesn't allow to use the device tree graph binding.
>>>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device.
>>>>
>>>> So I think it would be better to allow to use both of them, as a child device and graph binding.
>>>>
>>>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way?
>>> It could be done this way, but it should be done for panels and for
>>> bridges, and it should be rather generic helper, not Exynos specific. So
>> As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments.
>>
>>
>>> it is something which require additional investigation, discussion and
>>> separate patchset.
>> So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way.
> 
> But drm_of_find_panel_or_bridge is only for looking for non-dsi devices,
> or more specifically for looking for devices connected in DTS via
> DT-graph. This is not case of panels and bridges used in Exynos boards.
> So this function is currently useless with our boards. Maybe some day we
> will have bridge/panel controlled via I2C and then it will become
> useful, but for now it serves for nothing.
> 
>> Of course, as a separate patch, we could consider using this function for panel device later.
> 
> As you said drm_of_find_panel_or_bridge looks for panel and/or bridge,
> and it was created to merge similar code in panel and bridge lookup,
> using it only for bridges and not for panels seems against it's purpose.
> 
>>
>>> On the other side this patch is enough to correctly handle all DSI
>>> bridges in existing Exynos boards.
>>>
>>>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding.
>>>> Is there any document I can refer to?
>>> DSI devices (peripherals) should be described as child nodes of DSI host
>>> node in DT bindings, it is described in [1]. And you can find all dsi
>>> panels in exynos based boards are modeled this way.
>>> And the graph documentation [2] says that graphs should be used for
>>> connections that "can not be inferred from device tree parent-child
>>> relationships", it was emphasised multiple times by Rob in discussions
>>> regarding bindings of DSI devices.
>> Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes.
>> And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion.
> 
> Because this function was created for i2c/spi controlled bridges/panels
> and probably these boards use only such devices.

Clear. I will pick them up.

BTW, the bridge and panelI mentioned above are child devices of DSI driver. So I'm not sure but the child devices would be controlled by MIPI DSI protocol not by i2c/spi. 
If you are right then the devices - bridge or panel - are controlled by i2c or spi and only image data are transferred from DSI to bridge or panel by MIPI lanes?

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/9] drm/exynos: move connector creation to attach callback

[Andrzej Hajda]

On 22.08.2018 04:59, Inki Dae wrote:
>
> 2018년 08월 21일 20:21에 Andrzej Hajda 이(가) 쓴 글:
>> On 21.08.2018 07:27, Inki Dae wrote:
>>> 2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글:
>>>> On 17.08.2018 03:56, Inki Dae wrote:
>>>>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글:
>>>>>> On 07.08.2018 10:53, Inki Dae wrote:
>>>>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글:
>>>>>>>> From: Maciej Purski <m.purski@samsung.com>
>>>>>>>>
>>>>>>>> The current implementation assumes that the only possible peripheral
>>>>>>>> device for DSIM is a panel. Using an output bridge child device
>>>>>>>> should also be possible.
>>>>>>>>
>>>>>>>> If an output bridge is available, don't create a new connector.
>>>>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL
>>>>>>>> in order to avoid an out bridge from being visible by the framework, as
>>>>>>>> the DSI bus needs control on enabling its child output bridge.
>>>>>>>>
>>>>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI
>>>>>>>> peripheral bridge device.
>>>>>>>>
>>>>>>>> changed in v5:
>>>>>>>> - detach bridge in mipi_dsi detach callback
>>>>>>>>
>>>>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>>>>>> [ a.hajda@samsung.com: v5 ]
>>>>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++---------
>>>>>>>>  1 file changed, 32 insertions(+), 18 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>>>> index 351403f9d245..f5f51f584fa0 100644
>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi {
>>>>>>>>  	struct mipi_dsi_host dsi_host;
>>>>>>>>  	struct drm_connector connector;
>>>>>>>>  	struct drm_panel *panel;
>>>>>>>> +	struct drm_bridge *out_bridge;
>>>>>>>>  	struct device *dev;
>>>>>>>>  
>>>>>>>>  	void __iomem *reg_base;
>>>>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>>>>>  				  struct mipi_dsi_device *device)
>>>>>>>>  {
>>>>>>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>>>>> -	struct drm_device *drm = dsi->connector.dev;
>>>>>>>> +	struct drm_encoder *encoder = &dsi->encoder;
>>>>>>>> +	struct drm_device *drm = encoder->dev;
>>>>>>>> +	struct drm_bridge *out_bridge;
>>>>>>>> +
>>>>>>>> +	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>>>>>>> Is there any reason to find out_bridge without considering device tree graph binding?
>>>>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as
>>>>>> in case of all DSI panels in Exynos platforms.
>>>>>> In case bindings are not present you cannot use graph functions.
>>>>> In other words, this means that this patch doesn't allow to use the device tree graph binding.
>>>>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device.
>>>>>
>>>>> So I think it would be better to allow to use both of them, as a child device and graph binding.
>>>>>
>>>>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way?
>>>> It could be done this way, but it should be done for panels and for
>>>> bridges, and it should be rather generic helper, not Exynos specific. So
>>> As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments.
>>>
>>>
>>>> it is something which require additional investigation, discussion and
>>>> separate patchset.
>>> So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way.
>> But drm_of_find_panel_or_bridge is only for looking for non-dsi devices,
>> or more specifically for looking for devices connected in DTS via
>> DT-graph. This is not case of panels and bridges used in Exynos boards.
>> So this function is currently useless with our boards. Maybe some day we
>> will have bridge/panel controlled via I2C and then it will become
>> useful, but for now it serves for nothing.
>>
>>> Of course, as a separate patch, we could consider using this function for panel device later.
>> As you said drm_of_find_panel_or_bridge looks for panel and/or bridge,
>> and it was created to merge similar code in panel and bridge lookup,
>> using it only for bridges and not for panels seems against it's purpose.
>>
>>>> On the other side this patch is enough to correctly handle all DSI
>>>> bridges in existing Exynos boards.
>>>>
>>>>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding.
>>>>> Is there any document I can refer to?
>>>> DSI devices (peripherals) should be described as child nodes of DSI host
>>>> node in DT bindings, it is described in [1]. And you can find all dsi
>>>> panels in exynos based boards are modeled this way.
>>>> And the graph documentation [2] says that graphs should be used for
>>>> connections that "can not be inferred from device tree parent-child
>>>> relationships", it was emphasised multiple times by Rob in discussions
>>>> regarding bindings of DSI devices.
>>> Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes.
>>> And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion.
>> Because this function was created for i2c/spi controlled bridges/panels
>> and probably these boards use only such devices.
> Clear. I will pick them up.
>
> BTW, the bridge and panelI mentioned above are child devices of DSI driver. So I'm not sure but the child devices would be controlled by MIPI DSI protocol not by i2c/spi. 

Currently the only pure DSI bridge is tc358764, adv7511 registers as
both i2c and dsi bridge driver, but in dts files it is always as i2c
device, so it is never a child of DSI host.
In case of DSI panels I have found 2 which uses obsolete bindings
(orisetech,otm8009a, jdi,lt070me05000) - ie they are child nodes of dsi
host and have graph links- in their case drm_of_find_panel_or_bridge
will work.
The rest is either not present in dts, either follows current bindings
guidelines (all samsung panels) - no graph bindings if it is a child of
dsi host.
I gave up with checking panel-simple, as it has too many compatibles.

> If you are right then the devices - bridge or panel - are controlled by i2c or spi and only image data are transferred from DSI to bridge or panel by MIPI lanes?

Yes, or the control is mixed: power-up initialization/link-setup only
via i2c, other commands and data via dsi.

Regards
Andrzej

>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/9] ARM: dts: exynos5250: add DSI node

[Krzysztof Kozlowski]

On Wed, Jul 25, 2018 at 05:46:41PM +0200, Andrzej Hajda wrote:
> The patch adds common part of DSI node for Exynos5250 platforms
> and a required mipi-phy node.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250.dtsi | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

Thanks, applied.

Best regards,
Krzysztof

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 7/9] ARM: dts: exynos5250-arndale: add DSI and panel nodes

[Krzysztof Kozlowski]

On Wed, Jul 25, 2018 at 05:46:42PM +0200, Andrzej Hajda wrote:
> The patch adds bridge and panel nodes.
> It adds also DSI properties specific for arndale board and
> regulators required by the bridge.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250-arndale.dts | 61 ++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)

Thanks, applied.

Best regards,
Krzysztof

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 8/9] drm/panel: simple: fix BOE/HV070WSA-100 timings

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 413 bytes --]

On Wed, Jul 25, 2018 at 05:46:43PM +0200, Andrzej Hajda wrote:
> Panel timings were taken from vendor code and are not fully correct - refresh
> rate is about 50Hz instead of 60Hz. The patch fixes it.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel