Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir

[Thierry Reding]

On Fri, Sep 04, 2015 at 11:29:30PM +0200, Heiko Stuebner wrote:
> Am Freitag, 4. September 2015, 16:06:02 schrieb Rob Herring:
> > On Tue, Sep 1, 2015 at 3:46 PM, Heiko Stuebner <heiko@xxxxxxxxx> wrote:
> > > Am Dienstag, 1. September 2015, 13:49:58 schrieb Yakir Yang:
> > >> Split the dp core driver from exynos directory to bridge
> > >> directory, and rename the core driver to analogix_dp_*,
> > >> leave the platform code to analogix_dp-exynos.
> > >> 
> > >> Signed-off-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>
> > > 
> > > [...]
> > > 
> > >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c similarity index 50%
> > >> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > >> index bed0252..7d62f22 100644
> > >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > 
> > > [...]
> > > 
> > >>       connector->polled = DRM_CONNECTOR_POLL_HPD;
> > >>       
> > >>       ret = drm_connector_init(dp->drm_dev, connector,
> > >> 
> > >> -                              &exynos_dp_connector_funcs,
> > >> +                              &analogix_dp_connector_funcs,
> > >> 
> > >>                                DRM_MODE_CONNECTOR_eDP);
> > >>       
> > >>       if (ret) {
> > >>       
> > >>               DRM_ERROR("Failed to initialize connector with drm\n");
> > >>               return ret;
> > >>       
> > >>       }
> > >> 
> > >> -     drm_connector_helper_add(connector,
> > >> &exynos_dp_connector_helper_funcs); +    
> > >> drm_connector_helper_add(connector,
> > >> +                              &analogix_dp_connector_helper_funcs);
> > >> 
> > >>       drm_connector_register(connector);
> > > 
> > > this should only run on exynos, as we're doing all our connector
> > > registration in the core driver after all components are bound, so I
> > > guess something like> 
> > > the following is needed:
> > >        if (dp->plat_data && dp->plat_data->dev_type == EXYNOS_DP)
> > >        
> > >                drm_connector_register(connector);
> > 
> > Yuck!
> > 
> > Surely there is a better way. From what I've seen of DRM, I have no
> > doubt this is needed, but really a better solution is needed. Surely
> > there can be a more generic way for the driver to determine if it
> > should handle the connector or not. This seems like a common problem
> > including one I have seen. What I'm working on has onchip DSI encoder
> > -> ADV7533 -> HDMI. The DSI encoder can also have a direct attached
> > panel. So I have to check for a bridge in the encoder driver and only
> > register the connector for the panel if a bridge is not attached.
> 
> I'm also only a part-time drm meddler, so things may be inaccurate.
> 
> This conditional is not meant to prevent the registration of the dp connector, 
> only delay it for non-exynos drms. The connector registration does publish it 
> to userspace, so like i.MX we're doing that for all connectors at the same 
> time after all components are bound - to also make x11 happy. Exynos on the 
> other hand seems to register its connectors individually and I'm not sure if 
> they would be willing to change that.
> 
> see d3007dabeff4 ("drm/rockchip: register all connectors after bind") or 
> simply rockchip_drm_drv.c line 178.
> 
> and e355e7dd607b ("imx-drm: delay publishing sysfs connector entries")

There really shouldn't be a reason for both drivers to behave
differently in this case. Gustavo has done a lot of work on cleaning up
the Exynos driver lately, so perhaps you should sync up with him to see
if this is something that he can integrate with his changes.

If you can't reach an agreement and Exynos must keep the exact behaviour
as it has now, then perhaps a better option would be to have the
Analogix core driver not register the connector but rather leave that up
to users of the helpers. Then you don't have to clutter the core driver
with this type of platform-specific data.

Thierry

Attachment: signature.asc
Description: PGP signature