Message-ID: <d970873f-eb76-432e-a881-72b2c5cb2a9c@rock-chips.com>
Date: Tue, 25 Nov 2025 18:54:37 +0800
From: Chaoyi Chen <chaoyi.chen@...k-chips.com>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Chaoyi Chen <kernel@...kyi.com>,
 Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Peter Chen <hzpeterchen@...il.com>, Luca Ceresoli
 <luca.ceresoli@...tlin.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Vinod Koul <vkoul@...nel.org>,
 Kishon Vijay Abraham I <kishon@...nel.org>, Heiko Stuebner
 <heiko@...ech.de>, Sandy Huang <hjc@...k-chips.com>,
 Andy Yan <andy.yan@...k-chips.com>,
 Yubing Zhang <yubing.zhang@...k-chips.com>,
 Frank Wang <frank.wang@...k-chips.com>,
 Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Amit Sunil Dhamne <amitsd@...gle.com>, Dragan Simic <dsimic@...jaro.org>,
 Johan Jonker <jbx6244@...il.com>, Diederik de Haas <didi.debian@...ow.org>,
 Peter Robinson <pbrobinson@...il.com>, linux-usb@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v10 01/11] usb: typec: Add notifier functions

On 11/25/2025 6:06 PM, Heikki Krogerus wrote:
> Tue, Nov 25, 2025 at 10:23:02AM +0800, Chaoyi Chen kirjoitti:
>> On 11/25/2025 12:33 AM, Greg Kroah-Hartman wrote:
>>> On Mon, Nov 24, 2025 at 04:05:53PM +0800, Chaoyi Chen wrote:
>>>> Hi Greg,
>>>>
>>>> On 11/24/2025 3:10 PM, Greg Kroah-Hartman wrote:
>>>>
>>>>> On Mon, Nov 24, 2025 at 09:40:03AM +0800, Chaoyi Chen wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 11/21/2025 10:07 PM, Greg Kroah-Hartman wrote:
>>>>>>> On Thu, Nov 20, 2025 at 10:23:33AM +0800, Chaoyi Chen wrote:
>>>>>>>> From: Chaoyi Chen <chaoyi.chen@...k-chips.com>
>>>>>>>>
>>>>>>>> Some other part of kernel may want to know the event of typec bus.
>>>>>>> Be specific, WHAT part of the kernel will need to know this?
>>>>>> For now, it is DRM.
>>>>> Then say this.
>>>> Okay, please refer to the discussion below.
>>>>
>>>>>>> And why a new notifier, why not just use the existing notifiers that you
>>>>>>> already have?  And what is this going to be used for?
>>>>>> We have discussed this before, but the current bus notifier cannot achieve the expected notification [0].
>>>>>>
>>>>>> [0] https://lore.kernel.org/all/aPsuLREPS_FEV3DS@kuha.fi.intel.com/
>>>>> Then you need to document the heck out of this in the changelog text.
>>>>> But I'm still not quite understanding why the bus notifier does not work
>>>>> here, as you only want this information if the usb device is bound to
>>>>> the bus there, you do not want to know this if it did not complete.
>>>>>
>>>>> That thread says you want this not "too late", but why?  What is the
>>>>> problem there, and how will you handle your code getting loaded after
>>>>> the typec code is loaded?  Notifier callbacks don't work for that
>>>>> situation, right?
>>>> In fact, the typec_register_altmode() function generates two
>>>> registered events. The first one is the registered event of the port
>>>> device, and the second one is the registered event of the partner
>>>> device. The second one event only occurs after a Type-C device is
>>>> inserted.
>>>> The bus notifier event does not actually take effect for the port
>>>> device, because it only sets the bus for the partner device:
>>>>
>>>>      /* The partners are bind to drivers */
>>>>      if (is_typec_partner(parent))
>>>>          alt->adev.dev.bus = &typec_bus;
>>> Setting the bus is correct, then it needs to be registered with the
>>> driver core so the bus link shows up (and a driver is bound to it.)
>>> That is when the bus notifier can happen, right?
>> Yes, this is valid for the partner device. But for the port device, since the bus is not specified here, the corresponding bus notifier will not take effect.
> 
> Perhaps we should just fix this part and make also the port altmodes
> part of the bus.
> 
> Some background, in case this is not clear; the port alternate mode
> devices represent the capability of the ports to support specific
> alternate modes. The partner alternate mode devices do the same
> for the partner devices attached to the ports, but on top of the
> showing the capability, they are also used for the alternate mode
> specific communication using the USB Power Delivery VDM (vendor
> defined messages). That's why only the partner altmodes are bound
> to the generic alternate mode drivers.
> 
> And that's why the hack where the port altmodes are not added to the
> bus. But maybe it's not needed.
> 
> Chaoyi, can you try the attached patch?
> 

Thank you for providing the background information. Maybe this is what
we really want. I will try this in v11 :)

