[RFC 2/2] drm/bridge/ptn3460: add panel support

[Ajay kumar]

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
>
>
>
>
>
>