On Tue, Sep 23, 2025 at 09:34:39AM +0800, Chaoyi Chen wrote:
> On 9/23/2025 9:10 AM, Dmitry Baryshkov wrote:
>
> > On Mon, Sep 22, 2025 at 09:20:33AM +0800, Chaoyi Chen wrote:
> > > From: Chaoyi Chen <[email protected]>
> > >
> > > Add default DRM AUX HPD bridge device when register DisplayPort
> > > altmode. That makes it redundant for each Type-C driver to implement
> > > a similar registration process in embedded scenarios.
> > >
> > > Signed-off-by: Chaoyi Chen <[email protected]>
> > > ---
> > > drivers/usb/typec/altmodes/displayport.c | 27 ++++++++++++++++++++++++
> > > drivers/usb/typec/altmodes/displayport.h | 2 ++
> > > drivers/usb/typec/class.c | 8 +++++++
> > > include/linux/usb/typec_altmode.h | 2 ++
> > > 4 files changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/altmodes/displayport.c
> > > b/drivers/usb/typec/altmodes/displayport.c
> > > index 1dcb77faf85d..e026dc6e5430 100644
> > > --- a/drivers/usb/typec/altmodes/displayport.c
> > > +++ b/drivers/usb/typec/altmodes/displayport.c
> > > @@ -14,6 +14,7 @@
> > > #include <linux/property.h>
> > > #include <linux/usb/pd_vdo.h>
> > > #include <linux/usb/typec_dp.h>
> > > +#include <drm/bridge/aux-bridge.h>
> > > #include <drm/drm_connector.h>
> > > #include "displayport.h"
> > > @@ -182,6 +183,10 @@ static int dp_altmode_status_update(struct
> > > dp_altmode *dp)
> > > dp->pending_irq_hpd = true;
> > > }
> > > } else {
> > > + if (dp->port->hpd_dev)
> > > + drm_aux_hpd_bridge_notify(dp->port->hpd_dev,
> > > + hpd ?
> > > connector_status_connected :
> > > +
> > > connector_status_disconnected);
> > There should be no need for these calls. Once the HPD bridge is added to
> > a correct fwnode, the drm_connector_oob_hotplug_event() calls should
> > deliver the signal as expected.
>
> It seems that only drm_bridge_connector can do this. I'm not sure if I
> remember correctly. I'll give it a try.
Other connectors can implement the .oob_hotplug_event call. Calling
drm_bridge_hpd_notify() also depends on the connector setting the
callbacks via drm_bridge_hpd_enable(), a step which is done by only a
few drivers.
>
>
> >
> > > drm_connector_oob_hotplug_event(dp->connector_fwnode,
> > > hpd ?
> > > connector_status_connected :
> > >
> > > connector_status_disconnected);
> > > @@ -206,6 +211,9 @@ static int dp_altmode_configured(struct dp_altmode
> > > *dp)
> > > * configuration is complete to signal HPD.
> > > */
> > > if (dp->pending_hpd) {
> > > + if (dp->port->hpd_dev)
> > > + drm_aux_hpd_bridge_notify(dp->port->hpd_dev,
> > > + connector_status_connected);
> > > drm_connector_oob_hotplug_event(dp->connector_fwnode,
> > >
> > > connector_status_connected);
> > > sysfs_notify(&dp->alt->dev.kobj, "displayport", "hpd");
> > > @@ -391,6 +399,9 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
> > > dp->data.status = 0;
> > > dp->data.conf = 0;
> > > if (dp->hpd) {
> > > + if (dp->port->hpd_dev)
> > > +
> > > drm_aux_hpd_bridge_notify(dp->port->hpd_dev,
> > > +
> > > connector_status_disconnected);
> > >
> > > drm_connector_oob_hotplug_event(dp->connector_fwnode,
> > >
> > > connector_status_disconnected);
> > > dp->hpd = false;
> > > @@ -751,6 +762,18 @@ static const struct attribute_group
> > > *displayport_groups[] = {
> > > NULL,
> > > };
> > > +void dp_altmode_hpd_device_register(struct typec_altmode *alt)
> > > +{
> > > + if (alt->svid != USB_TYPEC_DP_SID)
> > > + return;
> > > +
> > > + alt->hpd_dev = drm_dp_hpd_bridge_register(alt->dev.parent->parent,
> > > +
> > > dev_of_node(alt->dev.parent->parent));
> > This needs at least a comment, what is dev.parent->parent. Also, the
> > of_node is not correct here. It should be a node of the connector,
> > rather than the device itself. Consider USB-C controllers which handle
> > several USB-C connectors (e.g. UCSI). The DRM core won't be able to
> > identify the correct bridge.
>
> I think alt.dev->parent->parent is the connector device. Am I missing
> something?
As I wrote, it needs a comment (in the source file). No, it's not a
connector device, it's a USB-C controller device. There is no guarantee
that there is a separate struct device for the USB-C connector. On
Qualcomm platforms, the device will point to the USB-C controller (TCPM
or UCSI), which contain usb-c-connector(s) as child node(s) in DT.
>
>
>
> >
> > > + if (IS_ERR(alt->hpd_dev))
> > > + alt->hpd_dev = NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dp_altmode_hpd_device_register);
> > Having the function here will bring a typec -> displayport dependency
> > between drivers (which you didn't document). It means it won't be
> > possible to build typec core into the kernel, having the DP AltMode
> > driver in the module (which also doesn't sound like a good idea).
>
> It make sense. Perhaps moving it into class.c would be a good idea.
>
>
> >
> > > +
> > > int dp_altmode_probe(struct typec_altmode *alt)
> > > {
> > > const struct typec_altmode *port =
> > > typec_altmode_get_partner(alt);
>
> --
> Best,
> Chaoyi
>
>
> --
> linux-phy mailing list
> [email protected]
> https://lists.infradead.org/mailman/listinfo/linux-phy
--
With best wishes
Dmitry
