diff mbox series

[RFC,v1,12/12] usb: typec: qcom: define the bridge's path

Message ID 20230903214150.2877023-13-dmitry.baryshkov@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series drm,usb/typec: uABI for USB-C DisplayPort connectors | expand

Commit Message

Dmitry Baryshkov Sept. 3, 2023, 9:41 p.m. UTC
In order to notify the userspace about the DRM connector's USB-C port,
export the corresponding port's name as the bridge's path field.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
 3 files changed, 14 insertions(+), 7 deletions(-)

Comments

Heikki Krogerus Sept. 15, 2023, 12:14 p.m. UTC | #1
Hi Dmitry,

On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> In order to notify the userspace about the DRM connector's USB-C port,
> export the corresponding port's name as the bridge's path field.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> index b9d4856101c7..452dc6437861 100644
> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	const struct pmic_typec_resources *res;
>  	struct regmap *regmap;
> +	char *tcpm_name;
>  	u32 base[2];
>  	int ret;
>  
> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  	mutex_init(&tcpm->lock);
>  	platform_set_drvdata(pdev, tcpm);
>  
> -	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> -	if (IS_ERR(tcpm->pmic_typec_drm))
> -		return PTR_ERR(tcpm->pmic_typec_drm);
> -
>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
>  	if (!tcpm->tcpc.fwnode)
>  		return -EINVAL;
> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  		goto fwnode_remove;
>  	}
>  
> +	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> +	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);

So I got some questions and concerns off-list. This was one of the
concerns. That tcpm_name is now the actual port device name, so I'm
afraid this is not acceptable.

You can't use device name as a reference, ever. There is no way to
guarantee that a device with a specific name is what you meant it to
be by the time it's accessed.

If you need to deal with a device, then you have to get an actual
reference to it (class_find_device_by_fwnode() should work in this
case).

Ideally you would get the reference in the place where you actually
use it (so drm_connector.c or more likely drm_sysfs.c) but that would
mean a dependency on typec in there, if the component framework or
something like that (device links?) is not an option. You could of
course try to confine the dependency somehow. drm_class does not have
implementation for dev_uevent, so you could take over that as a
temporary solution.

The only way to avoid the dependency completely would be to pass that
device reference from here through your drm bridge chain somehow.
But that's also really fragile. But it could be acceptable as a
temporary solution perhaps, if it's even possible.

Br,
Dmitry Baryshkov Oct. 23, 2023, 6:24 p.m. UTC | #2
On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
>Hi Dmitry,
>
>On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
>> In order to notify the userspace about the DRM connector's USB-C port,
>> export the corresponding port's name as the bridge's path field.
>> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
>>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
>>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
>>  3 files changed, 14 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
>> index b9d4856101c7..452dc6437861 100644
>> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
>> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
>> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>>  	struct device_node *np = dev->of_node;
>>  	const struct pmic_typec_resources *res;
>>  	struct regmap *regmap;
>> +	char *tcpm_name;
>>  	u32 base[2];
>>  	int ret;
>>  
>> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>>  	mutex_init(&tcpm->lock);
>>  	platform_set_drvdata(pdev, tcpm);
>>  
>> -	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
>> -	if (IS_ERR(tcpm->pmic_typec_drm))
>> -		return PTR_ERR(tcpm->pmic_typec_drm);
>> -
>>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
>>  	if (!tcpm->tcpc.fwnode)
>>  		return -EINVAL;
>> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>>  		goto fwnode_remove;
>>  	}
>>  
>> +	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
>> +	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
>
>So I got some questions and concerns off-list. This was one of the
>concerns. That tcpm_name is now the actual port device name, so I'm
>afraid this is not acceptable.
>
>You can't use device name as a reference, ever. There is no way to
>guarantee that a device with a specific name is what you meant it to
>be by the time it's accessed.

Hmm, could you please be more specific, why? I mean, class devices are not that easy to be renamed in sysfs, are they? Or are you concerned about the device being destroyed behind userspace's back? At least for MSM this will be a huge problem already, with the bridge driver suddenly being removed.

