[PATCH] drm/bridge: Move mhl.h into driver directory

Wed Apr 15 18:06:20 UTC 2020

On Wed, Apr 15, 2020 at 08:48:06PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Wed, Apr 15, 2020 at 07:38:33PM +0200, Daniel Vetter wrote:
> > include/drm/bridge is a bit a mistake, drivers are supposed to find
> > their bridges using one of the standard of_* functions the drm_bridge
> > core provides.
> 
> I'm confused, I don't really see how that's related to mhl.h. The header
> defines constants and structures related to the MHL (Mobile
> High-Definition Link) protocol, which is an industry standard. If you
> want to move it out of include/drm/bridge/ to eventually remove that
> directory, I think it should be renamted to include/drm/drm_mhl.h.

It looked misplaced at least ... I guess moving this out of drm/bridge
makes more sense.

> > dw-hdmi and analogix-dp are the only, historically
> > grown exception that we haven't managed to get rid of yet.
> 
> The reason why we have shared headers for those is because they're IP
> cores integrated with different glue layers in different SoCs. There's
> one driver for the IP core itself, and SoC-specific glue drivers that
> need to provide the IP core drivers with data and callbacks, defined in
> shared headers. Granted, there's also data in those headers that are
> only internal to the IP core drivers, and that should be moved out, but
> for the interface header, include/drm/bridge/ doesn't seem to be a bad
> location to me.

The thing that irks me on them is that they kinda implement bridges, but
they don't load like bridges. That's the part I think should get changed,
or we need to finally figure out what exactly isn't good with the current
drm_bridge handling and get that fixed (the relevant patches seem forever
stuck in limbo, hence why I'm kicking).

If that's not possible because these things just dont fit as drm_bridge,
then maybe they shouldn't be a bridge, but something else. But looking at
both dw-hdmi and analogix-dp these things look a lot like midlayers that
get fed huge structures. Instead of more bare-bones toolboxes to build a
set of similar drm_bridge drivers, which drivers then bind into using dt.

So all a bit fishy imo.

I guess step 1 at least would be to throw the connector and encoder code
out of all these drivers, that would be at least a first step.

Next one maybe push the per-variant bind code into drm/bridge and out of
drivers, and use more standard of_ functions to find the bridges and tie
them into the drm_device.

Then 3rd round, some refactoring to demidlayer these libraries and make
them real toolboxes.
-Daniel

> 
> > Make sure that at least no new ones grow by moving hardware header
> > files into the correct driver directory.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > Cc: Sam Ravnborg <sam at ravnborg.org>
> > Cc: Andrzej Hajda <a.hajda at samsung.com>
> > Cc: Neil Armstrong <narmstrong at baylibre.com>
> > Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> > Cc: Jonas Karlman <jonas at kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec at siol.net>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Kate Stewart <kstewart at linuxfoundation.org>
> > Cc: Thomas Gleixner <tglx at linutronix.de>
> > Cc: Allison Randal <allison at lohutok.net>
> > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > Cc: "Gustavo A. R. Silva" <gustavo at embeddedor.com>
> > ---
> >  {include => drivers/gpu}/drm/bridge/mhl.h | 0
> >  drivers/gpu/drm/bridge/sii9234.c          | 3 ++-
> >  drivers/gpu/drm/bridge/sil-sii8620.c      | 2 +-
> >  3 files changed, 3 insertions(+), 2 deletions(-)
> >  rename {include => drivers/gpu}/drm/bridge/mhl.h (100%)
> > 
> > diff --git a/include/drm/bridge/mhl.h b/drivers/gpu/drm/bridge/mhl.h
> > similarity index 100%
> > rename from include/drm/bridge/mhl.h
> > rename to drivers/gpu/drm/bridge/mhl.h
> > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
> > index b1258f0ed205..4c862c3af038 100644
> > --- a/drivers/gpu/drm/bridge/sii9234.c
> > +++ b/drivers/gpu/drm/bridge/sii9234.c
> > @@ -12,7 +12,6 @@
> >   *    Shankar Bandal <shankar.b at samsung.com>
> >   *    Dharam Kumar <dharam.kr at samsung.com>
> >   */
> > -#include <drm/bridge/mhl.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_edid.h>
> > @@ -29,6 +28,8 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  
> > +#include "mhl.h"
> > +
> >  #define CBUS_DEVCAP_OFFSET		0x80
> >  
> >  #define SII9234_MHL_VERSION		0x11
> > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> > index 92acd336aa89..017dbb67404e 100644
> > --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> > @@ -8,7 +8,6 @@
> >  
> >  #include <asm/unaligned.h>
> >  
> > -#include <drm/bridge/mhl.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_edid.h>
> > @@ -31,6 +30,7 @@
> >  
> >  #include <media/rc-core.h>
> >  
> > +#include "mhl.h"
> >  #include "sil-sii8620.h"
> >  
> >  #define SII8620_BURST_BUF_LEN 288
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch