[PATCH 0/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[PATCH 0/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Bjorn Andersson via B4 Relay]

Make the repeater optional in the SNSP eUSB2 PHY driver and describe the
involved components for enabling the fingerprint reader in the Dell
XPS13.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
Bjorn Andersson (2):
      phy: qcom: phy-qcom-snps-eusb2: Make repeater optional
      arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

 .../boot/dts/qcom/x1e80100-dell-xps13-9345.dts     | 59 +++++++++++++++++++++-
 drivers/phy/qualcomm/phy-qcom-snps-eusb2.c         | 10 ++--
 2 files changed, 64 insertions(+), 5 deletions(-)
---
base-commit: c4d4884b67802c41fd67399747165d65c770621a
change-id: 20250318-xps13-fingerprint-a7fbe2792c24

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

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

[Bjorn Andersson via B4 Relay]

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");
+	}
 
 	generic_phy = devm_phy_create(dev, NULL, &qcom_snps_eusb2_hsphy_ops);
 	if (IS_ERR(generic_phy)) {

-- 
2.48.1

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Stephan Gerhold]

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/

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

[Dmitry Baryshkov]

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/

-- 
With best wishes
Dmitry

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

[Ivaylo Ivanov]

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/

[PATCH 2/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Bjorn Andersson via B4 Relay]

From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

The fingerprint sensor, hidden in the power button, is connected to one
of the USB multiport ports; while the other port is unused.

