* Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> [170518 08:28]:
> On Wed, 17 May 2017, Tony Lindgren wrote:
>
> > This is needed in preparation of adding support for omap3 and
> > later OHCI. The runtime PM will only do something on platforms
> > that implement it.
>
> This patch isn't correct. See below.
>
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Roger Quadros <rogerq@xxxxxx>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> > ---
> > drivers/usb/host/ohci-platform.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
> > --- a/drivers/usb/host/ohci-platform.c
> > +++ b/drivers/usb/host/ohci-platform.c
> > @@ -24,6 +24,7 @@
> > #include <linux/err.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/reset.h>
> > #include <linux/usb/ohci_pdriver.h>
> > #include <linux/usb.h>
> > @@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device *dev)
> > struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
> > int clk, ret, phy_num;
> >
> > + ret = pm_runtime_get_sync(&dev->dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) {
> > ret = clk_prepare_enable(priv->clks[clk]);
> > if (ret)
> > @@ -96,6 +101,8 @@ static void ohci_platform_power_off(struct platform_device *dev)
> > for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
> > if (priv->clks[clk])
> > clk_disable_unprepare(priv->clks[clk]);
> > +
> > + pm_runtime_put_sync(&dev->dev);
> > }
>
> ohci_platform_power_on() is invoked (by default) by the runtime-resume
> callback routine ohci_platform_resume(), through the pdata->power_on
> method pointer. Likewise, ohci_platform_power_off() is invoked by the
> runtime-suspend callback routine.
>
> This means you shouldn't do pm_runtime_get/put calls from within these
> routines.
Oh OK great, so the above should not be needed at all.
> Is there any particular reason you wanted to add these calls? In
> general, USB host controllers are expected to go into runtime suspend
> whenever there aren't any children keeping them awake. Hence they
> usually don't need to worry about initiating their own suspends and
> resumes.
No particular reason as it sounds things work without it :) I'll check.
> > static struct hc_driver __read_mostly ohci_platform_hc_driver;
> > @@ -242,6 +249,7 @@ static int ohci_platform_probe(struct platform_device *dev)
> > }
> > #endif
> >
> > + pm_runtime_enable(&dev->dev);
>
> There should be a pm_runtime_set_active() just before this.
OK
> > if (pdata->power_on) {
> > err = pdata->power_on(dev);
> > if (err < 0)
> > @@ -271,6 +279,7 @@ static int ohci_platform_probe(struct platform_device *dev)
> > if (pdata->power_off)
> > pdata->power_off(dev);
> > err_reset:
> > + pm_runtime_disable(&dev->dev);
> > while (--rst >= 0)
> > reset_control_assert(priv->resets[rst]);
> > err_put_clks:
> > @@ -305,6 +314,8 @@ static int ohci_platform_remove(struct platform_device *dev)
> >
> > usb_put_hcd(hcd);
> >
> > + pm_runtime_disable(&dev->dev);
> > +
> > if (pdata == &ohci_platform_defaults)
> > dev->dev.platform_data = NULL;
>
> These changes make sense.
OK thanks for looking.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
