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

[Dmitry Baryshkov]

On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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,
>