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 11:11 AM, Dmitry Baryshkov wrote:

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

Hmm, let's go over this again. First, drm_connector_oob_hotplug_event() requires a connector fwnode.

On the Qualcomm platforms, the fwnode corresponds to the USB-C controller device node, so

drm_connector_oob_hotplug_event(dp->connector_fwnode, ..) can handle them directly.

But our platform doesn't use the USB-C controller device node as drm connector fwnode :(

So I use drm_dp_hpd_bridge_register() and drm_aux_hpd_bridge_notify() here, I think it just create a simple hpd bridge to bridge_list.

But drm_connector_oob_hotplug_event() use connector_list instead of bridge_list.



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

Thanks for the clarification.



>
>>
>>
>>>> +	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
>> linux-phy@lists.infradead.org
>> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
Best,
Chaoyi