[PATCH 0/8] RPi touchscreen panel driver v4

[PATCH 0/8] RPi touchscreen panel driver v4

[Eric Anholt]

The review for v3 was basically "no, the panel should probe first so
that we have the connector by the time KMS is done initializing."  To
do this, I needed to be able to register the custom (non-OF-generated)
DSI device without the host being present (patch 6).  Also check out
patch 4 for a new cleanup to panel-bridge.

Eric Anholt (8):
  drm/vc4: Fix DSI T_INIT timing.
  drm/vc4: Fix misleading name of the continuous flag.
  drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.
  drm/bridge: Add a devm_ allocator for panel bridge.
  drm/vc4: Delay DSI host registration until the panel has probed.
  drm: Allow DSI devices to be registered before the host registers.
  dt-bindings: Document the Raspberry Pi Touchscreen nodes.
  drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

 .../panel/raspberrypi,7inch-touchscreen.txt        |  49 ++
 drivers/gpu/drm/bridge/panel.c                     |  30 ++
 drivers/gpu/drm/drm_mipi_dsi.c                     |  49 +-
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 505 +++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_dsi.c                      |  64 +--
 include/drm/drm_bridge.h                           |   3 +
 include/drm/drm_mipi_dsi.h                         |   3 +
 9 files changed, 671 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

-- 
2.11.0

[PATCH 1/8] drm/vc4: Fix DSI T_INIT timing.

[Eric Anholt]

The DPHY spec requires a much larger T_INIT than I was specifying
before.  In the absence of clear specs from the slave of what their
timing is, just use the value that the firmware was using.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 5e8b81eaa168..15f6d5005ab9 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1035,7 +1035,17 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 				     DSI_HS_DLT4_TRAIL) |
 		       VC4_SET_FIELD(0, DSI_HS_DLT4_ANLAT));
 
-	DSI_PORT_WRITE(HS_DLT5, VC4_SET_FIELD(dsi_hs_timing(ui_ns, 1000, 5000),
+	/* T_INIT is how long STOP is driven after power-up to
+	 * indicate to the slave (also coming out of power-up) that
+	 * master init is complete, and should be greater than the
+	 * maximum of two value: T_INIT,MASTER and T_INIT,SLAVE.  The
+	 * D-PHY spec gives a minimum 100us for T_INIT,MASTER and
+	 * T_INIT,SLAVE, while allowing protocols on top of it to give
+	 * greater minimums.  The vc4 firmware uses an extremely
+	 * conservative 5ms, and we maintain that here.
+	 */
+	DSI_PORT_WRITE(HS_DLT5, VC4_SET_FIELD(dsi_hs_timing(ui_ns,
+							    5 * 1000 * 1000, 0),
 					      DSI_HS_DLT5_INIT));
 
 	DSI_PORT_WRITE(HS_DLT6,
-- 
2.11.0

[PATCH 2/8] drm/vc4: Fix misleading name of the continuous flag.

[Eric Anholt]

The logic was all right in the end, the name was just backwards.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 15f6d5005ab9..629d372633e6 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -736,18 +736,18 @@ static void vc4_dsi_latch_ulps(struct vc4_dsi *dsi, bool latch)
 /* Enters or exits Ultra Low Power State. */
 static void vc4_dsi_ulps(struct vc4_dsi *dsi, bool ulps)
 {
-	bool continuous = dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS;
-	u32 phyc_ulps = ((continuous ? DSI_PORT_BIT(PHYC_CLANE_ULPS) : 0) |
+	bool non_continuous = dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS;
+	u32 phyc_ulps = ((non_continuous ? DSI_PORT_BIT(PHYC_CLANE_ULPS) : 0) |
 			 DSI_PHYC_DLANE0_ULPS |
 			 (dsi->lanes > 1 ? DSI_PHYC_DLANE1_ULPS : 0) |
 			 (dsi->lanes > 2 ? DSI_PHYC_DLANE2_ULPS : 0) |
 			 (dsi->lanes > 3 ? DSI_PHYC_DLANE3_ULPS : 0));
-	u32 stat_ulps = ((continuous ? DSI1_STAT_PHY_CLOCK_ULPS : 0) |
+	u32 stat_ulps = ((non_continuous ? DSI1_STAT_PHY_CLOCK_ULPS : 0) |
 			 DSI1_STAT_PHY_D0_ULPS |
 			 (dsi->lanes > 1 ? DSI1_STAT_PHY_D1_ULPS : 0) |
 			 (dsi->lanes > 2 ? DSI1_STAT_PHY_D2_ULPS : 0) |
 			 (dsi->lanes > 3 ? DSI1_STAT_PHY_D3_ULPS : 0));
-	u32 stat_stop = ((continuous ? DSI1_STAT_PHY_CLOCK_STOP : 0) |
+	u32 stat_stop = ((non_continuous ? DSI1_STAT_PHY_CLOCK_STOP : 0) |
 			 DSI1_STAT_PHY_D0_STOP |
 			 (dsi->lanes > 1 ? DSI1_STAT_PHY_D1_STOP : 0) |
 			 (dsi->lanes > 2 ? DSI1_STAT_PHY_D2_STOP : 0) |
-- 
2.11.0

[PATCH 3/8] drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.

[Eric Anholt]

I'm not sure what changed where I started getting vrefresh=0 from the
mode to be fixed up.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 629d372633e6..fca4d7fd677e 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -866,7 +866,9 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
 	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
 
 	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
-	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
+	adjusted_mode->htotal = pixel_clock_hz / (drm_mode_vrefresh(mode) *
+						  mode->vtotal);
+
 	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
 	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
 
-- 
2.11.0

[PATCH 4/8] drm/bridge: Add a devm_ allocator for panel bridge.

[Eric Anholt]

This will let drivers reduce the error cleanup they need, in
particular the "is_panel_bridge" flag.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h       |  3 +++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 67fe19e5a9c6..cdf367d2653a 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -198,3 +198,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
 	devm_kfree(panel_bridge->panel->dev, bridge);
 }
 EXPORT_SYMBOL(drm_panel_bridge_remove);
+
+static void devm_drm_panel_bridge_release(struct device *dev, void *res)
+{
+	struct drm_bridge *bridge = *(struct drm_bridge **)res;
+
+	drm_panel_bridge_remove(bridge);
+}
+
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+					     struct drm_panel *panel,
+					     u32 connector_type)
+{
+	struct drm_bridge **ptr, *bridge;
+
+	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	bridge = drm_panel_bridge_add(panel, connector_type);
+	if (!IS_ERR(bridge)) {
+		*ptr = bridge;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return bridge;
+}
+EXPORT_SYMBOL(devm_drm_panel_bridge_add);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 1dc94d5392e2..6522d4cbc9d9 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
 					u32 connector_type);
 void drm_panel_bridge_remove(struct drm_bridge *bridge);
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+					     struct drm_panel *panel,
+					     u32 connector_type);
 #endif
 
 #endif
-- 
2.11.0

[PATCH 5/8] drm/vc4: Delay DSI host registration until the panel has probed.

[Eric Anholt]

The vc4 driver was unusual in that it was delaying the panel lookup
until the attach step, while most DSI hosts will -EPROBE_DEFER until
they get a panel.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index fca4d7fd677e..31627522dd8c 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
@@ -504,7 +505,6 @@ struct vc4_dsi {
 	struct mipi_dsi_host dsi_host;
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
-	bool is_panel_bridge;
 
 	void __iomem *regs;
 
@@ -1290,7 +1290,6 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
 	struct vc4_dsi *dsi = host_to_dsi(host);
-	int ret = 0;
 
 	dsi->lanes = device->lanes;
 	dsi->channel = device->channel;
@@ -1325,34 +1324,12 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 		return 0;
 	}
 
-	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
-	if (!dsi->bridge) {
-		struct drm_panel *panel =
-			of_drm_find_panel(device->dev.of_node);
-
-		dsi->bridge = drm_panel_bridge_add(panel,
-						   DRM_MODE_CONNECTOR_DSI);
-		if (IS_ERR(dsi->bridge)) {
-			ret = PTR_ERR(dsi->bridge);
-			dsi->bridge = NULL;
-			return ret;
-		}
-		dsi->is_panel_bridge = true;
-	}
-
 	return drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);
 }
 
 static int vc4_dsi_host_detach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
-	struct vc4_dsi *dsi = host_to_dsi(host);
-
-	if (dsi->is_panel_bridge) {
-		drm_panel_bridge_remove(dsi->bridge);
-		dsi->bridge = NULL;
-	}
-
 	return 0;
 }
 
@@ -1496,6 +1473,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_dsi *dsi;
 	struct vc4_dsi_encoder *vc4_dsi_encoder;
+	struct drm_panel *panel;
 	const struct of_device_id *match;
 	dma_cap_mask_t dma_mask;
 	int ret;
@@ -1599,6 +1577,20 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
+					  &panel, &dsi->bridge);
+	if (ret) {
+		dev_info(dev, "find panel: %d\n", ret);
+		return ret;
+	}
+
+	if (panel) {
+		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
+							DRM_MODE_CONNECTOR_DSI);
+		if (IS_ERR(dsi->bridge))
+			return PTR_ERR(dsi->bridge);
+	}
+
 	/* The esc clock rate is supposed to always be 100Mhz. */
 	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
 	if (ret) {
-- 
2.11.0

[PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

When a mipi_dsi_host is registered, the DT is walked to find any child
nodes with compatible strings.  Those get registered as DSI devices,
and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.

There is one special case currently, the adv7533 bridge, where the
bridge probes on I2C, and during the bridge attach step it looks up
the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
mipi_dsi_driver).

For the Raspberry Pi panel, though, we also need to attach on I2C (our
control bus), but don't have a bridge driver.  The lack of a bridge's
attach() step like adv7533 uses means that we aren't able to delay the
mipi_dsi_device creation until the mipi_dsi_host is present.

To fix this, we extend mipi_dsi_device_register_full() to allow being
called with a NULL host, which puts the device on a queue waiting for
a host to appear.  When a new host is registered, we fill in the host
value and finish the device creation process.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++----------
 include/drm/drm_mipi_dsi.h     |  3 +++
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 1160a579e0dc..9cdd68a7dc0d 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -45,6 +45,13 @@
  * subset of the MIPI DCS command set.
  */
 
+static DEFINE_MUTEX(host_lock);
+static LIST_HEAD(host_list);
+/* List of struct mipi_dsi_device which were registered while no host
+ * was available.
+ */
+static LIST_HEAD(unattached_device_list);
+
 static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
@@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
 
 	dsi->host = host;
 	dsi->dev.bus = &mipi_dsi_bus_type;
-	dsi->dev.parent = host->dev;
 	dsi->dev.type = &mipi_dsi_device_type;
 
-	device_initialize(&dsi->dev);
+	if (dsi->host) {
+		dsi->dev.parent = host->dev;
+		device_initialize(&dsi->dev);
+	}
 
 	return dsi;
 }
@@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 			      const struct mipi_dsi_device_info *info)
 {
 	struct mipi_dsi_device *dsi;
-	struct device *dev = host->dev;
+	struct device *dev = host ? host->dev : NULL;
 	int ret;
 
 	if (!info) {
@@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 	dsi->channel = info->channel;
 	strlcpy(dsi->name, info->type, sizeof(dsi->name));
 
-	ret = mipi_dsi_device_add(dsi);
-	if (ret) {
-		dev_err(dev, "failed to add DSI device %d\n", ret);
-		kfree(dsi);
-		return ERR_PTR(ret);
+	if (!dsi->host) {
+		mutex_lock(&host_lock);
+		list_add(&dsi->list, &unattached_device_list);
+		mutex_unlock(&host_lock);
+	} else {
+		ret = mipi_dsi_device_add(dsi);
+		if (ret) {
+			dev_err(dev, "failed to add DSI device %d\n", ret);
+			kfree(dsi);
+			return ERR_PTR(ret);
+		}
 	}
 
 	return dsi;
@@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
 }
 EXPORT_SYMBOL(mipi_dsi_device_unregister);
 
-static DEFINE_MUTEX(host_lock);
-static LIST_HEAD(host_list);
-
 /**
  * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a
  *				     device tree node
@@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
 int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;
+	struct mipi_dsi_device *dsi, *temp;
 
 	for_each_available_child_of_node(host->dev->of_node, node) {
 		/* skip nodes without reg property */
@@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
 
 	mutex_lock(&host_lock);
 	list_add_tail(&host->list, &host_list);
+
+	/* If any DSI devices were registered under our OF node, then
+	 * connect our host to it and probe them now.
+	 */
+	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
+		if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
+			dsi->host = host;
+			dsi->dev.parent = host->dev;
+			device_initialize(&dsi->dev);
+
+			mipi_dsi_device_add(dsi);
+			list_del_init(&dsi->list);
+		}
+	}
 	mutex_unlock(&host_lock);
 
 	return 0;
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4fef19064b0f..699ea4acd5b6 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -178,6 +178,9 @@ struct mipi_dsi_device {
 	unsigned int lanes;
 	enum mipi_dsi_pixel_format format;
 	unsigned long mode_flags;
+
+	/* Entry on the unattached_device_list */
+	struct list_head list;
 };
 
 #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
-- 
2.11.0

[PATCH 7/8] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

[Eric Anholt]

This doesn't yet cover input, but the driver does get the display
working when the firmware is disabled from talking to our I2C lines.

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../panel/raspberrypi,7inch-touchscreen.txt        | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt

diff --git a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
new file mode 100644
index 000000000000..e9e19c059260
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
@@ -0,0 +1,49 @@
+This binding covers the official 7" (800x480) Raspberry Pi touchscreen
+panel.
+
+This DSI panel contains:
+
+- TC358762 DSI->DPI bridge
+- Atmel microcontroller on I2C for power sequencing the DSI bridge and
+  controlling backlight
+- Touchscreen controller on I2C for touch input
+
+and this binding covers the DSI display parts but not its touch input.
+
+Required properties:
+- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"
+- reg:		Must be "45"
+- port:		See panel-common.txt
+
+Example:
+
+dsi1: dsi@7e700000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	<...>
+
+	port {
+		dsi_out_port: endpoint {
+			remote-endpoint = <&panel_dsi_port>;
+		};
+	};
+};
+
+i2c_dsi: i2c {
+	compatible = "i2c-gpio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+
+	lcd@45 {
+		compatible = "raspberrypi,7inch-touchscreen-panel";
+		reg = <0x45>;
+
+		port {
+			panel_dsi_port: endpoint {
+				remote-endpoint = <&dsi_out_port>;
+			};
+		};
+	};
+};
-- 
2.11.0

[PATCH 8/8] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

[Eric Anholt]

This driver communicates with the Atmel microcontroller for sequencing
the poweron of the TC358762 DSI-DPI bridge and controlling the
backlight PWM.

v2: Set the same default orientation as the closed source firmware
    used, which is the best for viewing angle.
v3: Rewrite as an i2c client driver after bridge driver rejection.
v4: Finish probe without the DSI host, using the new delayed
    registration, and attach to the host during mipi_dsi_driver probe.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 505 +++++++++++++++++++++
 3 files changed, 514 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d84a031fae24..d6f1969b8a3b 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -73,6 +73,14 @@ config DRM_PANEL_PANASONIC_VVX10F034N00
 	  WUXGA (1920x1200) Novatek NT1397-based DSI panel as found in some
 	  Xperia Z2 tablets
 
