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.pinch...@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.pinch...@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel