Re: [PATCH 1/2] arm64: dts: qcom: x1e80100-crd: Add USB multiport fingerprint reader

[Krishna Kurapati]

On 12/3/2024 6:45 PM, Krishna Kurapati wrote:
> On 12/3/2024 3:50 PM, Johan Hovold wrote:
> > [ +CC: Krishna, Thinh and the USB list ]
> >
> > On Mon, Nov 18, 2024 at 11:34:29AM +0100, Stephan Gerhold wrote:
> > > The X1E80100 CRD has a Goodix fingerprint reader connected to the USB
> > > multiport controller on eUSB6. All other ports (including USB 
> > > super-speed
> > > pins) are unused.
> > >
> > > Set it up in the device tree together with the NXP PTN3222 repeater.
> > >
> > > Signed-off-by: Stephan Gerhold <stephan.gerhold@xxxxxxxxxx>
> > > ---
> > >   arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 48 
> > > +++++++++++++++++++++++++++++++
> > >   1 file changed, 48 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts 
> > > b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > > index 39f9d9cdc10d..44942931c18f 100644
> > > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > > @@ -735,6 +735,26 @@ keyboard@3a {
> > >       };
> > >   };
> > > +&i2c5 {
> > > +    clock-frequency = <400000>;
> > > +
> > > +    status = "okay";
> > > +
> > > +    eusb6_repeater: redriver@4f {
> > > +        compatible = "nxp,ptn3222";
> > > +        reg = <0x4f>;
> >
> > The driver does not currently check that there's actually anything at
> > this address. Did you verify that this is the correct address?
> >
> > (Abel is adding a check to the driver as we speak to catch any such
> > mistakes going forward).
> >
> > > +        #phy-cells = <0>;
> >
> > nit: I'd put provider properties like this one last.
> >
> > > +        vdd3v3-supply = <&vreg_l13b_3p0>;
> > > +        vdd1v8-supply = <&vreg_l4b_1p8>;
> >
> > Sort by supply name?
> >
> > > +        reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> > > +
> > > +        pinctrl-0 = <&eusb6_reset_n>;
> > > +        pinctrl-names = "default";
> > > +    };
> > > +};
> > > +
> > >   &i2c8 {
> > >       clock-frequency = <400000>;
> > > @@ -1047,6 +1067,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;
> >
> > I don't think the pin config should assert reset, that should be up to
> > the driver to control.
> >
> > > +    };
> > > +
> > >       hall_int_n_default: hall-int-n-state {
> > >           pins = "gpio92";
> > >           function = "gpio";
> > > @@ -1260,3 +1288,23 @@ &usb_1_ss2_dwc3_hs {
> > >   &usb_1_ss2_qmpphy_out {
> > >       remote-endpoint = <&pmic_glink_ss2_ss_in>;
> > >   };
> > > +
> > > +&usb_mp {
> > > +    status = "okay";
> > > +};
> > > +
> > > +&usb_mp_dwc3 {
> > > +    /* Limit to USB 2.0 and single port */
> > > +    maximum-speed = "high-speed";
> > > +    phys = <&usb_mp_hsphy1>;
> > > +    phy-names = "usb2-1";
> > > +};
> >
> > The dwc3 driver determines (and acts on) the number of ports based on
> > the port interrupts in DT and controller capabilities.
> >
> > I'm not sure we can (should) just drop the other HS PHY and the SS PHYs
> > that would still be there in the SoC (possibly initialised by the boot
> > firmware).
>
> The DWC3 core driver identifies number of ports based on xHCI registers. 
> The QC Wrapper reads this number via interrupts. But these two values 
> are independent of each other. The core driver uses these values to 
> identify and manipulate phys. Even if only one HS is given in multiport 
> it would be sufficient if the name is "usb2-1". If the others are 
> missing, those phys would be read by driver as NULL and any ops to phys 
> would be NOP.

However do we need to reduce the number of interrupts used in DTS ?
We don't need to give all interrupts as there is only one port present.
We don't want to enable all interrupts when ports are not exposed.

Regards,
Krishna,