| Message ID | 1391529538-21685-15-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > Since the device is active while a successful probe has been completed, > the reference counting for the clock will be screwed up and never reach > zero. > > The issue is resolved by implementing runtime PM callbacks and let them > handle the resources accordingly, including the clock. > > Cc: Alessandro Rubini <rubini@unipv.it> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Hm do I read it right as patch 13 breaks runtime PM by leaving the device active after probe() and this patch 14 fixes it again? Maybe these two patches should be squashed then. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 February 2014 15:34, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> Since the device is active while a successful probe has been completed, >> the reference counting for the clock will be screwed up and never reach >> zero. >> >> The issue is resolved by implementing runtime PM callbacks and let them >> handle the resources accordingly, including the clock. >> >> Cc: Alessandro Rubini <rubini@unipv.it> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Hm do I read it right as patch 13 breaks runtime PM by leaving > the device active after probe() and this patch > 14 fixes it again? Maybe these two patches should be squashed > then. You are right; but the driver will still be working, you just don't get the benefit from inactivating the device at request inactivity - as you pointed out. The reason for why I wanted to do this as separate steps was to make it easier for reviewing, otherwise the patch(es) would have been quite big and messy. I am for sure open to adopt to your proposal, but just wanted to give you some more background, before I go ahead and send a v2. Kind regards Ulf Hansson > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 February 2014 15:45, Fabio Estevam <festevam@gmail.com> wrote: > On Tue, Feb 4, 2014 at 1:58 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> +static int nmk_i2c_runtime_resume(struct device *dev) >> +{ >> + struct amba_device *adev = to_amba_device(dev);, >> + struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev); >> + >> + clk_prepare_enable(nmk_i2c->clk); > > Previously the code was checking the return value from > clk_prepare_enable(). You should keep the check here. Will fix in v2! Thanks for reviewing! Kind regards Ulf Hansson > > Regards, > > Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 February 2014 11:14, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 5 February 2014 15:34, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >>> Since the device is active while a successful probe has been completed, >>> the reference counting for the clock will be screwed up and never reach >>> zero. >>> >>> The issue is resolved by implementing runtime PM callbacks and let them >>> handle the resources accordingly, including the clock. >>> >>> Cc: Alessandro Rubini <rubini@unipv.it> >>> Cc: Linus Walleij <linus.walleij@linaro.org> >>> Cc: Wolfram Sang <wsa@the-dreams.de> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> >> Hm do I read it right as patch 13 breaks runtime PM by leaving >> the device active after probe() and this patch >> 14 fixes it again? Maybe these two patches should be squashed >> then. In v2 I have now squashed patch 13 into this patch 14. That means patch13 shall be dropped from this patchset. Kind regards Uffe > > You are right; but the driver will still be working, you just don't > get the benefit from inactivating the device at request inactivity - > as you pointed out. > > The reason for why I wanted to do this as separate steps was to make > it easier for reviewing, otherwise the patch(es) would have been quite > big and messy. I am for sure open to adopt to your proposal, but just > wanted to give you some more background, before I go ahead and send a > v2. > > Kind regards > Ulf Hansson > >> >> Yours, >> Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 0caa8ea..2d7dbc9 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -674,7 +674,7 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *dev, u16 flags) static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[], int num_msgs) { - int status; + int status = 0; int i; struct nmk_i2c_dev *dev = i2c_get_adapdata(i2c_adap); int j; @@ -683,19 +683,6 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, pm_runtime_get_sync(&dev->adev->dev); - status = clk_prepare_enable(dev->clk); - if (status) { - dev_err(&dev->adev->dev, "can't prepare_enable clock\n"); - goto out_clk; - } - - /* Optionaly enable pins to be muxed in and configured */ - pinctrl_pm_select_default_state(&dev->adev->dev); - - status = init_hw(dev); - if (status) - goto out; - /* Attempt three times to send the message queue */ for (j = 0; j < 3; j++) { /* setup the i2c controller */ @@ -716,12 +703,6 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, break; } -out: - clk_disable_unprepare(dev->clk); -out_clk: - /* Optionally let pins go into idle state */ - pinctrl_pm_select_idle_state(&dev->adev->dev); - pm_runtime_put_sync(&dev->adev->dev); dev->busy = false; @@ -938,6 +919,29 @@ static int nmk_i2c_resume(struct device *dev) #define nmk_i2c_resume NULL #endif +#if CONFIG_PM +static int nmk_i2c_runtime_suspend(struct device *dev) +{ + struct amba_device *adev = to_amba_device(dev); + struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev); + + clk_disable_unprepare(nmk_i2c->clk); + pinctrl_pm_select_idle_state(dev); + return 0; +} + +static int nmk_i2c_runtime_resume(struct device *dev) +{ + struct amba_device *adev = to_amba_device(dev); + struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev); + + clk_prepare_enable(nmk_i2c->clk); + pinctrl_pm_select_default_state(dev); + init_hw(nmk_i2c); + return 0; +} +#endif + /* * We use noirq so that we suspend late and resume before the wakeup interrupt * to ensure that we do the !pm_runtime_suspended() check in resume before @@ -946,6 +950,9 @@ static int nmk_i2c_resume(struct device *dev) static const struct dev_pm_ops nmk_i2c_pm = { .suspend_noirq = nmk_i2c_suspend, .resume_noirq = nmk_i2c_resume, + SET_PM_RUNTIME_PM_OPS(nmk_i2c_runtime_suspend, + nmk_i2c_runtime_resume, + NULL) }; static unsigned int nmk_i2c_functionality(struct i2c_adapter *adap)
Since the device is active while a successful probe has been completed, the reference counting for the clock will be screwed up and never reach zero. The issue is resolved by implementing runtime PM callbacks and let them handle the resources accordingly, including the clock. Cc: Alessandro Rubini <rubini@unipv.it> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Wolfram Sang <wsa@the-dreams.de> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/i2c/busses/i2c-nomadik.c | 47 ++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 20 deletions(-)