+config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
+	tristate "Raspberry Pi 7-inch touchscreen panel"
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for the Raspberry
+	  Pi 7" Touchscreen.  To compile this driver as a module,
+	  choose M here.
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 9f6610d08b00..bd17fac21c7b 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
+obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
new file mode 100644
index 000000000000..b23331051d79
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -0,0 +1,505 @@
+/*
+ * Copyright © 2016-2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Portions of this file (derived from panel-simple.c) are:
+ *
+ * Copyright (C) 2013, NVIDIA Corporation.  All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Raspberry Pi 7" touchscreen panel driver.
+ *
+ * The 7" touchscreen consists of a DPI LCD panel, a Toshiba
+ * TC358762XBG DSI-DPI bridge, and an I2C-connected Atmel ATTINY88-MUR
+ * controlling power management, the LCD PWM, and initial register
+ * setup of the Tohsiba.
+ *
+ * This driver controls the TC358762 and ATTINY88, presenting a DSI
+ * device with a drm_panel.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pm.h>
+
+#include <drm/drm_panel.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define RPI_DSI_DRIVER_NAME "rpi-ts-dsi"
+
+/* I2C registers of the Atmel microcontroller. */
+enum REG_ADDR {
+	REG_ID = 0x80,
+	REG_PORTA, /* BIT(2) for horizontal flip, BIT(3) for vertical flip */
+	REG_PORTB,
+	REG_PORTC,
+	REG_PORTD,
+	REG_POWERON,
+	REG_PWM,
+	REG_DDRA,
+	REG_DDRB,
+	REG_DDRC,
+	REG_DDRD,
+	REG_TEST,
+	REG_WR_ADDRL,
+	REG_WR_ADDRH,
+	REG_READH,
+	REG_READL,
+	REG_WRITEH,
+	REG_WRITEL,
+	REG_ID2,
+};
+
+/* We only turn the PWM on or off, without varying values. */
+#define RPI_TOUCHSCREEN_MAX_BRIGHTNESS 1
+
+/* DSI D-PHY Layer Registers */
+#define D0W_DPHYCONTTX		0x0004
+#define CLW_DPHYCONTRX		0x0020
+#define D0W_DPHYCONTRX		0x0024
+#define D1W_DPHYCONTRX		0x0028
+#define COM_DPHYCONTRX		0x0038
+#define CLW_CNTRL		0x0040
+#define D0W_CNTRL		0x0044
+#define D1W_CNTRL		0x0048
+#define DFTMODE_CNTRL		0x0054
+
+/* DSI PPI Layer Registers */
+#define PPI_STARTPPI		0x0104
+#define PPI_BUSYPPI		0x0108
+#define PPI_LINEINITCNT		0x0110
+#define PPI_LPTXTIMECNT		0x0114
+#define PPI_CLS_ATMR		0x0140
+#define PPI_D0S_ATMR		0x0144
+#define PPI_D1S_ATMR		0x0148
+#define PPI_D0S_CLRSIPOCOUNT	0x0164
+#define PPI_D1S_CLRSIPOCOUNT	0x0168
+#define CLS_PRE			0x0180
+#define D0S_PRE			0x0184
+#define D1S_PRE			0x0188
+#define CLS_PREP		0x01A0
+#define D0S_PREP		0x01A4
+#define D1S_PREP		0x01A8
+#define CLS_ZERO		0x01C0
+#define D0S_ZERO		0x01C4
+#define D1S_ZERO		0x01C8
+#define PPI_CLRFLG		0x01E0
+#define PPI_CLRSIPO		0x01E4
+#define HSTIMEOUT		0x01F0
+#define HSTIMEOUTENABLE		0x01F4
+
+/* DSI Protocol Layer Registers */
+#define DSI_STARTDSI		0x0204
+#define DSI_BUSYDSI		0x0208
+#define DSI_LANEENABLE		0x0210
+# define DSI_LANEENABLE_CLOCK		BIT(0)
+# define DSI_LANEENABLE_D0		BIT(1)
+# define DSI_LANEENABLE_D1		BIT(2)
+
+#define DSI_LANESTATUS0		0x0214
+#define DSI_LANESTATUS1		0x0218
+#define DSI_INTSTATUS		0x0220
+#define DSI_INTMASK		0x0224
+#define DSI_INTCLR		0x0228
+#define DSI_LPTXTO		0x0230
+#define DSI_MODE		0x0260
+#define DSI_PAYLOAD0		0x0268
+#define DSI_PAYLOAD1		0x026C
+#define DSI_SHORTPKTDAT		0x0270
+#define DSI_SHORTPKTREQ		0x0274
+#define DSI_BTASTA		0x0278
+#define DSI_BTACLR		0x027C
+
+/* DSI General Registers */
+#define DSIERRCNT		0x0300
+#define DSISIGMOD		0x0304
+
+/* DSI Application Layer Registers */
+#define APLCTRL			0x0400
+#define APLSTAT			0x0404
+#define APLERR			0x0408
+#define PWRMOD			0x040C
+#define RDPKTLN			0x0410
+#define PXLFMT			0x0414
+#define MEMWRCMD		0x0418
+
+/* LCDC/DPI Host Registers */
+#define LCDCTRL			0x0420
+#define HSR			0x0424
+#define HDISPR			0x0428
+#define VSR			0x042C
+#define VDISPR			0x0430
+#define VFUEN			0x0434
+
+/* DBI-B Host Registers */
+#define DBIBCTRL		0x0440
+
+/* SPI Master Registers */
+#define SPICMR			0x0450
+#define SPITCR			0x0454
+
+/* System Controller Registers */
+#define SYSSTAT			0x0460
+#define SYSCTRL			0x0464
+#define SYSPLL1			0x0468
+#define SYSPLL2			0x046C
+#define SYSPLL3			0x0470
+#define SYSPMCTRL		0x047C
+
+/* GPIO Registers */
+#define GPIOC			0x0480
+#define GPIOO			0x0484
+#define GPIOI			0x0488
+
+/* I2C Registers */
+#define I2CCLKCTRL		0x0490
+
+/* Chip/Rev Registers */
+#define IDREG			0x04A0
+
+/* Debug Registers */
+#define WCMDQUEUE		0x0500
+#define RCMDQUEUE		0x0504
+
+struct rpi_touchscreen {
+	struct drm_panel base;
+	struct mipi_dsi_device *dsi;
+	struct i2c_client *i2c;
+};
+
+static const struct drm_display_mode rpi_touchscreen_modes[] = {
+	{
+		/* Modeline comes from the Raspberry Pi firmware, with HFP=1
+		 * plugged in and clock re-computed from that.
+		 */
+		.clock = 25979400 / 1000,
+		.hdisplay = 800,
+		.hsync_start = 800 + 1,
+		.hsync_end = 800 + 1 + 2,
+		.htotal = 800 + 1 + 2 + 46,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 2,
+		.vtotal = 480 + 7 + 2 + 21,
+		.vrefresh = 60,
+	},
+};
+
+static struct rpi_touchscreen *panel_to_ts(struct drm_panel *panel)
+{
+	return container_of(panel, struct rpi_touchscreen, base);
+}
+
+static u8 rpi_touchscreen_i2c_read(struct rpi_touchscreen *ts, u8 reg)
+{
+	return i2c_smbus_read_byte_data(ts->i2c, reg);
+}
+
+static void rpi_touchscreen_i2c_write(struct rpi_touchscreen *ts,
+				      u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
+	if (ret)
+		dev_err(&ts->dsi->dev, "I2C write failed: %d\n", ret);
+}
+
+static int rpi_touchscreen_write(struct rpi_touchscreen *ts, u16 reg, u32 val)
+{
+#if 0
+	/* The firmware uses LP DSI transactions like this to bring up
+	 * the hardware, which should be faster than using I2C to then
+	 * pass to the Toshiba.  However, I was unable to get it to
+	 * work.
+	 */
+	u8 msg[] = {
+		reg,
+		reg >> 8,
+		val,
+		val >> 8,
+		val >> 16,
+		val >> 24,
+	};
+
+	mipi_dsi_dcs_write_buffer(ts->dsi, msg, sizeof(msg));
+#else
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRH, reg >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRL, reg);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEH, val >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEL, val);
+#endif
+
+	return 0;
+}
+
+static int rpi_touchscreen_disable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+	udelay(1);
+
+	return 0;
+}
+
+static int rpi_touchscreen_noop(struct drm_panel *panel)
+{
+	return 0;
+}
+
+static int rpi_touchscreen_enable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+	int i;
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
+	/* Wait for nPWRDWN to go low to indicate poweron is done. */
+	for (i = 0; i < 100; i++) {
+		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
+			break;
+	}
+
+	rpi_touchscreen_write(ts, DSI_LANEENABLE,
+			      DSI_LANEENABLE_CLOCK |
+			      DSI_LANEENABLE_D0);
+	rpi_touchscreen_write(ts, PPI_D0S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D1S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D0S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_D1S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_LPTXTIMECNT, 0x03);
+
+	rpi_touchscreen_write(ts, SPICMR, 0x00);
+	rpi_touchscreen_write(ts, LCDCTRL, 0x00100150);
+	rpi_touchscreen_write(ts, SYSCTRL, 0x040f);
+	msleep(100);
+
+	rpi_touchscreen_write(ts, PPI_STARTPPI, 0x01);
+	rpi_touchscreen_write(ts, DSI_STARTDSI, 0x01);
+	msleep(100);
+
+	/* Turn on the backlight. */
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+
+	/* Default to the same orientation as the closed source
+	 * firmware used for the panel.  Runtime rotation
+	 * configuration will be supported using VC4's plane
+	 * orientation bits.
+	 */
+	rpi_touchscreen_i2c_write(ts, REG_PORTA, BIT(2));
+
+	return 0;
+}
+
+static int rpi_touchscreen_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct drm_device *drm = panel->drm;
+	unsigned int i, num = 0;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_touchscreen_modes); i++) {
+		const struct drm_display_mode *m = &rpi_touchscreen_modes[i];
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_duplicate(drm, m);
+		if (!mode) {
+			dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, m->vrefresh);
+			continue;
+		}
+
+		mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (i == 0)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+
+		drm_mode_probed_add(connector, mode);
+		num++;
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = 154;
+	connector->display_info.height_mm = 86;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return num;
+}
+
+static const struct drm_panel_funcs rpi_touchscreen_funcs = {
+	.disable = rpi_touchscreen_disable,
+	.unprepare = rpi_touchscreen_noop,
+	.prepare = rpi_touchscreen_noop,
+	.enable = rpi_touchscreen_enable,
+	.get_modes = rpi_touchscreen_get_modes,
+};
+
+static int rpi_touchscreen_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct rpi_touchscreen *ts;
+	struct device_node *endpoint;
+	int ret, ver;
+	struct mipi_dsi_device_info info = {
+		.type = RPI_DSI_DRIVER_NAME,
+		.channel = 0,
+		.node = NULL,
+	};
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, ts);
+
+	ts->i2c = i2c;
+
+	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
+	if (ver < 0) {
+		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
+		return -ENODEV;
+	}
+
+	switch (ver) {
+	case 0xde: /* ver 1 */
+	case 0xc3: /* ver 2 */
+		break;
+	default:
+		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
+		return -ENODEV;
+	}
+
+	/* Turn off at boot, so we can cleanly sequence powering on. */
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	info.node = of_graph_get_remote_port(endpoint);
+	of_node_put(endpoint);
+
+	ts->dsi = mipi_dsi_device_register_full(NULL, &info);
+	if (IS_ERR(ts->dsi)) {
+		dev_err(dev, "DSI device registration failed: %ld\n",
+			PTR_ERR(ts->dsi));
+		return PTR_ERR(ts->dsi);
+	}
+
+	ts->dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
+			       MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			       MIPI_DSI_MODE_LPM);
+	ts->dsi->format = MIPI_DSI_FMT_RGB888;
+	ts->dsi->lanes = 1;
+
+	ts->base.dev = dev;
+	ts->base.funcs = &rpi_touchscreen_funcs;
+
+	/* This appears last, as it's what will unblock the DSI host
+	 * driver's probe function and start the attach process.
+	 */
+	ret = drm_panel_add(&ts->base);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rpi_touchscreen_remove(struct i2c_client *i2c)
+{
+	struct rpi_touchscreen *ts = i2c_get_clientdata(i2c);
+
+	mipi_dsi_detach(ts->dsi);
+
+	drm_panel_remove(&ts->base);
+
+	mipi_dsi_device_unregister(ts->dsi);
+	kfree(ts->dsi);
+
+	return 0;
+}
+
+static int rpi_touchscreen_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	int ret = mipi_dsi_attach(dsi);
+
+	if (ret)
+		dev_err(&dsi->dev, "failed to attach dsi to host: %d\n", ret);
+
+	return ret;
+}
+
+static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = {
+	.driver.name = RPI_DSI_DRIVER_NAME,
+	.probe = rpi_touchscreen_dsi_probe,
+};
+
+static const struct of_device_id rpi_touchscreen_of_ids[] = {
+	{ .compatible = "raspberrypi,7inch-touchscreen-panel" },
+	{ } /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, rpi_touchscreen_of_ids);
+
+static struct i2c_driver rpi_touchscreen_driver = {
+	.driver = {
+		.name = "rpi_touchscreen",
+		.of_match_table = rpi_touchscreen_of_ids,
+	},
+	.probe = rpi_touchscreen_probe,
+	.remove = rpi_touchscreen_remove,
+};
+
+static int __init rpi_touchscreen_init(void)
+{
+	mipi_dsi_driver_register(&rpi_touchscreen_dsi_driver);
+	return i2c_add_driver(&rpi_touchscreen_driver);
+}
+module_init(rpi_touchscreen_init);
+
+static void __exit rpi_touchscreen_exit(void)
+{
+	i2c_del_driver(&rpi_touchscreen_driver);
+	mipi_dsi_driver_unregister(&rpi_touchscreen_dsi_driver);
+}
+module_exit(rpi_touchscreen_exit);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi 7-inch touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Archit Taneja]

