Re: [PATCH v8 03/10] drm/bridge: Implement generic USB Type-C DP HPD bridge

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index a250afd8d662..17257b223a28 100644
> > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > @@ -23,13 +23,16 @@ config DRM_AUX_BRIDGE
> > >   	  build bridges chain.
> > >   config DRM_AUX_HPD_BRIDGE
> > > -	tristate
> > > +	tristate "AUX HPD bridge support"
> > Why? No, this is supposed to be selected by other drivers. Users don't
> > know an wouldn't know what is this.
> 
> In v7, I implemented an additional module for selecting this option. But
> Heikki believes that it would be better to merge the two modules into one.

Like I said before, I was merely curious why not just squash the
support into that AUX_PD_HPD_BRIDGE. If that does not make sense, then
so be it - make it a "Display Interface Bridge" driver like you
originally proposed.

> > >   	depends on DRM_BRIDGE && OF
> > >   	select AUXILIARY_BUS
> > >   	help
> > >   	  Simple bridge that terminates the bridge chain and provides HPD
> > >   	  support.
> > > +	  Specifically, if you want a default Type-C DisplayPort HPD bridge for
> > > +	  each port of the Type-C controller, say Y here.
> > > +
> > >   menu "Display Interface Bridges"
> > >   	depends on DRM && DRM_BRIDGE
> > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > index c7dc03182e59..2998937444bc 100644
> > > --- a/drivers/gpu/drm/bridge/Makefile
> > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > @@ -1,6 +1,12 @@
> > >   # SPDX-License-Identifier: GPL-2.0
> > >   obj-$(CONFIG_DRM_AUX_BRIDGE) += aux-bridge.o
> > > -obj-$(CONFIG_DRM_AUX_HPD_BRIDGE) += aux-hpd-bridge.o
> > > +
> > > +hpd-bridge-y := aux-hpd-bridge.o
> > > +ifneq ($(CONFIG_TYPEC),)
> > > +hpd-bridge-y += aux-hpd-typec-dp-bridge.o
> > > +endif
> > > +obj-$(CONFIG_DRM_AUX_HPD_BRIDGE) += hpd-bridge.o
> > > +
> > >   obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
> > >   obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
> > >   obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
> > > diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> > > index 2e9c702c7087..11ad6dc776c7 100644
> > > --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> > > @@ -12,6 +12,8 @@
> > >   #include <drm/drm_bridge.h>
> > >   #include <drm/bridge/aux-bridge.h>
> > > +#include "aux-hpd-bridge.h"
> > > +
> > >   static DEFINE_IDA(drm_aux_hpd_bridge_ida);
> > >   struct drm_aux_hpd_bridge_data {
> > > @@ -204,7 +206,26 @@ static struct auxiliary_driver drm_aux_hpd_bridge_drv = {
> > >   	.id_table = drm_aux_hpd_bridge_table,
> > >   	.probe = drm_aux_hpd_bridge_probe,
> > >   };
> > > -module_auxiliary_driver(drm_aux_hpd_bridge_drv);
> > > +
> > > +static int drm_aux_hpd_bridge_mod_init(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = auxiliary_driver_register(&drm_aux_hpd_bridge_drv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return drm_aux_hpd_typec_dp_bridge_init();
> > > +}
> > > +
> > > +static void drm_aux_hpd_bridge_mod_exit(void)
> > > +{
> > > +	drm_aux_hpd_typec_dp_bridge_exit();
> > > +	auxiliary_driver_unregister(&drm_aux_hpd_bridge_drv);
> > > +}
> > > +
> > > +module_init(drm_aux_hpd_bridge_mod_init);
> > > +module_exit(drm_aux_hpd_bridge_mod_exit);
> > >   MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
> > >   MODULE_DESCRIPTION("DRM HPD bridge");
> > > diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.h b/drivers/gpu/drm/bridge/aux-hpd-bridge.h
> > > new file mode 100644
> > > index 000000000000..69364731c2f1
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.h
> > > @@ -0,0 +1,13 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +#ifndef AUX_HPD_BRIDGE_H
> > > +#define AUX_HPD_BRIDGE_H
> > > +
> > > +#if IS_REACHABLE(CONFIG_TYPEC)
> > > +int drm_aux_hpd_typec_dp_bridge_init(void);
> > > +void drm_aux_hpd_typec_dp_bridge_exit(void);
> > > +#else
> > > +static inline int drm_aux_hpd_typec_dp_bridge_init(void) { return 0; }
> > > +static inline void drm_aux_hpd_typec_dp_bridge_exit(void) { }
> > > +#endif /* IS_REACHABLE(CONFIG_TYPEC) */
> > > +
> > > +#endif /* AUX_HPD_BRIDGE_H */
> > > diff --git a/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c
> > > new file mode 100644
> > > index 000000000000..6f2a1fca0fc5
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c
> > > @@ -0,0 +1,47 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +#include <linux/of.h>
> > > +#include <linux/usb/typec_altmode.h>
> > > +#include <linux/usb/typec_dp.h>
> > > +#include <linux/usb/typec_notify.h>
> > > +
> > > +#include <drm/bridge/aux-bridge.h>
> > > +
> > > +#include "aux-hpd-bridge.h"
> > > +
> > > +#if IS_REACHABLE(CONFIG_TYPEC)
> > > +static int drm_typec_bus_event(struct notifier_block *nb,
> > > +			       unsigned long action, void *data)
> > > +{
> > This feels like this should be a part of the Type-C subsystem rather
> > than DRM.
> 
> In v7, this used to be a part of the Type-C subsystem. I'm not sure what
> Heikki thinks about this.

Your original proposal of making the entire TYPEC subsystem depend on
DRM is _not_ going to happen. In general, if I've now understood this
correctly, this thing probable should be a "display interface bridge
driver", similar to what you proposed in the previous version.

Note also that you could make it selected automatically, so there is
no need for user selectable option if that's the preference. Kconfig
and Makefile gives you options on how to do that. For example, maybe
this Kconfig works (or does not, but something like it will):

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index a250afd8d662..7487024ba2ce 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -30,6 +30,15 @@ config DRM_AUX_HPD_BRIDGE
          Simple bridge that terminates the bridge chain and provides HPD
          support.
 
+if DRM_AUX_HPD_BRIDGE
+
+config DRM_AUX_HPD_TYPEC_BRIDGE
+       tristate
+       depends on TYPEC || !TYPEC
+       default TYPEC
+
+endif /* DRM_AUX_HPD_BRIDGE */
+
 menu "Display Interface Bridges"
        depends on DRM && DRM_BRIDGE
 


thanks,

-- 
heikki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy