On Sun, Dec 09, 2018 at 04:44:16PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 6, 2018 at 8:49 PM Stephan Gerhold <[email protected]> wrote:
> >
> > Commit 211f658b7b40 ("usb: dwc3: pci: Use devm functions to get
> > the phy GPIOs") changed the code to claim the PHY GPIOs permanently
> > for Intel Baytrail devices.
> >
> > This causes issues when the actual PHY driver attempts to claim the
> > same GPIO descriptors. For example, tusb1210 now fails to probe with:
> >
> > tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)
> >
> > dwc3-pci needs to turn on the PHY once before dwc3 is loaded, but
> > usually the PHY driver will then hold the GPIOs to turn off the
> > PHY when requested (e.g. during suspend).
> >
> > To fix the problem, this reverts the commit to restore the old
> > behavior to put the GPIOs immediately after usage.
> >
> > Link: https://www.spinics.net/lists/linux-usb/msg174681.html
> > Cc: [email protected]
> > Signed-off-by: Stephan Gerhold <[email protected]>
> > ---
> > drivers/usb/dwc3/dwc3-pci.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > index 842795856bf4..fdc6e4e403e8 100644
> > --- a/drivers/usb/dwc3/dwc3-pci.c
> > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > @@ -170,20 +170,20 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
> > * put the gpio descriptors again here because the
> > phy driver
> > * might want to grab them, too.
> > */
> > - gpio = devm_gpiod_get_optional(&pdev->dev, "cs",
> > - GPIOD_OUT_LOW);
> > + gpio = gpiod_get_optional(&pdev->dev, "cs",
> > GPIOD_OUT_LOW);
> > if (IS_ERR(gpio))
> > return PTR_ERR(gpio);
> >
> > gpiod_set_value_cansleep(gpio, 1);
> > + gpiod_put(gpio);
> >
> > - gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> > - GPIOD_OUT_LOW);
> > + gpio = gpiod_get_optional(&pdev->dev, "reset",
> > GPIOD_OUT_LOW);
> > if (IS_ERR(gpio))
> > return PTR_ERR(gpio);
> >
> > if (gpio) {
> > gpiod_set_value_cansleep(gpio, 1);
>
> > + gpiod_put(gpio);
> > usleep_range(10000, 11000);
>
> If something happens to GPIO line in between of these lines, the sleep
> might become obsolete. Shouldn't gpiod_put() be placed after?
That's a good point, but I believe this would be more appropriately
fixed in a separate patch, since this is just an exact revert of
211f658b7b40 ("usb: dwc3: pci: Use devm functions to get the phy GPIOs")
(This is the way it was written when it was added to mainline 4 years
ago...)
I can send a separate patch for this, or would you like to?
>
> > }
> > }
>
>
> --
> With Best Regards,
> Andy Shevchenko