On 06/28/2017 01:28 AM, Eric Anholt wrote:
> When a mipi_dsi_host is registered, the DT is walked to find any child
> nodes with compatible strings.  Those get registered as DSI devices,
> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
> 
> There is one special case currently, the adv7533 bridge, where the
> bridge probes on I2C, and during the bridge attach step it looks up
> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
> mipi_dsi_driver).
> 
> For the Raspberry Pi panel, though, we also need to attach on I2C (our
> control bus), but don't have a bridge driver.  The lack of a bridge's
> attach() step like adv7533 uses means that we aren't able to delay the
> mipi_dsi_device creation until the mipi_dsi_host is present.
> 
> To fix this, we extend mipi_dsi_device_register_full() to allow being
> called with a NULL host, which puts the device on a queue waiting for
> a host to appear.  When a new host is registered, we fill in the host
> value and finish the device creation process.

This is quite a nice idea. The only bothering thing is the info.of_node usage
varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
bus).

For DSI children expressed in DT, the of_node in info holds the DT node
corresponding to the DSI child itself. For non-DT ones, this patch assumes
that info.of_node stores the DSI host DT node. I think it should be okay as
long as we mention the usage in a comment somewhere. The other option is to
have a new info.host_node field to keep a track of the host DT node.

Thanks,
Archit

> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>   drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++----------
>   include/drm/drm_mipi_dsi.h     |  3 +++
>   2 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 1160a579e0dc..9cdd68a7dc0d 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -45,6 +45,13 @@
>    * subset of the MIPI DCS command set.
>    */
>   
> +static DEFINE_MUTEX(host_lock);
> +static LIST_HEAD(host_list);
> +/* List of struct mipi_dsi_device which were registered while no host
> + * was available.
> + */
> +static LIST_HEAD(unattached_device_list);
> +
>   static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
>   {
>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
> @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
>   
>   	dsi->host = host;
>   	dsi->dev.bus = &mipi_dsi_bus_type;
> -	dsi->dev.parent = host->dev;
>   	dsi->dev.type = &mipi_dsi_device_type;
>   
> -	device_initialize(&dsi->dev);
> +	if (dsi->host) {
> +		dsi->dev.parent = host->dev;
> +		device_initialize(&dsi->dev);
> +	}
>   
>   	return dsi;
>   }
> @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>   			      const struct mipi_dsi_device_info *info)
>   {
>   	struct mipi_dsi_device *dsi;
> -	struct device *dev = host->dev;
> +	struct device *dev = host ? host->dev : NULL;
>   	int ret;
>   
>   	if (!info) {
> @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>   	dsi->channel = info->channel;
>   	strlcpy(dsi->name, info->type, sizeof(dsi->name));
>   
> -	ret = mipi_dsi_device_add(dsi);
> -	if (ret) {
> -		dev_err(dev, "failed to add DSI device %d\n", ret);
> -		kfree(dsi);
> -		return ERR_PTR(ret);
> +	if (!dsi->host) {
> +		mutex_lock(&host_lock);
> +		list_add(&dsi->list, &unattached_device_list);
> +		mutex_unlock(&host_lock);
> +	} else {
> +		ret = mipi_dsi_device_add(dsi);
> +		if (ret) {
> +			dev_err(dev, "failed to add DSI device %d\n", ret);
> +			kfree(dsi);
> +			return ERR_PTR(ret);
> +		}
>   	}
>   
>   	return dsi;
> @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
>   }
>   EXPORT_SYMBOL(mipi_dsi_device_unregister);
>   
> -static DEFINE_MUTEX(host_lock);
> -static LIST_HEAD(host_list);
> -
>   /**
>    * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a
>    *				     device tree node
> @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
>   int mipi_dsi_host_register(struct mipi_dsi_host *host)
>   {
>   	struct device_node *node;
> +	struct mipi_dsi_device *dsi, *temp;
>   
>   	for_each_available_child_of_node(host->dev->of_node, node) {
>   		/* skip nodes without reg property */
> @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>   
>   	mutex_lock(&host_lock);
>   	list_add_tail(&host->list, &host_list);
> +
> +	/* If any DSI devices were registered under our OF node, then
> +	 * connect our host to it and probe them now.
> +	 */
> +	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
> +		if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
> +			dsi->host = host;
> +			dsi->dev.parent = host->dev;
> +			device_initialize(&dsi->dev);
> +
> +			mipi_dsi_device_add(dsi);
> +			list_del_init(&dsi->list);
> +		}
> +	}
>   	mutex_unlock(&host_lock);
>   
>   	return 0;
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4fef19064b0f..699ea4acd5b6 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -178,6 +178,9 @@ struct mipi_dsi_device {
>   	unsigned int lanes;
>   	enum mipi_dsi_pixel_format format;
>   	unsigned long mode_flags;
> +
> +	/* Entry on the unattached_device_list */
> +	struct list_head list;
>   };
>   
>   #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/8] drm/vc4: Fix DSI T_INIT timing.

[Andrzej Hajda]

On 27.06.2017 21:58, Eric Anholt wrote:
> The DPHY spec requires a much larger T_INIT than I was specifying
> before.  In the absence of clear specs from the slave of what their
> timing is, just use the value that the firmware was using.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

Re: [PATCH 2/8] drm/vc4: Fix misleading name of the continuous flag.

[Andrzej Hajda]

On 27.06.2017 21:58, Eric Anholt wrote:
> The logic was all right in the end, the name was just backwards.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

Re: [PATCH 3/8] drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.

[Andrzej Hajda]

On 27.06.2017 21:58, Eric Anholt wrote:
> I'm not sure what changed where I started getting vrefresh=0 from the
> mode to be fixed up.

It can be a case of low pixel_clock value, maybe it should be
investigated further, unless there is execution path with forgotten
mode->vrefresh =

drm_mode_vrefresh(mode)

>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/vc4/vc4_dsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 629d372633e6..fca4d7fd677e 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -866,7 +866,9 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
>  	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
>  
>  	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
> -	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
> +	adjusted_mode->htotal = pixel_clock_hz / (drm_mode_vrefresh(mode) *
> +						  mode->vtotal);
> +

I am not sure but I guess division by zero is also possible here.
I do not know if you need to handle interlaced/dblscan/vscan modes, but
maybe it would be safer to calculate adjusted_htotal according to:

adjusted_mode->htotal = pixel_clock_hz * mode->htotal / (mode->clock * 1000)

or sth similar.


Regards
Andrzej

>  	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
>  	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
>  

Re: [PATCH 4/8] drm/bridge: Add a devm_ allocator for panel bridge.

[Andrzej Hajda]

On 27.06.2017 21:58, Eric Anholt wrote:
> This will let drivers reduce the error cleanup they need, in
> particular the "is_panel_bridge" flag.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h       |  3 +++
>  2 files changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 67fe19e5a9c6..cdf367d2653a 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -198,3 +198,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
>  	devm_kfree(panel_bridge->panel->dev, bridge);
>  }
>  EXPORT_SYMBOL(drm_panel_bridge_remove);
> +
> +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> +{
> +	struct drm_bridge *bridge = *(struct drm_bridge **)res;
> +
> +	drm_panel_bridge_remove(bridge);

I guess more elegant would be:
    struct drm_bridge **bridge = res;

    drm_panel_bridge_remove(*bridge);


I am still not convinced to the idea of this panel_bridge stuff, but
this is different issue :)

Anyway:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

> +}
> +
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type)
> +{
> +	struct drm_bridge **ptr, *bridge;
> +
> +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge = drm_panel_bridge_add(panel, connector_type);
> +	if (!IS_ERR(bridge)) {
> +		*ptr = bridge;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1dc94d5392e2..6522d4cbc9d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>  					u32 connector_type);
>  void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type);
>  #endif
>  
>  #endif

Re: [PATCH 5/8] drm/vc4: Delay DSI host registration until the panel has probed.

[Andrzej Hajda]

On 27.06.2017 21:58, Eric Anholt wrote:
> The vc4 driver was unusual in that it was delaying the panel lookup
> until the attach step, while most DSI hosts will -EPROBE_DEFER until
> they get a panel.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/vc4/vc4_dsi.c | 40 ++++++++++++++++------------------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index fca4d7fd677e..31627522dd8c 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> @@ -504,7 +505,6 @@ struct vc4_dsi {
>  	struct mipi_dsi_host dsi_host;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *bridge;
> -	bool is_panel_bridge;
>  
>  	void __iomem *regs;
>  
> @@ -1290,7 +1290,6 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
>  			       struct mipi_dsi_device *device)
>  {
>  	struct vc4_dsi *dsi = host_to_dsi(host);
> -	int ret = 0;
>  
>  	dsi->lanes = device->lanes;
>  	dsi->channel = device->channel;
> @@ -1325,34 +1324,12 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
>  		return 0;
>  	}
>  
> -	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
> -	if (!dsi->bridge) {
> -		struct drm_panel *panel =
> -			of_drm_find_panel(device->dev.of_node);
> -
> -		dsi->bridge = drm_panel_bridge_add(panel,
> -						   DRM_MODE_CONNECTOR_DSI);
> -		if (IS_ERR(dsi->bridge)) {
> -			ret = PTR_ERR(dsi->bridge);
> -			dsi->bridge = NULL;
> -			return ret;
> -		}
> -		dsi->is_panel_bridge = true;
> -	}
> -
>  	return drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);
>  }
>  
>  static int vc4_dsi_host_detach(struct mipi_dsi_host *host,
>  			       struct mipi_dsi_device *device)
>  {
> -	struct vc4_dsi *dsi = host_to_dsi(host);
> -
> -	if (dsi->is_panel_bridge) {
> -		drm_panel_bridge_remove(dsi->bridge);
> -		dsi->bridge = NULL;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1496,6 +1473,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  	struct vc4_dev *vc4 = to_vc4_dev(drm);
>  	struct vc4_dsi *dsi;
>  	struct vc4_dsi_encoder *vc4_dsi_encoder;
> +	struct drm_panel *panel;
>  	const struct of_device_id *match;
>  	dma_cap_mask_t dma_mask;
>  	int ret;
> @@ -1599,6 +1577,20 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>  
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> +					  &panel, &dsi->bridge);
> +	if (ret) {
> +		dev_info(dev, "find panel: %d\n", ret);

Message should probably mention also bridge.
Beside this:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

> +		return ret;
> +	}
> +
> +	if (panel) {
> +		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> +							DRM_MODE_CONNECTOR_DSI);
> +		if (IS_ERR(dsi->bridge))
> +			return PTR_ERR(dsi->bridge);
> +	}
> +
>  	/* The esc clock rate is supposed to always be 100Mhz. */
>  	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
>  	if (ret) {

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Andrzej Hajda]