>
>If you need to deal with a device, then you have to get an actual
>reference to it (class_find_device_by_fwnode() should work in this
>case).
>
>Ideally you would get the reference in the place where you actually
>use it (so drm_connector.c or more likely drm_sysfs.c) but that would
>mean a dependency on typec in there, if the component framework or
>something like that (device links?) is not an option. You could of
>course try to confine the dependency somehow. drm_class does not have
>implementation for dev_uevent, so you could take over that as a
>temporary solution.
>
>The only way to avoid the dependency completely would be to pass that
>device reference from here through your drm bridge chain somehow.
>But that's also really fragile. But it could be acceptable as a
>temporary solution perhaps, if it's even possible.
>
>Br,
>
Heikki Krogerus Oct. 30, 2023, 8:19 a.m. UTC | #3
On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> >Hi Dmitry,
> >
> >On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> >> In order to notify the userspace about the DRM connector's USB-C port,
> >> export the corresponding port's name as the bridge's path field.
> >> 
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
> >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
> >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
> >>  3 files changed, 14 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> >> index b9d4856101c7..452dc6437861 100644
> >> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> >> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> >> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> >>  	struct device_node *np = dev->of_node;
> >>  	const struct pmic_typec_resources *res;
> >>  	struct regmap *regmap;
> >> +	char *tcpm_name;
> >>  	u32 base[2];
> >>  	int ret;
> >>  
> >> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> >>  	mutex_init(&tcpm->lock);
> >>  	platform_set_drvdata(pdev, tcpm);
> >>  
> >> -	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> >> -	if (IS_ERR(tcpm->pmic_typec_drm))
> >> -		return PTR_ERR(tcpm->pmic_typec_drm);
> >> -
> >>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> >>  	if (!tcpm->tcpc.fwnode)
> >>  		return -EINVAL;
> >> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> >>  		goto fwnode_remove;
> >>  	}
> >>  
> >> +	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> >> +	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> >
> >So I got some questions and concerns off-list. This was one of the
> >concerns. That tcpm_name is now the actual port device name, so I'm
> >afraid this is not acceptable.
> >
> >You can't use device name as a reference, ever. There is no way to
> >guarantee that a device with a specific name is what you meant it to
> >be by the time it's accessed.
> 
> Hmm, could you please be more specific, why? I mean, class devices are not
> that easy to be renamed in sysfs, are they? Or are you concerned about the
> device being destroyed behind userspace's back? At least for MSM this will be
> a huge problem already, with the bridge driver suddenly being removed.

The race exists even in your case, but please do not look at this as a
solution for only your platform.

This is about showing the user space a link between two device
instances (struct device), and the way you do that is by creating a
symlink. That way the kernel can take care of reference counting and
guarantee that the link always points to the correct device. That way
the link will also be always visible in user space without requirement
for any specific ABI like it should.

thanks,
Dmitry Baryshkov Oct. 30, 2023, 9:47 a.m. UTC | #4
On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > >Hi Dmitry,
> > >
> > >On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > >> In order to notify the userspace about the DRM connector's USB-C port,
> > >> export the corresponding port's name as the bridge's path field.
> > >>
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >> ---
> > >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
> > >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
> > >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
> > >>  3 files changed, 14 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > >> index b9d4856101c7..452dc6437861 100644
> > >> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > >> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > >> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > >>    struct device_node *np = dev->of_node;
> > >>    const struct pmic_typec_resources *res;
> > >>    struct regmap *regmap;
> > >> +  char *tcpm_name;
> > >>    u32 base[2];
> > >>    int ret;
> > >>
> > >> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > >>    mutex_init(&tcpm->lock);
> > >>    platform_set_drvdata(pdev, tcpm);
> > >>
> > >> -  tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > >> -  if (IS_ERR(tcpm->pmic_typec_drm))
> > >> -          return PTR_ERR(tcpm->pmic_typec_drm);
> > >> -
> > >>    tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > >>    if (!tcpm->tcpc.fwnode)
> > >>            return -EINVAL;
> > >> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > >>            goto fwnode_remove;
> > >>    }
> > >>
> > >> +  tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > >> +  tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > >
> > >So I got some questions and concerns off-list. This was one of the
> > >concerns. That tcpm_name is now the actual port device name, so I'm
> > >afraid this is not acceptable.
> > >
> > >You can't use device name as a reference, ever. There is no way to
> > >guarantee that a device with a specific name is what you meant it to
> > >be by the time it's accessed.
> >
> > Hmm, could you please be more specific, why? I mean, class devices are not
> > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > device being destroyed behind userspace's back? At least for MSM this will be
> > a huge problem already, with the bridge driver suddenly being removed.
>
> The race exists even in your case, but please do not look at this as a
> solution for only your platform.

Yes!

>
> This is about showing the user space a link between two device
> instances (struct device), and the way you do that is by creating a
> symlink. That way the kernel can take care of reference counting and
> guarantee that the link always points to the correct device. That way
> the link will also be always visible in user space without requirement
> for any specific ABI like it should.

I'm fine with the symlink approach (and I'll follow that up after
finishing the UCSI glue driver rework). However I feel several
deficiencies there:

1) It creates asymmetry with the DP MST case. Do we want to have
symlinks in each of the MST connectors? Or do we follow the PATH
properties in the MST case until we find the root port, which has
symlink? Please note, that fine X11 renames DP MST connectors
internally, so in xrandr I see DP-2-1, which maps to
/sys/class/drm/card0-DP-2. Kind of hard to follow.

2) For the multi-card cases, one has to remap the connector to the
card index + connector path. And this needs to be done by all user
space applications, which would like to present this kind of
information for the user.

3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
and MHL used simple micro-USB connectors) there would be a completely
new uABI. And any external port / wrapper will also require a
completely new symlink kind.

I understand your concerns regarding mentioning external device in the
PATH property. However I think we should make it easier for the
userspace app to determine the kind of the external connector. What
would you think about extending the PATH property in the following
way:

For the USB-C connectors the PATH property has the value of
`typec:cardN-DP-m` value. Userspace app can then look for the
typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
find the information about the corresponding USB-C port.

In future this will allow us to define e.g.:

For the SlimPort / MyDP the PATH property has the value of
`micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
called /sys/class/drm/cardN-DP-m/micro_usb_connector.

Or:

For the SlimPort / MyDP the PATH property has the value of
`mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
/sys/class/drm/cardN-DP-m/mydp_connector.
Simon Ser Oct. 30, 2023, 10:13 a.m. UTC | #5
On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> heikki.krogerus@linux.intel.com wrote:
> 
> > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > 
> > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > 
> > > > Hi Dmitry,
> > > > 
> > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > 
> > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > export the corresponding port's name as the bridge's path field.
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > ---
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > index b9d4856101c7..452dc6437861 100644
> > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > struct device_node *np = dev->of_node;
> > > > > const struct pmic_typec_resources *res;
> > > > > struct regmap *regmap;
> > > > > + char *tcpm_name;
> > > > > u32 base[2];
> > > > > int ret;
> > > > > 
> > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > mutex_init(&tcpm->lock);
> > > > > platform_set_drvdata(pdev, tcpm);
> > > > > 
> > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > -
> > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > if (!tcpm->tcpc.fwnode)
> > > > > return -EINVAL;
> > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > goto fwnode_remove;
> > > > > }
> > > > > 
> > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > 
> > > > So I got some questions and concerns off-list. This was one of the
> > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > afraid this is not acceptable.
> > > > 
> > > > You can't use device name as a reference, ever. There is no way to
> > > > guarantee that a device with a specific name is what you meant it to
> > > > be by the time it's accessed.
> > > 
> > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > device being destroyed behind userspace's back? At least for MSM this will be
> > > a huge problem already, with the bridge driver suddenly being removed.
> > 
> > The race exists even in your case, but please do not look at this as a
> > solution for only your platform.
> 
> Yes!
> 
> > This is about showing the user space a link between two device
> > instances (struct device), and the way you do that is by creating a
> > symlink. That way the kernel can take care of reference counting and
> > guarantee that the link always points to the correct device. That way
> > the link will also be always visible in user space without requirement
> > for any specific ABI like it should.
> 
> I'm fine with the symlink approach (and I'll follow that up after
> finishing the UCSI glue driver rework). However I feel several
> deficiencies there:
> 
> 1) It creates asymmetry with the DP MST case. Do we want to have
> symlinks in each of the MST connectors? Or do we follow the PATH
> properties in the MST case until we find the root port, which has
> symlink? Please note, that fine X11 renames DP MST connectors
> internally, so in xrandr I see DP-2-1, which maps to
> /sys/class/drm/card0-DP-2. Kind of hard to follow.
> 
> 2) For the multi-card cases, one has to remap the connector to the
> card index + connector path. And this needs to be done by all user
> space applications, which would like to present this kind of
> information for the user.
> 
> 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> and MHL used simple micro-USB connectors) there would be a completely
> new uABI. And any external port / wrapper will also require a
> completely new symlink kind.
> 
> I understand your concerns regarding mentioning external device in the
> PATH property. However I think we should make it easier for the
> userspace app to determine the kind of the external connector. What
> would you think about extending the PATH property in the following
> way:
> 
> For the USB-C connectors the PATH property has the value of
> `typec:cardN-DP-m` value. Userspace app can then look for the
> typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> find the information about the corresponding USB-C port.

This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
physical path of the connector. All of the parts of this string are
exposed elsewhere in the KMS uAPI already.

> In future this will allow us to define e.g.:
> 
> For the SlimPort / MyDP the PATH property has the value of
> `micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
> called /sys/class/drm/cardN-DP-m/micro_usb_connector.
> 
> Or:
> 
> For the SlimPort / MyDP the PATH property has the value of
> `mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
> /sys/class/drm/cardN-DP-m/mydp_connector.
> 
> 
> --
> With best wishes
> Dmitry
Dmitry Baryshkov Oct. 30, 2023, 10:22 a.m. UTC | #6
On Mon, 30 Oct 2023 at 12:13, Simon Ser <contact@emersion.fr> wrote:
>
> On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > heikki.krogerus@linux.intel.com wrote:
> >
> > > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > >
> > > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > >
> > > > > Hi Dmitry,
> > > > >
> > > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > >
> > > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > > export the corresponding port's name as the bridge's path field.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > > ---
> > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > index b9d4856101c7..452dc6437861 100644
> > > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > struct device_node *np = dev->of_node;
> > > > > > const struct pmic_typec_resources *res;
> > > > > > struct regmap *regmap;
> > > > > > + char *tcpm_name;
> > > > > > u32 base[2];
> > > > > > int ret;
> > > > > >
> > > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > mutex_init(&tcpm->lock);
> > > > > > platform_set_drvdata(pdev, tcpm);
> > > > > >
> > > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > > -
> > > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > > if (!tcpm->tcpc.fwnode)
> > > > > > return -EINVAL;
> > > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > goto fwnode_remove;
> > > > > > }
> > > > > >
> > > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > >
> > > > > So I got some questions and concerns off-list. This was one of the
> > > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > > afraid this is not acceptable.
> > > > >
> > > > > You can't use device name as a reference, ever. There is no way to
> > > > > guarantee that a device with a specific name is what you meant it to
> > > > > be by the time it's accessed.
> > > >
> > > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > > device being destroyed behind userspace's back? At least for MSM this will be
> > > > a huge problem already, with the bridge driver suddenly being removed.
> > >
> > > The race exists even in your case, but please do not look at this as a
> > > solution for only your platform.
> >
> > Yes!
> >
> > > This is about showing the user space a link between two device
> > > instances (struct device), and the way you do that is by creating a
> > > symlink. That way the kernel can take care of reference counting and
> > > guarantee that the link always points to the correct device. That way
> > > the link will also be always visible in user space without requirement
> > > for any specific ABI like it should.
> >
> > I'm fine with the symlink approach (and I'll follow that up after
> > finishing the UCSI glue driver rework). However I feel several
> > deficiencies there:
> >
> > 1) It creates asymmetry with the DP MST case. Do we want to have
> > symlinks in each of the MST connectors? Or do we follow the PATH
> > properties in the MST case until we find the root port, which has
> > symlink? Please note, that fine X11 renames DP MST connectors
> > internally, so in xrandr I see DP-2-1, which maps to
> > /sys/class/drm/card0-DP-2. Kind of hard to follow.
> >
> > 2) For the multi-card cases, one has to remap the connector to the
> > card index + connector path. And this needs to be done by all user
> > space applications, which would like to present this kind of
> > information for the user.
> >
> > 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> > and MHL used simple micro-USB connectors) there would be a completely
> > new uABI. And any external port / wrapper will also require a
> > completely new symlink kind.
> >
> > I understand your concerns regarding mentioning external device in the
> > PATH property. However I think we should make it easier for the
> > userspace app to determine the kind of the external connector. What
> > would you think about extending the PATH property in the following
> > way:
> >
> > For the USB-C connectors the PATH property has the value of
> > `typec:cardN-DP-m` value. Userspace app can then look for the
> > typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> > find the information about the corresponding USB-C port.
>
> This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
> physical path of the connector. All of the parts of this string are
> exposed elsewhere in the KMS uAPI already.

