Re: [PATCH v4 1/7] usb: typec: Add default HPD device when register DisplayPort altmode

[Chaoyi Chen <chaoyi.chen@rock-chips.com>]

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 <chaoyi.chen@rock-chips.com>
>>
>> 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 <chaoyi.chen@rock-chips.com>
>> ---
>>   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.


>
>>   		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?



>
>> +	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