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@xxxxxxxxx>
> > Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Cc: Jonas Karlman <jonas@xxxxxxxxx>
> > Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Kate Stewart <kstewart@xxxxxxxxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Allison Randal <allison@xxxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: "Gustavo A. R. Silva" <gustavo@xxxxxxxxxxxxxx>
> > ---
> > {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@xxxxxxxxxxx>
> > * Dharam Kumar <dharam.kr@xxxxxxxxxxx>
> > */
> > -#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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