On 29.06.2017 07:03, Archit Taneja wrote:
>
> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>> When a mipi_dsi_host is registered, the DT is walked to find any child
>> nodes with compatible strings.  Those get registered as DSI devices,
>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>
>> There is one special case currently, the adv7533 bridge, where the
>> bridge probes on I2C, and during the bridge attach step it looks up
>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>> mipi_dsi_driver).
>>
>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>> control bus), but don't have a bridge driver.  The lack of a bridge's
>> attach() step like adv7533 uses means that we aren't able to delay the
>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>
>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>> called with a NULL host, which puts the device on a queue waiting for
>> a host to appear.  When a new host is registered, we fill in the host
>> value and finish the device creation process.
> This is quite a nice idea. The only bothering thing is the info.of_node usage
> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
> bus).
>
> For DSI children expressed in DT, the of_node in info holds the DT node
> corresponding to the DSI child itself. For non-DT ones, this patch assumes
> that info.of_node stores the DSI host DT node. I think it should be okay as
> long as we mention the usage in a comment somewhere. The other option is to
> have a new info.host_node field to keep a track of the host DT node.

Field abuse is not a biggest issue.

This patch changes totally semantic of mipi_dsi_device_register_full.
Currently semantic of *_device_register* function is to create and add
device to existing bus, ie after return we have device attached to bus,
so it can be instantly used. With this change function can return some
unattached device, without warranty it will be ever attached - kind of
hidden deferring. Such change looks for me quite dangerous, even if it
looks convenient in this case.

As discussed in other thread more appealing solution for me would be:
1. host creates dsi bus, but doesn't call component_add as it does not
have all required resources.
2. host waits for all required dsi devs attached, gets registered panels
or bridges and calls component_add after that.
3. in bind phase it has all it needs, hasn't it?

I did not spent much time on this idea, so I cannot guarantee it has not
fundamental issues :)

Regards
Andrzej

>
> Thanks,
> Archit
>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>   drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++----------
>>   include/drm/drm_mipi_dsi.h     |  3 +++
>>   2 files changed, 41 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 1160a579e0dc..9cdd68a7dc0d 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -45,6 +45,13 @@
>>    * subset of the MIPI DCS command set.
>>    */
>>   
>> +static DEFINE_MUTEX(host_lock);
>> +static LIST_HEAD(host_list);
>> +/* List of struct mipi_dsi_device which were registered while no host
>> + * was available.
>> + */
>> +static LIST_HEAD(unattached_device_list);
>> +
>>   static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
>>   {
>>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
>> @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
>>   
>>   	dsi->host = host;
>>   	dsi->dev.bus = &mipi_dsi_bus_type;
>> -	dsi->dev.parent = host->dev;
>>   	dsi->dev.type = &mipi_dsi_device_type;
>>   
>> -	device_initialize(&dsi->dev);
>> +	if (dsi->host) {
>> +		dsi->dev.parent = host->dev;
>> +		device_initialize(&dsi->dev);
>> +	}
>>   
>>   	return dsi;
>>   }
>> @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>>   			      const struct mipi_dsi_device_info *info)
>>   {
>>   	struct mipi_dsi_device *dsi;
>> -	struct device *dev = host->dev;
>> +	struct device *dev = host ? host->dev : NULL;
>>   	int ret;
>>   
>>   	if (!info) {
>> @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>>   	dsi->channel = info->channel;
>>   	strlcpy(dsi->name, info->type, sizeof(dsi->name));
>>   
>> -	ret = mipi_dsi_device_add(dsi);
>> -	if (ret) {
>> -		dev_err(dev, "failed to add DSI device %d\n", ret);
>> -		kfree(dsi);
>> -		return ERR_PTR(ret);
>> +	if (!dsi->host) {
>> +		mutex_lock(&host_lock);
>> +		list_add(&dsi->list, &unattached_device_list);
>> +		mutex_unlock(&host_lock);
>> +	} else {
>> +		ret = mipi_dsi_device_add(dsi);
>> +		if (ret) {
>> +			dev_err(dev, "failed to add DSI device %d\n", ret);
>> +			kfree(dsi);
>> +			return ERR_PTR(ret);
>> +		}
>>   	}
>>   
>>   	return dsi;
>> @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
>>   }
>>   EXPORT_SYMBOL(mipi_dsi_device_unregister);
>>   
>> -static DEFINE_MUTEX(host_lock);
>> -static LIST_HEAD(host_list);
>> -
>>   /**
>>    * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a
>>    *				     device tree node
>> @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
>>   int mipi_dsi_host_register(struct mipi_dsi_host *host)
>>   {
>>   	struct device_node *node;
>> +	struct mipi_dsi_device *dsi, *temp;
>>   
>>   	for_each_available_child_of_node(host->dev->of_node, node) {
>>   		/* skip nodes without reg property */
>> @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>>   
>>   	mutex_lock(&host_lock);
>>   	list_add_tail(&host->list, &host_list);
>> +
>> +	/* If any DSI devices were registered under our OF node, then
>> +	 * connect our host to it and probe them now.
>> +	 */
>> +	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
>> +		if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
>> +			dsi->host = host;
>> +			dsi->dev.parent = host->dev;
>> +			device_initialize(&dsi->dev);
>> +
>> +			mipi_dsi_device_add(dsi);
>> +			list_del_init(&dsi->list);
>> +		}
>> +	}
>>   	mutex_unlock(&host_lock);
>>   
>>   	return 0;
>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>> index 4fef19064b0f..699ea4acd5b6 100644
>> --- a/include/drm/drm_mipi_dsi.h
>> +++ b/include/drm/drm_mipi_dsi.h
>> @@ -178,6 +178,9 @@ struct mipi_dsi_device {
>>   	unsigned int lanes;
>>   	enum mipi_dsi_pixel_format format;
>>   	unsigned long mode_flags;
>> +
>> +	/* Entry on the unattached_device_list */
>> +	struct list_head list;
>>   };
>>   
>>   #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
>>

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

Andrzej Hajda <a.hajda@samsung.com> writes:

> On 29.06.2017 07:03, Archit Taneja wrote:
>>
>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>> nodes with compatible strings.  Those get registered as DSI devices,
>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>
>>> There is one special case currently, the adv7533 bridge, where the
>>> bridge probes on I2C, and during the bridge attach step it looks up
>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>> mipi_dsi_driver).
>>>
>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>> attach() step like adv7533 uses means that we aren't able to delay the
>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>
>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>> called with a NULL host, which puts the device on a queue waiting for
>>> a host to appear.  When a new host is registered, we fill in the host
>>> value and finish the device creation process.
>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>> bus).
>>
>> For DSI children expressed in DT, the of_node in info holds the DT node
>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>> that info.of_node stores the DSI host DT node. I think it should be okay as
>> long as we mention the usage in a comment somewhere. The other option is to
>> have a new info.host_node field to keep a track of the host DT node.
>
> Field abuse is not a biggest issue.
>
> This patch changes totally semantic of mipi_dsi_device_register_full.
> Currently semantic of *_device_register* function is to create and add
> device to existing bus, ie after return we have device attached to bus,
> so it can be instantly used. With this change function can return some
> unattached device, without warranty it will be ever attached - kind of
> hidden deferring. Such change looks for me quite dangerous, even if it
> looks convenient in this case.

It only changes the semantic if you past in a NULL host, from "oops" to
"do something useful".

> As discussed in other thread more appealing solution for me would be:
> 1. host creates dsi bus, but doesn't call component_add as it does not
> have all required resources.
> 2. host waits for all required dsi devs attached, gets registered panels
> or bridges and calls component_add after that.
> 3. in bind phase it has all it needs, hasn't it?
>
> I did not spent much time on this idea, so I cannot guarantee it has not
> fundamental issues :)

If component_add() isn't called during probe, then DSI would just get
skipped during bind as far as I know.

I *think* what you're thinking is moving DSI host register to probe, and
then panel lookup stays in bind.  That seems much more risky to me -- do
we know for sure that no mipi_dsi_device will do any DSI transactions
during its probe?  I would expect some of them to.

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

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Andrzej Hajda]

On 29.06.2017 17:22, Eric Anholt wrote:
> Andrzej Hajda <a.hajda@samsung.com> writes:
>
>> On 29.06.2017 07:03, Archit Taneja wrote:
>>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>>> nodes with compatible strings.  Those get registered as DSI devices,
>>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>>
>>>> There is one special case currently, the adv7533 bridge, where the
>>>> bridge probes on I2C, and during the bridge attach step it looks up
>>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>>> mipi_dsi_driver).
>>>>
>>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>>> attach() step like adv7533 uses means that we aren't able to delay the
>>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>>
>>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>>> called with a NULL host, which puts the device on a queue waiting for
>>>> a host to appear.  When a new host is registered, we fill in the host
>>>> value and finish the device creation process.
>>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>>> bus).
>>>
>>> For DSI children expressed in DT, the of_node in info holds the DT node
>>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>>> that info.of_node stores the DSI host DT node. I think it should be okay as
>>> long as we mention the usage in a comment somewhere. The other option is to
>>> have a new info.host_node field to keep a track of the host DT node.
>> Field abuse is not a biggest issue.
>>
>> This patch changes totally semantic of mipi_dsi_device_register_full.
>> Currently semantic of *_device_register* function is to create and add
>> device to existing bus, ie after return we have device attached to bus,
>> so it can be instantly used. With this change function can return some
>> unattached device, without warranty it will be ever attached - kind of
>> hidden deferring. Such change looks for me quite dangerous, even if it
>> looks convenient in this case.
> It only changes the semantic if you past in a NULL host, from "oops" to
> "do something useful".
>
>> As discussed in other thread more appealing solution for me would be:
>> 1. host creates dsi bus, but doesn't call component_add as it does not
>> have all required resources.
>> 2. host waits for all required dsi devs attached, gets registered panels
>> or bridges and calls component_add after that.
>> 3. in bind phase it has all it needs, hasn't it?
>>
>> I did not spent much time on this idea, so I cannot guarantee it has not
>> fundamental issues :)
> If component_add() isn't called during probe, then DSI would just get
> skipped during bind as far as I know.

No, drm master will wait till all components are present.

>
> I *think* what you're thinking is moving DSI host register to probe, 

yes, this way you will allow to create dsi devices.

> and
> then panel lookup stays in bind. 

no, panel lookup will be performed in dsi_host attach callback, and
component_add will be called also there, if all required panels/bridges
are get.

>  That seems much more risky to me -- do
> we know for sure that no mipi_dsi_device will do any DSI transactions
> during its probe?  I would expect some of them to.

I think it is irrelevant, with the current design only transactions
between prepare/unprepare callbacks have chances to succeed.

Andrzej

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Archit Taneja]

On 06/29/2017 04:09 PM, Andrzej Hajda wrote:
> On 29.06.2017 07:03, Archit Taneja wrote:
>>
>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>> nodes with compatible strings.  Those get registered as DSI devices,
>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>
>>> There is one special case currently, the adv7533 bridge, where the
>>> bridge probes on I2C, and during the bridge attach step it looks up
>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>> mipi_dsi_driver).
>>>
>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>> attach() step like adv7533 uses means that we aren't able to delay the
>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>
>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>> called with a NULL host, which puts the device on a queue waiting for
>>> a host to appear.  When a new host is registered, we fill in the host
>>> value and finish the device creation process.
>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>> bus).
>>
>> For DSI children expressed in DT, the of_node in info holds the DT node
>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>> that info.of_node stores the DSI host DT node. I think it should be okay as
>> long as we mention the usage in a comment somewhere. The other option is to
>> have a new info.host_node field to keep a track of the host DT node.
> 
> Field abuse is not a biggest issue.
> 
> This patch changes totally semantic of mipi_dsi_device_register_full.
> Currently semantic of *_device_register* function is to create and add
> device to existing bus, ie after return we have device attached to bus,
> so it can be instantly used. With this change function can return some
> unattached device, without warranty it will be ever attached - kind of
> hidden deferring. Such change looks for me quite dangerous, even if it
> looks convenient in this case.
> 
> As discussed in other thread more appealing solution for me would be:
> 1. host creates dsi bus, but doesn't call component_add as it does not
> have all required resources.
> 2. host waits for all required dsi devs attached, gets registered panels
> or bridges and calls component_add after that.
> 3. in bind phase it has all it needs, hasn't it?

This seems like it would work, but would require KMS drivers to restructure
themselves around this approach. For KMS drivers that don't even use the
component stuff, it might be asking too much.

We could maybe consider Eric's patch as an intermediate solution, we should
definitely put WARN_ON(!dsi->host) like checks for all the existing
mipi_dsi_*() API.

About the solution that you and Boris discussed in the other thread, I'll give
this a try on the msm driver and see how it does.

Thanks,
Archit

> 
> I did not spent much time on this idea, so I cannot guarantee it has not
> fundamental issues :)
> 
> Regards
> Andrzej
> 
>>
>> Thanks,
>> Archit
>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>> ---
>>>    drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++----------
>>>    include/drm/drm_mipi_dsi.h     |  3 +++
>>>    2 files changed, 41 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>>> index 1160a579e0dc..9cdd68a7dc0d 100644
>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>>> @@ -45,6 +45,13 @@
>>>     * subset of the MIPI DCS command set.
>>>     */
>>>    
>>> +static DEFINE_MUTEX(host_lock);
>>> +static LIST_HEAD(host_list);
>>> +/* List of struct mipi_dsi_device which were registered while no host
>>> + * was available.
>>> + */
>>> +static LIST_HEAD(unattached_device_list);
>>> +
>>>    static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
>>>    {
>>>    	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
>>> @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
>>>    
>>>    	dsi->host = host;
>>>    	dsi->dev.bus = &mipi_dsi_bus_type;
>>> -	dsi->dev.parent = host->dev;
>>>    	dsi->dev.type = &mipi_dsi_device_type;
>>>    
>>> -	device_initialize(&dsi->dev);
>>> +	if (dsi->host) {
>>> +		dsi->dev.parent = host->dev;
>>> +		device_initialize(&dsi->dev);
>>> +	}
>>>    
>>>    	return dsi;
>>>    }
>>> @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>>>    			      const struct mipi_dsi_device_info *info)
>>>    {
>>>    	struct mipi_dsi_device *dsi;
>>> -	struct device *dev = host->dev;
>>> +	struct device *dev = host ? host->dev : NULL;
>>>    	int ret;
>>>    
>>>    	if (!info) {
>>> @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>>>    	dsi->channel = info->channel;
>>>    	strlcpy(dsi->name, info->type, sizeof(dsi->name));
>>>    
>>> -	ret = mipi_dsi_device_add(dsi);
>>> -	if (ret) {
>>> -		dev_err(dev, "failed to add DSI device %d\n", ret);
>>> -		kfree(dsi);
>>> -		return ERR_PTR(ret);
>>> +	if (!dsi->host) {
>>> +		mutex_lock(&host_lock);
>>> +		list_add(&dsi->list, &unattached_device_list);
>>> +		mutex_unlock(&host_lock);
>>> +	} else {
>>> +		ret = mipi_dsi_device_add(dsi);
>>> +		if (ret) {
>>> +			dev_err(dev, "failed to add DSI device %d\n", ret);
>>> +			kfree(dsi);
>>> +			return ERR_PTR(ret);
>>> +		}
>>>    	}
>>>    
>>>    	return dsi;
>>> @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
>>>    }
>>>    EXPORT_SYMBOL(mipi_dsi_device_unregister);
>>>    
>>> -static DEFINE_MUTEX(host_lock);
>>> -static LIST_HEAD(host_list);
>>> -
>>>    /**
>>>     * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a
>>>     *				     device tree node
>>> @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
>>>    int mipi_dsi_host_register(struct mipi_dsi_host *host)
>>>    {
>>>    	struct device_node *node;
>>> +	struct mipi_dsi_device *dsi, *temp;
>>>    
>>>    	for_each_available_child_of_node(host->dev->of_node, node) {
>>>    		/* skip nodes without reg property */
>>> @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>>>    
>>>    	mutex_lock(&host_lock);
>>>    	list_add_tail(&host->list, &host_list);
>>> +
>>> +	/* If any DSI devices were registered under our OF node, then
>>> +	 * connect our host to it and probe them now.
>>> +	 */
>>> +	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
>>> +		if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
>>> +			dsi->host = host;
>>> +			dsi->dev.parent = host->dev;
>>> +			device_initialize(&dsi->dev);
>>> +
>>> +			mipi_dsi_device_add(dsi);
>>> +			list_del_init(&dsi->list);
>>> +		}
>>> +	}
>>>    	mutex_unlock(&host_lock);
>>>    
>>>    	return 0;
>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>> index 4fef19064b0f..699ea4acd5b6 100644
>>> --- a/include/drm/drm_mipi_dsi.h
>>> +++ b/include/drm/drm_mipi_dsi.h
>>> @@ -178,6 +178,9 @@ struct mipi_dsi_device {
>>>    	unsigned int lanes;
>>>    	enum mipi_dsi_pixel_format format;
>>>    	unsigned long mode_flags;
>>> +
>>> +	/* Entry on the unattached_device_list */
>>> +	struct list_head list;
>>>    };
>>>    
>>>    #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
>>>
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

Archit Taneja <architt@codeaurora.org> writes:

> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>> When a mipi_dsi_host is registered, the DT is walked to find any child
>> nodes with compatible strings.  Those get registered as DSI devices,
>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>> 
>> There is one special case currently, the adv7533 bridge, where the
>> bridge probes on I2C, and during the bridge attach step it looks up
>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>> mipi_dsi_driver).
>> 
>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>> control bus), but don't have a bridge driver.  The lack of a bridge's
>> attach() step like adv7533 uses means that we aren't able to delay the
>> mipi_dsi_device creation until the mipi_dsi_host is present.
>> 
>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>> called with a NULL host, which puts the device on a queue waiting for
>> a host to appear.  When a new host is registered, we fill in the host
>> value and finish the device creation process.
>
> This is quite a nice idea. The only bothering thing is the info.of_node usage
> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
> bus).
>
> For DSI children expressed in DT, the of_node in info holds the DT node
> corresponding to the DSI child itself. For non-DT ones, this patch assumes
> that info.of_node stores the DSI host DT node. I think it should be okay as
> long as we mention the usage in a comment somewhere. The other option is to
> have a new info.host_node field to keep a track of the host DT node.

I think maybe you misread the patch?  We're using
of_get_parent(dsi->dev.node), which came from info->node, to compare to
host->dev->of_node().

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

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]

Archit Taneja <architt@codeaurora.org> writes:

