Rob,

Signed-off-by: Rob Clark

drivers/gpu/drm/bridge/ptn3460.c | 39 +++++++++++++++++++++++++++++++++------ include/drm/bridge/ptn3460.h | 6 ++++-- 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index b171901..e3e6b46 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -26,6 +26,7 @@ #include "drm_crtc_helper.h"

#include "bridge/ptn3460.h" +#include "drm_bridge_util.h"

#define PTN3460_EDID_ADDR 0x0 #define PTN3460_EDID_EMULATION_ADDR 0x84 @@ -112,7 +113,6 @@ static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) static void ptn3460_pre_enable(struct drm_bridge *bridge) { struct ptn3460_bridge *ptn_bridge = bridge->driver_private;

• int ret;

if (ptn_bridge->enabled) return; @@ -132,6 +132,15 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)

• time specified in the chip's datasheet to make sure we're really up.

*/ msleep(90); +}

+static void ptn3460_enable(struct drm_bridge *bridge) +{

• struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
• int ret;
• if (ptn_bridge->enabled)
• return;

No need to move it into enable. We should keep this entirely inside pre_enable itself.

ret = ptn3460_select_edid(ptn_bridge); if (ret) @@ -140,10 +149,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) ptn_bridge->enabled = true; }

-static void ptn3460_enable(struct drm_bridge *bridge) -{ -}

static void ptn3460_disable(struct drm_bridge *bridge) { struct ptn3460_bridge *ptn_bridge = bridge->driver_private; @@ -265,7 +270,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = { };

int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,

• struct i2c_client *client, struct device_node *node)

• struct i2c_client *client, struct device_node *node,
• struct drm_panel *panel)

{ int ret; struct drm_bridge *bridge; @@ -324,6 +330,27 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }

• if (panel) {
• struct drm_bridge *cbridge, *pbridge;
• pbridge = drm_panel_bridge_init(panel);
• if (IS_ERR(pbridge)) {
• ret = PTR_ERR(pbridge);
• goto err;
• }
• /* panel sequence is after ptn4360 bridge bridge in
• • enable path, before in disable path:
• */
• cbridge = drm_composite_bridge_init(bridge, pbridge);
• if (IS_ERR(cbridge)) {
• ret = PTR_ERR(cbridge);
• goto err;
• }
• bridge = cbridge;
• }

We cannot have this here. Because, at this point the drm_panel probe would not have happened and the drm_panel would not have got added into the panel_list itself!

Instead, the panel discovery should be moved into "detect" callback of the ptn3460 connector. I tried this, and it causes one more problem. We cannot make a call to drm_panel_bridge_init/drm_composite_bridge_init sicne both of them call "drm_bridge_init", and drm_bridge_init tries to acquire dev->mode_config.mutex via drm_modeset_lock_all(dev), and the same lock is already held by drm core!

I could somehow test your patchset on a board with ptn3460 by commenting the locking part inside drm_bridge_init, and it works only with all these hacks.

bridge->driver_private = ptn_bridge; encoder->bridge = bridge;

diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h index ff62344..9a557da 100644 --- a/include/drm/bridge/ptn3460.h +++ b/include/drm/bridge/ptn3460.h @@ -16,18 +16,20 @@

struct drm_device; struct drm_encoder; +struct drm_panel; struct i2c_client; struct device_node;

#if defined(CONFIG_DRM_PTN3460) || defined(CONFIG_DRM_PTN3460_MODULE)

int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,

• struct i2c_client *client, struct device_node *node);

• struct i2c_client *client, struct device_node *node,
• struct drm_panel *panel);

#else

static inline int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, struct i2c_client *client,

• struct device_node *node)

• struct device_node *node, struct drm_panel *panel)

{ return 0; } -- 1.9.0