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

Wed Apr 15 18:12:23 UTC 2020

Hi Daniel,

On Wed, Apr 15, 2020 at 08:06:20PM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 08:48:06PM +0300, Laurent Pinchart wrote:
> > 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).

dw-hdmi certainly loads like a bridge when used with R-Car DU :-) Are
you referring to the component-based probe mode for that driver ?

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

Oh yes!! DRM_BRIDGE_ATTACH_NO_CONNECTOR for everybody :-) It's a
step-by-step process though:

1. Convert bridge drivers to support both modes (adding support for
   DRM_BRIDGE_ATTACH_NO_CONNECTOR).
2. Convert display drivers to make use of DRM_BRIDGE_ATTACH_NO_CONNECTOR
   (with the DRM bridge connector helper, or custom code if really
   needed).
3. Remove support for the !DRM_BRIDGE_ATTACH_NO_CONNECTOR mode in bridge
   drivers.
4. Drop the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag itself.

Sam and I are working on the first step (I'll convert the dw-hdmi driver
soon), and we're passing the message around that new bridge drivers
must support DRM_BRIDGE_ATTACH_NO_CONNECTOR and new display controller
drivers must use it.

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