diff mbox series

[1/2] phy: qcom: phy-qcom-snps-eusb2: Make repeater optional

Message ID 20250318-xps13-fingerprint-v1-1-fbb02d5a34a7@oss.qualcomm.com
State New
Headers show
Series arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor | expand

Commit Message

Bjorn Andersson via B4 Relay March 19, 2025, 3:22 a.m. UTC 
From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

In a multiport configuration based on the SNPS eUSB2 PHY it's not
necessary that all ports are connected to something.

While this is allowed by the Devicetree binding, the implementation
current fails probing for such PHYs, which also prevents the multiport
controller from probing.

The lack of repeater does not alter the fact that the PHY is there and
attempts at describing only the used PHYs in Devicetree results in
failures to initialize the USB controller.

Make the repeater optional, to allow the these PHYs to be described in
the DeviceTree and for the associated multiport controller to operate
the other ports.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Dmitry Baryshkov March 19, 2025, 10:20 a.m. UTC | #1 
On Tue, Mar 18, 2025 at 10:22:56PM -0500, Bjorn Andersson via B4 Relay wrote:
> From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> In a multiport configuration based on the SNPS eUSB2 PHY it's not
> necessary that all ports are connected to something.
> 
> While this is allowed by the Devicetree binding, the implementation
> current fails probing for such PHYs, which also prevents the multiport
> controller from probing.
> 
> The lack of repeater does not alter the fact that the PHY is there and
> attempts at describing only the used PHYs in Devicetree results in
> failures to initialize the USB controller.
> 
> Make the repeater optional, to allow the these PHYs to be described in
> the DeviceTree and for the associated multiport controller to operate
> the other ports.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> index 1484691a41d59a7eaf257ef44300827c668bf7e0..8897d2072ccfcaa5b4a510c17761dcdeed5bad0f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> @@ -401,9 +401,13 @@ static int qcom_snps_eusb2_hsphy_probe(struct platform_device *pdev)
>  				     "failed to get regulator supplies\n");
>  
>  	phy->repeater = devm_of_phy_get_by_index(dev, np, 0);
> -	if (IS_ERR(phy->repeater))
> -		return dev_err_probe(dev, PTR_ERR(phy->repeater),
> -				     "failed to get repeater\n");
> +	if (IS_ERR(phy->repeater)) {
> +		if (PTR_ERR(phy->repeater) == -ENODEV)
> +			phy->repeater = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(phy->repeater),
> +					     "failed to get repeater\n");

Can you use devm_of_phy_optional_get() or devm_phy_optional_get()
instead?