> On 06/29/2017 04:09 PM, Andrzej Hajda wrote:
>> On 29.06.2017 07:03, Archit Taneja wrote:
>>>
>>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>>> nodes with compatible strings.  Those get registered as DSI devices,
>>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>>
>>>> There is one special case currently, the adv7533 bridge, where the
>>>> bridge probes on I2C, and during the bridge attach step it looks up
>>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>>> mipi_dsi_driver).
>>>>
>>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>>> attach() step like adv7533 uses means that we aren't able to delay the
>>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>>
>>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>>> called with a NULL host, which puts the device on a queue waiting for
>>>> a host to appear.  When a new host is registered, we fill in the host
>>>> value and finish the device creation process.
>>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>>> bus).
>>>
>>> For DSI children expressed in DT, the of_node in info holds the DT node
>>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>>> that info.of_node stores the DSI host DT node. I think it should be okay as
>>> long as we mention the usage in a comment somewhere. The other option is to
>>> have a new info.host_node field to keep a track of the host DT node.
>> 
>> Field abuse is not a biggest issue.
>> 
>> This patch changes totally semantic of mipi_dsi_device_register_full.
>> Currently semantic of *_device_register* function is to create and add
>> device to existing bus, ie after return we have device attached to bus,
>> so it can be instantly used. With this change function can return some
>> unattached device, without warranty it will be ever attached - kind of
>> hidden deferring. Such change looks for me quite dangerous, even if it
>> looks convenient in this case.
>> 
>> As discussed in other thread more appealing solution for me would be:
>> 1. host creates dsi bus, but doesn't call component_add as it does not
>> have all required resources.
>> 2. host waits for all required dsi devs attached, gets registered panels
>> or bridges and calls component_add after that.
>> 3. in bind phase it has all it needs, hasn't it?
>
> This seems like it would work, but would require KMS drivers to restructure
> themselves around this approach. For KMS drivers that don't even use the
> component stuff, it might be asking too much.
>
> We could maybe consider Eric's patch as an intermediate solution, we should
> definitely put WARN_ON(!dsi->host) like checks for all the existing
> mipi_dsi_*() API.

Could you clarify which entrypoints you'd like a warning on?  Is it just
"everything that gets the host ops"?

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

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Archit Taneja]

On 07/15/2017 04:28 AM, Eric Anholt wrote:
> Archit Taneja <architt@codeaurora.org> writes:
> 
>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>> nodes with compatible strings.  Those get registered as DSI devices,
>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>
>>> There is one special case currently, the adv7533 bridge, where the
>>> bridge probes on I2C, and during the bridge attach step it looks up
>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>> mipi_dsi_driver).
>>>
>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>> attach() step like adv7533 uses means that we aren't able to delay the
>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>
>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>> called with a NULL host, which puts the device on a queue waiting for
>>> a host to appear.  When a new host is registered, we fill in the host
>>> value and finish the device creation process.
>>
>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>> bus).
>>
>> For DSI children expressed in DT, the of_node in info holds the DT node
>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>> that info.of_node stores the DSI host DT node. I think it should be okay as
>> long as we mention the usage in a comment somewhere. The other option is to
>> have a new info.host_node field to keep a track of the host DT node.
> 
> I think maybe you misread the patch?  We're using
> of_get_parent(dsi->dev.node), which came from info->node, to compare to
> host->dev->of_node().

I think I did misread it.

Although, I'm not entirely clear what we should be setting info.node to.
In patch #8, info.node is set by:

	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
	info.node = of_graph_get_remote_port(endpoint);

Looking at the dt bindings in patch #7, it looks like info.node is set
to the 'port' device node in dsi@7e700000, is that right?

I suppose 'port' here seems like a reasonable representation of
dsi->dev.node, I wonder how it would work if the dsi host had multiple
ports underneath it. I.e:

dsi@7e700000 {
	...
	...
	ports {
		port@0 {
			...
			dsi_out_port: endpoint {
				remote-endpoint = <&panel_dsi_port>;
			};
		};
		port@1 {
			...
			...
		};
	};
};

Here, we would need to set info.node to the 'ports' node, so that
of_get_parent(dsi->dev.of_node) equals host->dev->of_node. That doesn't
seem correct.

Ideally, a dev's 'of_node' should be left to NULL if we don't have a
corresponding OF node. We're sort of overriding it here since we don't
have any other place to store this information in the mipi_dsi_device
struct.

Maybe we could add a 'host_node' entry in mipi_dsi_device itself, which
is exclusively used cases where the DSI device doesn't have a DT node.
Our check in mipi_dsi_host_register() could then be something like:

	if (dsi->host_node) == host->dev->of_node) {
		...
		...
	}

Since Thierry also reviews drm_mipi_dsi.c changes, it would nice to
get some comments from him too.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Archit Taneja]

On 07/15/2017 04:31 AM, Eric Anholt wrote:
> Archit Taneja <architt@codeaurora.org> writes:
> 
>> On 06/29/2017 04:09 PM, Andrzej Hajda wrote:
>>> On 29.06.2017 07:03, Archit Taneja wrote:
>>>>
>>>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>>>> nodes with compatible strings.  Those get registered as DSI devices,
>>>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>>>
>>>>> There is one special case currently, the adv7533 bridge, where the
>>>>> bridge probes on I2C, and during the bridge attach step it looks up
>>>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>>>> mipi_dsi_driver).
>>>>>
>>>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>>>> attach() step like adv7533 uses means that we aren't able to delay the
>>>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>>>
>>>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>>>> called with a NULL host, which puts the device on a queue waiting for
>>>>> a host to appear.  When a new host is registered, we fill in the host
>>>>> value and finish the device creation process.
>>>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>>>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>>>> bus).
>>>>
>>>> For DSI children expressed in DT, the of_node in info holds the DT node
>>>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>>>> that info.of_node stores the DSI host DT node. I think it should be okay as
>>>> long as we mention the usage in a comment somewhere. The other option is to
>>>> have a new info.host_node field to keep a track of the host DT node.
>>>
>>> Field abuse is not a biggest issue.
>>>
>>> This patch changes totally semantic of mipi_dsi_device_register_full.
>>> Currently semantic of *_device_register* function is to create and add
>>> device to existing bus, ie after return we have device attached to bus,
>>> so it can be instantly used. With this change function can return some
>>> unattached device, without warranty it will be ever attached - kind of
>>> hidden deferring. Such change looks for me quite dangerous, even if it
>>> looks convenient in this case.
>>>
>>> As discussed in other thread more appealing solution for me would be:
>>> 1. host creates dsi bus, but doesn't call component_add as it does not
>>> have all required resources.
>>> 2. host waits for all required dsi devs attached, gets registered panels
>>> or bridges and calls component_add after that.
>>> 3. in bind phase it has all it needs, hasn't it?
>>
>> This seems like it would work, but would require KMS drivers to restructure
>> themselves around this approach. For KMS drivers that don't even use the
>> component stuff, it might be asking too much.
>>
>> We could maybe consider Eric's patch as an intermediate solution, we should
>> definitely put WARN_ON(!dsi->host) like checks for all the existing
>> mipi_dsi_*() API.
> 
> Could you clarify which entrypoints you'd like a warning on?  Is it just
> "everything that gets the host ops"?

Sorry, all API was a bad suggestion.

I think a warning and early bail in mipi_dsi_attach() should be sufficient.
A panel/bridge DSI device shouldn't use any other API if it hasn't called
mipi_dsi_attach().

Thanks,
Archit

> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 4839 bytes --]

Archit Taneja <architt@codeaurora.org> writes:

> On 07/15/2017 04:28 AM, Eric Anholt wrote:
>> Archit Taneja <architt@codeaurora.org> writes:
>> 
>>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>>> nodes with compatible strings.  Those get registered as DSI devices,
>>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>>
>>>> There is one special case currently, the adv7533 bridge, where the
>>>> bridge probes on I2C, and during the bridge attach step it looks up
>>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>>> mipi_dsi_driver).
>>>>
>>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>>> attach() step like adv7533 uses means that we aren't able to delay the
>>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>>
>>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>>> called with a NULL host, which puts the device on a queue waiting for
>>>> a host to appear.  When a new host is registered, we fill in the host
>>>> value and finish the device creation process.
>>>
>>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>>> bus).
>>>
>>> For DSI children expressed in DT, the of_node in info holds the DT node
>>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>>> that info.of_node stores the DSI host DT node. I think it should be okay as
>>> long as we mention the usage in a comment somewhere. The other option is to
>>> have a new info.host_node field to keep a track of the host DT node.
>> 
>> I think maybe you misread the patch?  We're using
>> of_get_parent(dsi->dev.node), which came from info->node, to compare to
>> host->dev->of_node().
>
> I think I did misread it.
>
> Although, I'm not entirely clear what we should be setting info.node to.
> In patch #8, info.node is set by:
>
> 	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> 	info.node = of_graph_get_remote_port(endpoint);
>
> Looking at the dt bindings in patch #7, it looks like info.node is set
> to the 'port' device node in dsi@7e700000, is that right?

Yeah.


> I suppose 'port' here seems like a reasonable representation of
> dsi->dev.node, I wonder how it would work if the dsi host had multiple
> ports underneath it. I.e:
>
> dsi@7e700000 {
> 	...
> 	...
> 	ports {
> 		port@0 {
> 			...
> 			dsi_out_port: endpoint {
> 				remote-endpoint = <&panel_dsi_port>;
> 			};
> 		};
> 		port@1 {
> 			...
> 			...
> 		};
> 	};
> };
>
> Here, we would need to set info.node to the 'ports' node, so that
> of_get_parent(dsi->dev.of_node) equals host->dev->of_node. That doesn't
> seem correct.
>
> Ideally, a dev's 'of_node' should be left to NULL if we don't have a
> corresponding OF node. We're sort of overriding it here since we don't
> have any other place to store this information in the mipi_dsi_device
> struct.
>
> Maybe we could add a 'host_node' entry in mipi_dsi_device itself, which
> is exclusively used cases where the DSI device doesn't have a DT node.
> Our check in mipi_dsi_host_register() could then be something like:

I think instead of extending the struct, we can just walk to the parent
similarly to how of_graph_get_remove_port_parent() does.  And fix some
node refcounting at the same time:

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index ed3d505dc203..77d439254da6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -313,7 +313,12 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
         * connect our host to it and probe them now.
         */
        list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
