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
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,
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
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) |
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
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;
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;
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
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
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) {
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) {
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:"
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:"
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:"
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:
- 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.
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:
- 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
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:
- 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:"
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:
- 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"?
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:
- 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
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().
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
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);
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);
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);
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>; + }; + }; + }; +};
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");
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Archit Taneja -
Boris Brezillon -
Eric Anholt