Describe the USB controller, the four phys and the repeater involved to
make the fingerprint sensor operational.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 .../boot/dts/qcom/x1e80100-dell-xps13-9345.dts     | 59 +++++++++++++++++++++-
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
index 967f6dba0878b51a985fd7c9570b8c4e71afe57d..a35557c562d771e2ce209fca05b82c1943d70f63 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
@@ -744,8 +744,21 @@ touchscreen@10 {
 
 &i2c9 {
 	clock-frequency = <400000>;
-	status = "disabled";
-	/* USB3 retimer device @0x4f */
+	status = "okay";
+
+	eusb6_repeater: redriver@4f {
+		compatible = "nxp,ptn3222";
+		reg = <0x4f>;
+		#phy-cells = <0>;
+
+		vdd3v3-supply = <&vreg_l13b_3p0>;
+		vdd1v8-supply = <&vreg_l4b_1p8>;
+
+		reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
+
+		pinctrl-0 = <&eusb6_reset_n>;
+		pinctrl-names = "default";
+	};
 };
 
 &i2c17 {
@@ -967,6 +980,14 @@ edp_reg_en: edp-reg-en-state {
 		bias-disable;
 	};
 
+	eusb6_reset_n: eusb6-reset-n-state {
+		pins = "gpio184";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+		output-low;
+	};
+
 	hall_int_n_default: hall-int-n-state {
 		pins = "gpio92";
 		function = "gpio";
@@ -1172,3 +1193,37 @@ &usb_1_ss1_dwc3_hs {
 &usb_1_ss1_qmpphy_out {
 	remote-endpoint = <&retimer_ss1_ss_in>;
 };
+
+&usb_mp {
+	status = "okay";
+};
+
+&usb_mp_hsphy0 {
+	vdd-supply = <&vreg_l2e_0p8>;
+	vdda12-supply = <&vreg_l3e_1p2>;
+
+	phys = <&eusb6_repeater>;
+
+	status = "okay";
+};
+
+&usb_mp_hsphy1 {
+	vdd-supply = <&vreg_l2e_0p8>;
+	vdda12-supply = <&vreg_l3e_1p2>;
+
+	status = "okay";
+};
+
+&usb_mp_qmpphy0 {
+	vdda-phy-supply = <&vreg_l3e_1p2>;
+	vdda-pll-supply = <&vreg_l3c_0p9>;
+
+	status = "okay";
+};
+
+&usb_mp_qmpphy1 {
+	vdda-phy-supply = <&vreg_l3e_1p2>;
+	vdda-pll-supply = <&vreg_l3c_0p9>;
+
+	status = "okay";
+};

-- 
2.48.1

Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Konrad Dybcio]

On 3/19/25 4:22 AM, Bjorn Andersson via B4 Relay wrote:
> From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> The fingerprint sensor, hidden in the power button, is connected to one
> of the USB multiport ports; while the other port is unused.
> 
> Describe the USB controller, the four phys and the repeater involved to
> make the fingerprint sensor operational.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Aleksandrs Vinarskis]

On Wed, 19 Mar 2025 at 04:22, Bjorn Andersson via B4 Relay
<devnull+bjorn.andersson.oss.qualcomm.com@kernel.org> wrote:
>
> From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>
> The fingerprint sensor, hidden in the power button, is connected to one
> of the USB multiport ports; while the other port is unused.
>
> Describe the USB controller, the four phys and the repeater involved to
> make the fingerprint sensor operational.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---

Thanks for getting to the bottom of this, it was certainly a long
awaited feature :)

>  .../boot/dts/qcom/x1e80100-dell-xps13-9345.dts     | 59 +++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> index 967f6dba0878b51a985fd7c9570b8c4e71afe57d..a35557c562d771e2ce209fca05b82c1943d70f63 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> @@ -744,8 +744,21 @@ touchscreen@10 {
>
>  &i2c9 {
>         clock-frequency = <400000>;
> -       status = "disabled";
> -       /* USB3 retimer device @0x4f */
> +       status = "okay";
> +
> +       eusb6_repeater: redriver@4f {
> +               compatible = "nxp,ptn3222";
> +               reg = <0x4f>;
> +               #phy-cells = <0>;
> +
> +               vdd3v3-supply = <&vreg_l13b_3p0>;
> +               vdd1v8-supply = <&vreg_l4b_1p8>;
> +
> +               reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> +
> +               pinctrl-0 = <&eusb6_reset_n>;
> +               pinctrl-names = "default";
> +       };
>  };
>
>  &i2c17 {
> @@ -967,6 +980,14 @@ edp_reg_en: edp-reg-en-state {
>                 bias-disable;
>         };
>
> +       eusb6_reset_n: eusb6-reset-n-state {
> +               pins = "gpio184";
> +               function = "gpio";
> +               drive-strength = <2>;
> +               bias-disable;
> +               output-low;
> +       };
> +
>         hall_int_n_default: hall-int-n-state {
>                 pins = "gpio92";
>                 function = "gpio";
> @@ -1172,3 +1193,37 @@ &usb_1_ss1_dwc3_hs {
>  &usb_1_ss1_qmpphy_out {
>         remote-endpoint = <&retimer_ss1_ss_in>;
>  };
> +
> +&usb_mp {
> +       status = "okay";
> +};
> +
> +&usb_mp_hsphy0 {
> +       vdd-supply = <&vreg_l2e_0p8>;
> +       vdda12-supply = <&vreg_l3e_1p2>;
> +
> +       phys = <&eusb6_repeater>;

I was under the impression that the fingerprint reader is on the 2nd
port of the root hub, as:
* In ACPI, the only USB device of MP is listed under PRT1, PRT0 is empty
* On Windows the device is listed as PORT2...HUB1...
* `lsusb -t` for the device gives `Port 002: Dev 002,...12M`

Do `usb_mp_hsphy0` and `usb_mp_hsphy1` translate to port 1 and 2
respectively? Because if yes, repeater may belong to `usb_mp_hsphy1`
instead?

Current series works. Moving `phys = <&eusb6_repeater>;` to
`usb_mp_hsphy1` also works, I'm assuming because we are not actually
disabling unused phys.

Tested-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>


> +
> +       status = "okay";
> +};
> +
> +&usb_mp_hsphy1 {
> +       vdd-supply = <&vreg_l2e_0p8>;
> +       vdda12-supply = <&vreg_l3e_1p2>;
> +
> +       status = "okay";
> +};
> +
> +&usb_mp_qmpphy0 {
> +       vdda-phy-supply = <&vreg_l3e_1p2>;
> +       vdda-pll-supply = <&vreg_l3c_0p9>;
> +
> +       status = "okay";
> +};
> +
> +&usb_mp_qmpphy1 {
> +       vdda-phy-supply = <&vreg_l3e_1p2>;
> +       vdda-pll-supply = <&vreg_l3c_0p9>;
> +
> +       status = "okay";
> +};
>
> --
> 2.48.1
>
>

Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Bjorn Andersson]

On Wed, Mar 19, 2025 at 04:05:41PM +0100, Aleksandrs Vinarskis wrote:
> On Wed, 19 Mar 2025 at 04:22, Bjorn Andersson via B4 Relay
> <devnull+bjorn.andersson.oss.qualcomm.com@kernel.org> wrote:
> >
> > From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> >
> > The fingerprint sensor, hidden in the power button, is connected to one
> > of the USB multiport ports; while the other port is unused.
> >
> > Describe the USB controller, the four phys and the repeater involved to
> > make the fingerprint sensor operational.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> 
> Thanks for getting to the bottom of this, it was certainly a long
> awaited feature :)
> 

Didn't think it was something I wanted, but now that it's working I
proved myself wrong ;)

> >  .../boot/dts/qcom/x1e80100-dell-xps13-9345.dts     | 59 +++++++++++++++++++++-
> >  1 file changed, 57 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> > index 967f6dba0878b51a985fd7c9570b8c4e71afe57d..a35557c562d771e2ce209fca05b82c1943d70f63 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> > @@ -744,8 +744,21 @@ touchscreen@10 {
> >
> >  &i2c9 {
> >         clock-frequency = <400000>;
> > -       status = "disabled";
> > -       /* USB3 retimer device @0x4f */
> > +       status = "okay";
> > +
> > +       eusb6_repeater: redriver@4f {
> > +               compatible = "nxp,ptn3222";
> > +               reg = <0x4f>;
> > +               #phy-cells = <0>;
> > +
> > +               vdd3v3-supply = <&vreg_l13b_3p0>;
> > +               vdd1v8-supply = <&vreg_l4b_1p8>;
> > +
> > +               reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> > +
> > +               pinctrl-0 = <&eusb6_reset_n>;
> > +               pinctrl-names = "default";
> > +       };
> >  };
> >
> >  &i2c17 {
> > @@ -967,6 +980,14 @@ edp_reg_en: edp-reg-en-state {
> >                 bias-disable;
> >         };
> >
> > +       eusb6_reset_n: eusb6-reset-n-state {
> > +               pins = "gpio184";
> > +               function = "gpio";
> > +               drive-strength = <2>;
> > +               bias-disable;
> > +               output-low;
> > +       };
> > +
> >         hall_int_n_default: hall-int-n-state {
> >                 pins = "gpio92";
> >                 function = "gpio";
> > @@ -1172,3 +1193,37 @@ &usb_1_ss1_dwc3_hs {
> >  &usb_1_ss1_qmpphy_out {
> >         remote-endpoint = <&retimer_ss1_ss_in>;
> >  };
> > +
> > +&usb_mp {
> > +       status = "okay";
> > +};
> > +
> > +&usb_mp_hsphy0 {
> > +       vdd-supply = <&vreg_l2e_0p8>;
> > +       vdda12-supply = <&vreg_l3e_1p2>;
> > +
> > +       phys = <&eusb6_repeater>;
> 
> I was under the impression that the fingerprint reader is on the 2nd
> port of the root hub, as:
> * In ACPI, the only USB device of MP is listed under PRT1, PRT0 is empty
> * On Windows the device is listed as PORT2...HUB1...
> * `lsusb -t` for the device gives `Port 002: Dev 002,...12M`
> 
> Do `usb_mp_hsphy0` and `usb_mp_hsphy1` translate to port 1 and 2
> respectively? Because if yes, repeater may belong to `usb_mp_hsphy1`
> instead?
> 

That would be more logical, I'll dig up some documentation for the
SoC and see if I can better understand the naming of these instances.

> Current series works. Moving `phys = <&eusb6_repeater>;` to
> `usb_mp_hsphy1` also works, I'm assuming because we are not actually
> disabling unused phys.
> 

While not being used for any communication, the PHY is there and the
multiport controller seems to need them both to be present. Further
regardless of something being connected to the PHY, it's still there, so
it seems correct to represent it in the dtsi. That said, I didn't dig
deeper into the exact details here.

> Tested-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> 

Thanks.

Regards,
Bjorn

> 
> > +
> > +       status = "okay";
> > +};
> > +
> > +&usb_mp_hsphy1 {
> > +       vdd-supply = <&vreg_l2e_0p8>;
> > +       vdda12-supply = <&vreg_l3e_1p2>;
> > +
> > +       status = "okay";
> > +};
> > +
> > +&usb_mp_qmpphy0 {
> > +       vdda-phy-supply = <&vreg_l3e_1p2>;
> > +       vdda-pll-supply = <&vreg_l3c_0p9>;
> > +
> > +       status = "okay";
> > +};
> > +
> > +&usb_mp_qmpphy1 {
> > +       vdda-phy-supply = <&vreg_l3e_1p2>;
> > +       vdda-pll-supply = <&vreg_l3c_0p9>;
> > +
> > +       status = "okay";
> > +};
> >
> > --
> > 2.48.1
> >
> >

Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Bryan O'Donoghue]

On 19/03/2025 03:22, Bjorn Andersson via B4 Relay wrote:
> From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> The fingerprint sensor, hidden in the power button, is connected to one
> of the USB multiport ports; while the other port is unused.
> 
> Describe the USB controller, the four phys and the repeater involved to
> make the fingerprint sensor operational.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>   .../boot/dts/qcom/x1e80100-dell-xps13-9345.dts     | 59 +++++++++++++++++++++-
>   1 file changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> index 967f6dba0878b51a985fd7c9570b8c4e71afe57d..a35557c562d771e2ce209fca05b82c1943d70f63 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> @@ -744,8 +744,21 @@ touchscreen@10 {
> 
>   &i2c9 {
>   	clock-frequency = <400000>;
> -	status = "disabled";
> -	/* USB3 retimer device @0x4f */
> +	status = "okay";
> +
> +	eusb6_repeater: redriver@4f {

How about

eusb6_frp_repeater: redriver@4f

leading to> +		compatible = "nxp,ptn3222";
> +		reg = <0x4f>;
> +		#phy-cells = <0>;
> +
> +		vdd3v3-supply = <&vreg_l13b_3p0>;
> +		vdd1v8-supply = <&vreg_l4b_1p8>;
> +
> +		reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> +
> +		pinctrl-0 = <&eusb6_reset_n>;
> +		pinctrl-names = "default";
> +	};
>   };
> 
>   &i2c17 {
> @@ -967,6 +980,14 @@ edp_reg_en: edp-reg-en-state {
>   		bias-disable;
>   	};
> 
> +	eusb6_reset_n: eusb6-reset-n-state {
> +		pins = "gpio184";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};
> +
>   	hall_int_n_default: hall-int-n-state {
>   		pins = "gpio92";
>   		function = "gpio";
> @@ -1172,3 +1193,37 @@ &usb_1_ss1_dwc3_hs {
>   &usb_1_ss1_qmpphy_out {
>   	remote-endpoint = <&retimer_ss1_ss_in>;
>   };
> +
> +&usb_mp {
> +	status = "okay";
> +};
> +
> +&usb_mp_hsphy0 {
> +	vdd-supply = <&vreg_l2e_0p8>;
> +	vdda12-supply = <&vreg_l3e_1p2>;
> +
> +	phys = <&eusb6_repeater>;

phys = <&eusb6_frp_repeater>;

That's how I did it on Insprion14 and it helps me make sense of the dts 
back to the schematic.

> +
> +	status = "okay";
> +};
> +
> +&usb_mp_hsphy1 {
> +	vdd-supply = <&vreg_l2e_0p8>;
> +	vdda12-supply = <&vreg_l3e_1p2>;
> +
> +	status = "okay";
> +};
> +
> +&usb_mp_qmpphy0 {
> +	vdda-phy-supply = <&vreg_l3e_1p2>;
> +	vdda-pll-supply = <&vreg_l3c_0p9>;
> +
> +	status = "okay";
> +};
> +
> +&usb_mp_qmpphy1 {
> +	vdda-phy-supply = <&vreg_l3e_1p2>;
> +	vdda-pll-supply = <&vreg_l3c_0p9>;
> +
> +	status = "okay";
> +};
> 
> --
> 2.48.1
> 
> 
> 
Completely optional obviously.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-dell-xps13-9345: Enable fingerprint sensor

[Stefan Schmidt]

Hello Bjorn,

On Wed, 19 Mar 2025 at 04:23, Bjorn Andersson via B4 Relay
<devnull+bjorn.andersson.oss.qualcomm.com@kernel.org> wrote:
>
> From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>
> The fingerprint sensor, hidden in the power button, is connected to one
> of the USB multiport ports; while the other port is unused.
>
> Describe the USB controller, the four phys and the repeater involved to
> make the fingerprint sensor operational.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Goodix Fingerprint USB Device fingerprint device shows up on my XPS 9345 now.

Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>

regards
Stefan Schmidt