> +	}
>  
>  	generic_phy = devm_phy_create(dev, NULL, &qcom_snps_eusb2_hsphy_ops);
>  	if (IS_ERR(generic_phy)) {
> 
> -- 
> 2.48.1
> 
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy
Stephan Gerhold March 19, 2025, 10:35 a.m. UTC | #2 
On Wed, Mar 19, 2025 at 12:20:07PM +0200, Dmitry Baryshkov wrote:
> On Tue, Mar 18, 2025 at 10:22:56PM -0500, Bjorn Andersson via B4 Relay wrote:
> > From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > 
> > In a multiport configuration based on the SNPS eUSB2 PHY it's not
> > necessary that all ports are connected to something.
> > 
> > While this is allowed by the Devicetree binding, the implementation
> > current fails probing for such PHYs, which also prevents the multiport
> > controller from probing.
> > 
> > The lack of repeater does not alter the fact that the PHY is there and
> > attempts at describing only the used PHYs in Devicetree results in
> > failures to initialize the USB controller.
> > 
> > Make the repeater optional, to allow the these PHYs to be described in
> > the DeviceTree and for the associated multiport controller to operate
> > the other ports.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> > index 1484691a41d59a7eaf257ef44300827c668bf7e0..8897d2072ccfcaa5b4a510c17761dcdeed5bad0f 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> > @@ -401,9 +401,13 @@ static int qcom_snps_eusb2_hsphy_probe(struct platform_device *pdev)
> >  				     "failed to get regulator supplies\n");
> >  
> >  	phy->repeater = devm_of_phy_get_by_index(dev, np, 0);
> > -	if (IS_ERR(phy->repeater))
> > -		return dev_err_probe(dev, PTR_ERR(phy->repeater),
> > -				     "failed to get repeater\n");
> > +	if (IS_ERR(phy->repeater)) {
> > +		if (PTR_ERR(phy->repeater) == -ENODEV)
> > +			phy->repeater = NULL;
> > +		else
> > +			return dev_err_probe(dev, PTR_ERR(phy->repeater),
> > +					     "failed to get repeater\n");
> 
> Can you use devm_of_phy_optional_get() or devm_phy_optional_get()
> instead?
> 

There is such a patch from Ivaylo already [1].

@Ivaylo: Are you planning to re-spin that patch set? Might be even worth
putting that patch first / sending it separately, since Neil pointed out
there that the bindings already have the repeater as non-required.

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20250223122227.725233-6-ivo.ivanov.ivanov1@gmail.com/
Dmitry Baryshkov March 19, 2025, 11:07 a.m. UTC | #3 
On Wed, Mar 19, 2025 at 11:35:00AM +0100, Stephan Gerhold wrote:
> On Wed, Mar 19, 2025 at 12:20:07PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Mar 18, 2025 at 10:22:56PM -0500, Bjorn Andersson via B4 Relay wrote:
> > > From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > 
> > > In a multiport configuration based on the SNPS eUSB2 PHY it's not
> > > necessary that all ports are connected to something.
> > > 
> > > While this is allowed by the Devicetree binding, the implementation
> > > current fails probing for such PHYs, which also prevents the multiport
> > > controller from probing.
> > > 
> > > The lack of repeater does not alter the fact that the PHY is there and
> > > attempts at describing only the used PHYs in Devicetree results in
> > > failures to initialize the USB controller.
> > > 
> > > Make the repeater optional, to allow the these PHYs to be described in
> > > the DeviceTree and for the associated multiport controller to operate
> > > the other ports.
> > > 
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> > > index 1484691a41d59a7eaf257ef44300827c668bf7e0..8897d2072ccfcaa5b4a510c17761dcdeed5bad0f 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
> > > @@ -401,9 +401,13 @@ static int qcom_snps_eusb2_hsphy_probe(struct platform_device *pdev)
> > >  				     "failed to get regulator supplies\n");
> > >  
> > >  	phy->repeater = devm_of_phy_get_by_index(dev, np, 0);
> > > -	if (IS_ERR(phy->repeater))
> > > -		return dev_err_probe(dev, PTR_ERR(phy->repeater),
> > > -				     "failed to get repeater\n");
> > > +	if (IS_ERR(phy->repeater)) {
> > > +		if (PTR_ERR(phy->repeater) == -ENODEV)
> > > +			phy->repeater = NULL;
> > > +		else
> > > +			return dev_err_probe(dev, PTR_ERR(phy->repeater),
> > > +					     "failed to get repeater\n");
> > 
> > Can you use devm_of_phy_optional_get() or devm_phy_optional_get()
> > instead?
> > 
> 
> There is such a patch from Ivaylo already [1].

I will respond there.

> 
> @Ivaylo: Are you planning to re-spin that patch set? Might be even worth
> putting that patch first / sending it separately, since Neil pointed out
> there that the bindings already have the repeater as non-required.
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20250223122227.725233-6-ivo.ivanov.ivanov1@gmail.com/
Ivaylo Ivanov March 19, 2025, 11:41 a.m. UTC | #4 
On 3/19/25 12:35, Stephan Gerhold wrote:
> On Wed, Mar 19, 2025 at 12:20:07PM +0200, Dmitry Baryshkov wrote:
>> On Tue, Mar 18, 2025 at 10:22:56PM -0500, Bjorn Andersson via B4 Relay wrote:
>>> From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>>>
>>> In a multiport configuration based on the SNPS eUSB2 PHY it's not
>>> necessary that all ports are connected to something.
>>>
>>> While this is allowed by the Devicetree binding, the implementation
>>> current fails probing for such PHYs, which also prevents the multiport
>>> controller from probing.
>>>
>>> The lack of repeater does not alter the fact that the PHY is there and
>>> attempts at describing only the used PHYs in Devicetree results in
>>> failures to initialize the USB controller.
>>>
>>> Make the repeater optional, to allow the these PHYs to be described in
>>> the DeviceTree and for the associated multiport controller to operate
>>> the other ports.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>>> ---
>>>  drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
>>> index 1484691a41d59a7eaf257ef44300827c668bf7e0..8897d2072ccfcaa5b4a510c17761dcdeed5bad0f 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
>>> @@ -401,9 +401,13 @@ static int qcom_snps_eusb2_hsphy_probe(struct platform_device *pdev)
>>>  				     "failed to get regulator supplies\n");
>>>  
>>>  	phy->repeater = devm_of_phy_get_by_index(dev, np, 0);
>>> -	if (IS_ERR(phy->repeater))
>>> -		return dev_err_probe(dev, PTR_ERR(phy->repeater),
>>> -				     "failed to get repeater\n");
>>> +	if (IS_ERR(phy->repeater)) {
>>> +		if (PTR_ERR(phy->repeater) == -ENODEV)
>>> +			phy->repeater = NULL;
>>> +		else
>>> +			return dev_err_probe(dev, PTR_ERR(phy->repeater),
>>> +					     "failed to get repeater\n");
>> Can you use devm_of_phy_optional_get() or devm_phy_optional_get()
>> instead?
>>
> There is such a patch from Ivaylo already [1].
>
> @Ivaylo: Are you planning to re-spin that patch set?

Yes. I've spent the past week digging deeper into how my hardware works,
as well as improving the patchset.

>  Might be even worth
> putting that patch first / sending it separately, since Neil pointed out
> there that the bindings already have the repeater as non-required.

That's going to be... quite a bit of work. I have around 6-7 patches for this
driver alone, including moving the whole driver to ../, so moving this patch
to the front will be annoying.

Best regards,
Ivaylo

>
> Thanks,
> Stephan
>
> [1]: https://lore.kernel.org/linux-arm-msm/20250223122227.725233-6-ivo.ivanov.ivanov1@gmail.com/
diff mbox series

Patch

diff --git a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
index 1484691a41d59a7eaf257ef44300827c668bf7e0..8897d2072ccfcaa5b4a510c17761dcdeed5bad0f 100644
--- a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c
@@ -401,9 +401,13 @@  static int qcom_snps_eusb2_hsphy_probe(struct platform_device *pdev)
 				     "failed to get regulator supplies\n");
 
 	phy->repeater = devm_of_phy_get_by_index(dev, np, 0);
-	if (IS_ERR(phy->repeater))
-		return dev_err_probe(dev, PTR_ERR(phy->repeater),
-				     "failed to get repeater\n");
+	if (IS_ERR(phy->repeater)) {
+		if (PTR_ERR(phy->repeater) == -ENODEV)
+			phy->repeater = NULL;
+		else
+			return dev_err_probe(dev, PTR_ERR(phy->repeater),
+					     "failed to get repeater\n");
+	}
 
 	generic_phy = devm_phy_create(dev, NULL, &qcom_snps_eusb2_hsphy_ops);
 	if (IS_ERR(generic_phy)) {