[PATCH 10/60] drm/bridge: Add bridge driver for display connectors

[Laurent Pinchart]

Hi Sam,

On Tue, Jul 16, 2019 at 11:28:57AM +0200, Sam Ravnborg wrote:
> On Sun, Jul 07, 2019 at 09:18:47PM +0300, Laurent Pinchart wrote:
> > Display connectors are modelled in DT as a device node, but have so far
> > been handled manually in several bridge drivers. This resulted in
> > duplicate code in several bridge drivers, with slightly different (and
> > thus confusing) logics.
> > 
> > In order to fix this, implement a bridge driver for display connectors.
> > The driver centralises logic for the DVI, HDMI, VGAn composite and
> > S-video connectors and exposes corresponding bridge operations.
> > 
> > This driver in itself doesn't solve the issue completely, changes in
> > bridge and display controller drivers are needed to make use of the new
> > connector driver.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig             |  11 +
> >  drivers/gpu/drm/bridge/Makefile            |   1 +
> >  drivers/gpu/drm/bridge/display-connector.c | 327 +++++++++++++++++++++
> >  3 files changed, 339 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/display-connector.c
> > 
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index a78392e2dbb9..295a62f65ef9 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -37,6 +37,17 @@ config DRM_CDNS_DSI
> >  	  Support Cadence DPI to DSI bridge. This is an internal
> >  	  bridge and is meant to be directly embedded in a SoC.
> >  
> > +config DRM_DISPLAY_CONNECTOR
> > +	tristate "Display connector support"
> > +	depends on OF
> > +	help
> > +	  Driver for display connectors with support for DDC and hot-plug
> > +	  detection. Most display controller handle display connectors
> > +	  internally and don't need this driver, but the DRM subsystem is
> > +	  moving towards separating connector handling from display controllers
> > +	  on ARM-based platforms. Saying Y here when this driver is not needed
> > +	  will not cause any issue.
> > +
> >  config DRM_LVDS_ENCODER
> >  	tristate "Transparent parallel to LVDS encoder support"
> >  	depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 6ff7f2adbb0e..e5987b3aaf62 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > +obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
> >  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> > diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> > new file mode 100644
> > index 000000000000..2e1e7ee89275
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/display-connector.c
> > @@ -0,0 +1,327 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > + */
> > +
> ...
> > +
> > +static void display_connector_hpd_enable(struct drm_bridge *bridge)
> > +{
> > +}
> > +
> > +static void display_connector_hpd_disable(struct drm_bridge *bridge)
> > +{
> > +}
> 
> It seems wrong that a new driver needs empty implementation of
> hpd_enable() and hpd_disable().
> I noticed the same in a later patch too.
> 
> Can we do without these empty functions?

Absolutely, I'll fix that.

> > +
> > +static const struct drm_bridge_funcs display_connector_bridge_funcs = {
> > +	.attach = display_connector_attach,
> > +	.detect = display_connector_detect,
> > +	.get_edid = display_connector_get_edid,
> > +	.hpd_enable = display_connector_hpd_enable,
> > +	.hpd_disable = display_connector_hpd_disable,
> > +};
> > +
> > +	struct display_connector *conn = arg;
> > +	struct drm_bridge *bridge = &conn->bridge;
> > +
> > +	drm_bridge_hpd_notify(bridge, display_connector_detect(bridge));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static const char *display_connector_type_name(struct display_connector *conn)
> > +{
> > +	switch (conn->bridge.type) {
> > +	case DRM_MODE_CONNECTOR_Composite:
> > +		return "Composite";
> > +	case DRM_MODE_CONNECTOR_DVIA:
> > +		return "DVI-A";
> > +	case DRM_MODE_CONNECTOR_DVID:
> > +		return "DVI-D";
> > +	case DRM_MODE_CONNECTOR_DVII:
> > +		return "DVI-I";
> > +	case DRM_MODE_CONNECTOR_HDMIA:
> > +		return "HDMI-A";
> > +	case DRM_MODE_CONNECTOR_HDMIB:
> > +		return "HDMI-B";
> > +	case DRM_MODE_CONNECTOR_SVIDEO:
> > +		return "S-Video";
> > +	case DRM_MODE_CONNECTOR_VGA:
> > +		return "VGA";
> > +	default:
> > +		return "unknown";
> > +	}
> > +}
> We already have the relation DRM_MODE_CONNECTOR <=> name in drm_connector -
> see drm_connector_enum_list.
> 
> Add a small function in drm_connector.c get the name, so we do not hardcode the
> name twice?

That will be in v2.

-- 
Regards,

Laurent Pinchart