>>>> I hope it's not too late. In fact, the notifier here will notify DRM to establish a bridge chain.
>>> What is a "bridge chain"?
>> In DRM, the bridge chain is often used to describe the chain connection relationship
>> of the output of multi level display conversion chips. The bridge chain we are referring to here
>> is actually a chain  structure formed by connecting various devices using a simple transparent bridge [0].
>>
>> For example, the schematic diagram of a bridge chain is as follows:
>>
>> DP controller bridge -> DP PHY bridge -> onnn,nb7vpq904m retimer bridge -> fsa4480 analog audio switch bridge -> fusb302 HPD bridge
>>
>> Here, apart from the DP controller bridge, the rest are transparent DRM bridges, which are used solely to
>> describe the link relationships between various devices.
>>
>>
>> [0] https://patchwork.freedesktop.org/patch/msgid/20231203114333.1305826-2-dmitry.baryshkov@linaro.org
>>>
>>>> The downstream DP controller driver hopes to obtain the fwnode of the last-level Type-C device
>>>> through this bridge chain to create a DRM connector. And when a device is inserted,
>>>> drivers/usb/typec/altmodes/displayport.c can notify the HPD (Hot Plug Detect) event.
>>> But aren't you just the driver for the "partner device"?
>>>
>>> If not, why isn't a real device being created that you then bind to,
>>> what "fake" type of thing are you attempting to do here that would
>>> require you to do this out-of-band?
>> The HPD event is pass by drm_connector_oob_hotplug_event(), which does not use the device in Type-C.
>> This function will find the corresponding DRM connector device, and the lookup of the DRM connector is
>> done through the fwnode.
>>
>> And the partner device and the port device have the same fwnode.
>>
>>>
>>>> If relying on the second event, the bridge chain may never be established, and the operations of the DP driver will be
>>>> always deferred. Furthermore, other parts of the display controller driver will also be deferred accordingly.
>>> What operations?  What exactly is delayed?  You should not be touching a
>>> device before you have it on your bus, right?
>> To complete the HPD operation, it is necessary to create a drm connector device that
>> has the appropriate fwnode. This operation will be carried out by the DP controller driver.
>>
>> As you can see, since it cross multiple devices, we need to set the fwnode to the last device fusb302.
>> This requires relying on the bridge chain. We can register bridges for multiple devices and then connect
>> them to form a chain. The connection process is completed through drm_bridge_attach().
>>
>> A brief example of the process of establishing a bridge chain is as follows, starting from the last bridge:
>>
>> step1: fusb302 HPD bridge
>> step2: fsa4480 analog audio switch bridge -> fusb302 HPD bridge
>> step3: onnn,nb7vpq904m retimer bridge -> fsa4480 analog audio switch bridge -> fusb302 HPD bridge
>> step4: DP PHY bridge -> onnn,nb7vpq904m retimer bridge -> fsa4480 analog audio switch bridge -> fusb302 HPD bridge
>> step5: DP controller bridge -> DP PHY bridge -> onnn,nb7vpq904m retimer bridge -> fsa4480 analog audio switch bridge -> fusb302 HPD bridge
>>
>> Step 1 is the most crucial, because essentially, regardless of whether we use notifiers or not, what we ultimately want to achieve is to create an HPD bridge.
>> The DP controller needs to wait for the subsequent bridge chain to be established because it needs to know the connection relationships of the devices.
>>
>> The question now is when to create the HPD bridge, during the registration of the port device or during the registration of the partner device.
>> If it's the latter, then the delay occurs here.
>>
>> And I don't think I'm touching the Type-C device here. I'm just using the bridge chain to get a suitable fwnode and create a suitable DRM connector device.
>> The subsequent Type-C HPD events will be on the DRM connector device.
>>
>> This solution is somewhat complex, and I apologize once again for any confusion caused earlier.
>>
>>>
>>>>>>> Notifiers are a pain, and should almost never be added.  Use real
>>>>>>> function calls instead.
>>>>>> In v6, I used direct function calls, but had to switch to notifiers because couldn't resolve the dependencies between DRM and Type-C [1]. Do you have any good ideas? Thank you.
>>>>> Only allow this DRM code to be built if typec code is enabled, do NOT
>>>>> use a select, use a depends in the drm code.
>>>> Sorry, I didn't get your point. Does this mean that the current notifiers approach still needs to be changed to direct function calls?
>>> If you somehow convince me that the existing bus notifiers will not
>>> work, yes :)
>>>
>>>> If so, then based on the previous discussion, typec should not depend
>>>> on any DRM components. Does this mean that we should add the if
>>>> (IS_REACHABLE(CONFIG_DRM_AUX_BRIDGE)) before the direct function call?
>>> No, do it properly like any other function call to another subsystem.
>>>
>>>> Additionally, the current version of CONFIG_DRM_AUX_BRIDGE is selected
>>>> by the DP driver in patch9.
>>> Don't do "select" if at all possible, always try to do "depends on".
>> Thank you for clarifying this. However, CONFIG_DRM_AUX_BRIDGE is not exposed in the menu, and it is not intended for the end user to select it by design. Therefore, I think there still needs to be some place to select it?
> 
> You don't "select TYPEC", you already "depend on TYPEC", so you are
> all set with this one.
> 
> thanks,
> 

Ah, it is.

-- 
Best, 
Chaoyi

Powered by blists - more mailing lists