True. It seems I mixed KMS and xrandr clients in my head.
Just 'typec:' then? This way userspace will still know that it is a
USB-C connector (and can stop there) and if it needs more information
(e.g. physical location) it can further look for the symlink in the
sysfs.

>
> > In future this will allow us to define e.g.:
> >
> > For the SlimPort / MyDP the PATH property has the value of
> > `micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
> > called /sys/class/drm/cardN-DP-m/micro_usb_connector.
> >
> > Or:
> >
> > For the SlimPort / MyDP the PATH property has the value of
> > `mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
> > /sys/class/drm/cardN-DP-m/mydp_connector.
> >
> >
> > --
> > With best wishes
> > Dmitry
Simon Ser Oct. 30, 2023, 10:26 a.m. UTC | #7
On Monday, October 30th, 2023 at 11:22, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, 30 Oct 2023 at 12:13, Simon Ser contact@emersion.fr wrote:
> 
> > On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
> > 
> > > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > > heikki.krogerus@linux.intel.com wrote:
> > > 
> > > > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > > > 
> > > > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > > > 
> > > > > > Hi Dmitry,
> > > > > > 
> > > > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > > > 
> > > > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > > > export the corresponding port's name as the bridge's path field.
> > > > > > > 
> > > > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > > > ---
> > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > index b9d4856101c7..452dc6437861 100644
> > > > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > struct device_node *np = dev->of_node;
> > > > > > > const struct pmic_typec_resources *res;
> > > > > > > struct regmap *regmap;
> > > > > > > + char *tcpm_name;
> > > > > > > u32 base[2];
> > > > > > > int ret;
> > > > > > > 
> > > > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > mutex_init(&tcpm->lock);
> > > > > > > platform_set_drvdata(pdev, tcpm);
> > > > > > > 
> > > > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > > > -
> > > > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > > > if (!tcpm->tcpc.fwnode)
> > > > > > > return -EINVAL;
> > > > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > goto fwnode_remove;
> > > > > > > }
> > > > > > > 
> > > > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > > > 
> > > > > > So I got some questions and concerns off-list. This was one of the
> > > > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > > > afraid this is not acceptable.
> > > > > > 
> > > > > > You can't use device name as a reference, ever. There is no way to
> > > > > > guarantee that a device with a specific name is what you meant it to
> > > > > > be by the time it's accessed.
> > > > > 
> > > > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > > > device being destroyed behind userspace's back? At least for MSM this will be
> > > > > a huge problem already, with the bridge driver suddenly being removed.
> > > > 
> > > > The race exists even in your case, but please do not look at this as a
> > > > solution for only your platform.
> > > 
> > > Yes!
> > > 
> > > > This is about showing the user space a link between two device
> > > > instances (struct device), and the way you do that is by creating a
> > > > symlink. That way the kernel can take care of reference counting and
> > > > guarantee that the link always points to the correct device. That way
> > > > the link will also be always visible in user space without requirement
> > > > for any specific ABI like it should.
> > > 
> > > I'm fine with the symlink approach (and I'll follow that up after
> > > finishing the UCSI glue driver rework). However I feel several
> > > deficiencies there:
> > > 
> > > 1) It creates asymmetry with the DP MST case. Do we want to have
> > > symlinks in each of the MST connectors? Or do we follow the PATH
> > > properties in the MST case until we find the root port, which has
> > > symlink? Please note, that fine X11 renames DP MST connectors
> > > internally, so in xrandr I see DP-2-1, which maps to
> > > /sys/class/drm/card0-DP-2. Kind of hard to follow.
> > > 
> > > 2) For the multi-card cases, one has to remap the connector to the
> > > card index + connector path. And this needs to be done by all user
> > > space applications, which would like to present this kind of
> > > information for the user.
> > > 
> > > 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> > > and MHL used simple micro-USB connectors) there would be a completely
> > > new uABI. And any external port / wrapper will also require a
> > > completely new symlink kind.
> > > 
> > > I understand your concerns regarding mentioning external device in the
> > > PATH property. However I think we should make it easier for the
> > > userspace app to determine the kind of the external connector. What
> > > would you think about extending the PATH property in the following
> > > way:
> > > 
> > > For the USB-C connectors the PATH property has the value of
> > > `typec:cardN-DP-m` value. Userspace app can then look for the
> > > typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> > > find the information about the corresponding USB-C port.
> > 
> > This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
> > physical path of the connector. All of the parts of this string are
> > exposed elsewhere in the KMS uAPI already.
> 
> True. It seems I mixed KMS and xrandr clients in my head.
> Just 'typec:' then? This way userspace will still know that it is a
> USB-C connector (and can stop there) and if it needs more information
> (e.g. physical location) it can further look for the symlink in the
> sysfs.

It sounds like an abuse of the PATH property. PATH is supposed to
contain an actual path, not just some connector type.

User-space can directly look into sysfs if it wants to figure out
whether a connector is typec.
Dmitry Baryshkov Oct. 30, 2023, 12:12 p.m. UTC | #8
On Mon, 30 Oct 2023 at 12:27, Simon Ser <contact@emersion.fr> wrote:
>
> On Monday, October 30th, 2023 at 11:22, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On Mon, 30 Oct 2023 at 12:13, Simon Ser contact@emersion.fr wrote:
> >
> > > On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
> > >
> > > > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > > > heikki.krogerus@linux.intel.com wrote:
> > > >
> > > > > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > > > >
> > > > > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > > > >
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > > > >
> > > > > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > > > > export the corresponding port's name as the bridge's path field.
> > > > > > > >
> > > > > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > > > > ---
> > > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > > index b9d4856101c7..452dc6437861 100644
> > > > > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > > struct device_node *np = dev->of_node;
> > > > > > > > const struct pmic_typec_resources *res;
> > > > > > > > struct regmap *regmap;
> > > > > > > > + char *tcpm_name;
> > > > > > > > u32 base[2];
> > > > > > > > int ret;
> > > > > > > >
> > > > > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > > mutex_init(&tcpm->lock);
> > > > > > > > platform_set_drvdata(pdev, tcpm);
> > > > > > > >
> > > > > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > > > > -
> > > > > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > > > > if (!tcpm->tcpc.fwnode)
> > > > > > > > return -EINVAL;
> > > > > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > > goto fwnode_remove;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > > > >
> > > > > > > So I got some questions and concerns off-list. This was one of the
> > > > > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > > > > afraid this is not acceptable.
> > > > > > >
> > > > > > > You can't use device name as a reference, ever. There is no way to
> > > > > > > guarantee that a device with a specific name is what you meant it to
> > > > > > > be by the time it's accessed.
> > > > > >
> > > > > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > > > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > > > > device being destroyed behind userspace's back? At least for MSM this will be
> > > > > > a huge problem already, with the bridge driver suddenly being removed.
> > > > >
> > > > > The race exists even in your case, but please do not look at this as a
> > > > > solution for only your platform.
> > > >
> > > > Yes!
> > > >
> > > > > This is about showing the user space a link between two device
> > > > > instances (struct device), and the way you do that is by creating a
> > > > > symlink. That way the kernel can take care of reference counting and
> > > > > guarantee that the link always points to the correct device. That way
> > > > > the link will also be always visible in user space without requirement
> > > > > for any specific ABI like it should.
> > > >
> > > > I'm fine with the symlink approach (and I'll follow that up after
> > > > finishing the UCSI glue driver rework). However I feel several
> > > > deficiencies there:
> > > >
> > > > 1) It creates asymmetry with the DP MST case. Do we want to have
> > > > symlinks in each of the MST connectors? Or do we follow the PATH
> > > > properties in the MST case until we find the root port, which has
> > > > symlink? Please note, that fine X11 renames DP MST connectors
> > > > internally, so in xrandr I see DP-2-1, which maps to
> > > > /sys/class/drm/card0-DP-2. Kind of hard to follow.
> > > >
> > > > 2) For the multi-card cases, one has to remap the connector to the
> > > > card index + connector path. And this needs to be done by all user
> > > > space applications, which would like to present this kind of
> > > > information for the user.
> > > >
> > > > 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> > > > and MHL used simple micro-USB connectors) there would be a completely
> > > > new uABI. And any external port / wrapper will also require a
> > > > completely new symlink kind.
> > > >
> > > > I understand your concerns regarding mentioning external device in the
> > > > PATH property. However I think we should make it easier for the
> > > > userspace app to determine the kind of the external connector. What
> > > > would you think about extending the PATH property in the following
> > > > way:
> > > >
> > > > For the USB-C connectors the PATH property has the value of
> > > > `typec:cardN-DP-m` value. Userspace app can then look for the
> > > > typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> > > > find the information about the corresponding USB-C port.
> > >
> > > This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
> > > physical path of the connector. All of the parts of this string are
> > > exposed elsewhere in the KMS uAPI already.
> >
> > True. It seems I mixed KMS and xrandr clients in my head.
> > Just 'typec:' then? This way userspace will still know that it is a
> > USB-C connector (and can stop there) and if it needs more information
> > (e.g. physical location) it can further look for the symlink in the
> > sysfs.
>
> It sounds like an abuse of the PATH property. PATH is supposed to
> contain an actual path, not just some connector type.
>
> User-space can directly look into sysfs if it wants to figure out
> whether a connector is typec.

typec:N, where N is an id of the port?
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index b9d4856101c7..452dc6437861 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -156,6 +156,7 @@  static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	const struct pmic_typec_resources *res;
 	struct regmap *regmap;
+	char *tcpm_name;
 	u32 base[2];
 	int ret;
 
@@ -211,10 +212,6 @@  static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	mutex_init(&tcpm->lock);
 	platform_set_drvdata(pdev, tcpm);
 
-	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
-	if (IS_ERR(tcpm->pmic_typec_drm))
-		return PTR_ERR(tcpm->pmic_typec_drm);
-
 	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
 	if (!tcpm->tcpc.fwnode)
 		return -EINVAL;
@@ -225,6 +222,12 @@  static int qcom_pmic_typec_probe(struct platform_device *pdev)
 		goto fwnode_remove;
 	}
 
+	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
+	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
+	kfree(tcpm_name);
+	if (IS_ERR(tcpm->pmic_typec_drm))
+		return PTR_ERR(tcpm->pmic_typec_drm);
+
 	ret = qcom_pmic_typec_port_start(tcpm->pmic_typec_port,
 					 tcpm->tcpm_port);
 	if (ret)
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
index e202eb7b52db..7bd7f4bf3539 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
@@ -21,7 +21,8 @@  static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
 	.attach = qcom_pmic_typec_attach,
 };
 
-struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
+struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev,
+						const char *path)
 {
 	struct pmic_typec_drm *tcpm_drm;
 
@@ -33,6 +34,7 @@  struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
 	tcpm_drm->bridge.of_node = of_get_child_by_name(dev->of_node, "connector");
 	tcpm_drm->bridge.ops = DRM_BRIDGE_OP_HPD;
 	tcpm_drm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+	tcpm_drm->bridge.path = devm_kstrdup(dev, path, GFP_KERNEL);
 
 	return ERR_PTR(devm_drm_bridge_add(dev, &tcpm_drm->bridge));
 }
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
index 01f4bb71346b..259c047265f7 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
@@ -9,9 +9,11 @@ 
 struct pmic_typec_drm;
 
 #if IS_ENABLED(CONFIG_DRM)
-struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev);
+struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev,
+						const char *path);
 #else
-static inline pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
+static inline pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev,
+						       const char *path)
 {
 	return NULL;
 }