-               if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
+               struct device_node *host_node = of_get_parent(dsi->dev.of_node);
+
+               if (!of_node_cmp(host_node->name, "ports"))
+                       host_node = of_get_next_parent(host_node);
+
+               if (host_node == host->dev->of_node) {
                        dsi->host = host;
                        dsi->dev.parent = host->dev;
                        device_initialize(&dsi->dev);
@@ -321,6 +326,8 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
                        mipi_dsi_device_add(dsi);
                        list_del_init(&dsi->list);
                }
+
+               of_node_put(host_node);
        }
        mutex_unlock(&host_lock);
 

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

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Archit Taneja]

On 07/19/2017 01:43 AM, Eric Anholt wrote:
> Archit Taneja <architt@codeaurora.org> writes:
> 
>> On 07/15/2017 04:28 AM, Eric Anholt wrote:
>>> Archit Taneja <architt@codeaurora.org> writes:
>>>
>>>> On 06/28/2017 01:28 AM, Eric Anholt wrote:
>>>>> When a mipi_dsi_host is registered, the DT is walked to find any child
>>>>> nodes with compatible strings.  Those get registered as DSI devices,
>>>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>>>>>
>>>>> There is one special case currently, the adv7533 bridge, where the
>>>>> bridge probes on I2C, and during the bridge attach step it looks up
>>>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
>>>>> mipi_dsi_driver).
>>>>>
>>>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
>>>>> control bus), but don't have a bridge driver.  The lack of a bridge's
>>>>> attach() step like adv7533 uses means that we aren't able to delay the
>>>>> mipi_dsi_device creation until the mipi_dsi_host is present.
>>>>>
>>>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
>>>>> called with a NULL host, which puts the device on a queue waiting for
>>>>> a host to appear.  When a new host is registered, we fill in the host
>>>>> value and finish the device creation process.
>>>>
>>>> This is quite a nice idea. The only bothering thing is the info.of_node usage
>>>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
>>>> bus).
>>>>
>>>> For DSI children expressed in DT, the of_node in info holds the DT node
>>>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
>>>> that info.of_node stores the DSI host DT node. I think it should be okay as
>>>> long as we mention the usage in a comment somewhere. The other option is to
>>>> have a new info.host_node field to keep a track of the host DT node.
>>>
>>> I think maybe you misread the patch?  We're using
>>> of_get_parent(dsi->dev.node), which came from info->node, to compare to
>>> host->dev->of_node().
>>
>> I think I did misread it.
>>
>> Although, I'm not entirely clear what we should be setting info.node to.
>> In patch #8, info.node is set by:
>>
>> 	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>> 	info.node = of_graph_get_remote_port(endpoint);
>>
>> Looking at the dt bindings in patch #7, it looks like info.node is set
>> to the 'port' device node in dsi@7e700000, is that right?
> 
> Yeah.
> 
> 
>> I suppose 'port' here seems like a reasonable representation of
>> dsi->dev.node, I wonder how it would work if the dsi host had multiple
>> ports underneath it. I.e:
>>
>> dsi@7e700000 {
>> 	...
>> 	...
>> 	ports {
>> 		port@0 {
>> 			...
>> 			dsi_out_port: endpoint {
>> 				remote-endpoint = <&panel_dsi_port>;
>> 			};
>> 		};
>> 		port@1 {
>> 			...
>> 			...
>> 		};
>> 	};
>> };
>>
>> Here, we would need to set info.node to the 'ports' node, so that
>> of_get_parent(dsi->dev.of_node) equals host->dev->of_node. That doesn't
>> seem correct.
>>
>> Ideally, a dev's 'of_node' should be left to NULL if we don't have a
>> corresponding OF node. We're sort of overriding it here since we don't
>> have any other place to store this information in the mipi_dsi_device
>> struct.
>>
>> Maybe we could add a 'host_node' entry in mipi_dsi_device itself, which
>> is exclusively used cases where the DSI device doesn't have a DT node.
>> Our check in mipi_dsi_host_register() could then be something like:
> 
> I think instead of extending the struct, we can just walk to the parent
> similarly to how of_graph_get_remove_port_parent() does.  And fix some
> node refcounting at the same time:

Yeah, I guess this works. The only thing that's a slight irritant is that
we're setting an 'of_node' to a device that doesn't have a DT
representation. But I don't have any strong feelings against it.

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

> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index ed3d505dc203..77d439254da6 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -313,7 +313,12 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>           * connect our host to it and probe them now.
>           */
>          list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
> -               if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
> +               struct device_node *host_node = of_get_parent(dsi->dev.of_node);
> +
> +               if (!of_node_cmp(host_node->name, "ports"))
> +                       host_node = of_get_next_parent(host_node);
> +
> +               if (host_node == host->dev->of_node) {
>                          dsi->host = host;
>                          dsi->dev.parent = host->dev;
>                          device_initialize(&dsi->dev);
> @@ -321,6 +326,8 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>                          mipi_dsi_device_add(dsi);
>                          list_del_init(&dsi->list);
>                  }
> +
> +               of_node_put(host_node);
>          }
>          mutex_unlock(&host_lock);
>   
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Boris Brezillon]

Hi all,

Sorry to enter the discussion so late in the review process.

Eric, I'm replying here because this is where the initial discussion
happened, but I actually reviewed v5 of your patchset.

On Tue, 18 Jul 2017 13:13:04 -0700
Eric Anholt <eric@anholt.net> wrote:

> Archit Taneja <architt@codeaurora.org> writes:
> 
> > On 07/15/2017 04:28 AM, Eric Anholt wrote:  
> >> Archit Taneja <architt@codeaurora.org> writes:
> >>   
> >>> On 06/28/2017 01:28 AM, Eric Anholt wrote:  
> >>>> When a mipi_dsi_host is registered, the DT is walked to find any child
> >>>> nodes with compatible strings.  Those get registered as DSI devices,
> >>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
> >>>>
> >>>> There is one special case currently, the adv7533 bridge, where the
> >>>> bridge probes on I2C, and during the bridge attach step it looks up
> >>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
> >>>> mipi_dsi_driver).
> >>>>
> >>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
> >>>> control bus), but don't have a bridge driver.  The lack of a bridge's
> >>>> attach() step like adv7533 uses means that we aren't able to delay the
> >>>> mipi_dsi_device creation until the mipi_dsi_host is present.
> >>>>
> >>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
> >>>> called with a NULL host, which puts the device on a queue waiting for
> >>>> a host to appear.  When a new host is registered, we fill in the host
> >>>> value and finish the device creation process.  
> >>>
> >>> This is quite a nice idea. The only bothering thing is the info.of_node usage
> >>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
> >>> bus).
> >>>
> >>> For DSI children expressed in DT, the of_node in info holds the DT node
> >>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
> >>> that info.of_node stores the DSI host DT node. I think it should be okay as
> >>> long as we mention the usage in a comment somewhere. The other option is to
> >>> have a new info.host_node field to keep a track of the host DT node.  
> >> 
> >> I think maybe you misread the patch?  We're using
> >> of_get_parent(dsi->dev.node), which came from info->node, to compare to
> >> host->dev->of_node().  
> >
> > I think I did misread it.
> >
> > Although, I'm not entirely clear what we should be setting info.node to.
> > In patch #8, info.node is set by:
> >
> > 	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> > 	info.node = of_graph_get_remote_port(endpoint);
> >
> > Looking at the dt bindings in patch #7, it looks like info.node is set
> > to the 'port' device node in dsi@7e700000, is that right?  
> 
> Yeah.
> 
> 
> > I suppose 'port' here seems like a reasonable representation of
> > dsi->dev.node, I wonder how it would work if the dsi host had multiple
> > ports underneath it. I.e:
> >
> > dsi@7e700000 {
> > 	...
> > 	...
> > 	ports {
> > 		port@0 {
> > 			...
> > 			dsi_out_port: endpoint {
> > 				remote-endpoint = <&panel_dsi_port>;
> > 			};
> > 		};
> > 		port@1 {
> > 			...
> > 			...
> > 		};
> > 	};
> > };
> >
> > Here, we would need to set info.node to the 'ports' node, so that
> > of_get_parent(dsi->dev.of_node) equals host->dev->of_node. That doesn't
> > seem correct.

I agree. I think we're abusing dev->of_node here, which makes the whole
thing even harder to understand.

> >
> > Ideally, a dev's 'of_node' should be left to NULL if we don't have a
> > corresponding OF node.

Well, there are cases where the device actually has a valid OF node,
like the adv7533, but this device is defined under a different control
bus (i2c in this case). So, to be accurate with the DT representation,
dsi->dev.of_node should be set to the adv7533 OF node (the one under
the I2C bus), right?

> > We're sort of overriding it here since we don't
> > have any other place to store this information in the mipi_dsi_device
> > struct.
> >
> > Maybe we could add a 'host_node' entry in mipi_dsi_device itself, which
> > is exclusively used cases where the DSI device doesn't have a DT node.

I think that would be clearer.

> > Our check in mipi_dsi_host_register() could then be something like:  
> 
> I think instead of extending the struct, we can just walk to the parent
> similarly to how of_graph_get_remove_port_parent() does.

It's working as long as dsi->dev.of_node points to one of the port node
defined under the DSI host, but is this node really representing the DSI
device?

Sure, your solution works, but it makes the whole DSI registration even
harder to follow.

> And fix some
> node refcounting at the same time:
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index ed3d505dc203..77d439254da6 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -313,7 +313,12 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>          * connect our host to it and probe them now.
>          */
>         list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
> -               if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
> +               struct device_node *host_node = of_get_parent(dsi->dev.of_node);
> +
> +               if (!of_node_cmp(host_node->name, "ports"))
> +                       host_node = of_get_next_parent(host_node);
> +
> +               if (host_node == host->dev->of_node) {
>                         dsi->host = host;
>                         dsi->dev.parent = host->dev;
>                         device_initialize(&dsi->dev);
> @@ -321,6 +326,8 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>                         mipi_dsi_device_add(dsi);
>                         list_del_init(&dsi->list);
>                 }
> +
> +               of_node_put(host_node);
>         }
>         mutex_unlock(&host_lock);
>