| Message ID | 1360933026-30325-2-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 15, 2013 at 12:56:32PM +0000, Lee Jones wrote: > +static int ab8500_gpadc_suspend(struct device *dev) > +{ > + struct ab8500_gpadc *gpadc = dev_get_drvdata(dev); > + > + mutex_lock(&gpadc->ab8500_gpadc_lock); > + > + pm_runtime_get_sync(dev); > + > + regulator_disable(gpadc->regu); > + return 0; > +} This doesn't look especially sane... You're doing a runtime get, taking the lock without releasing it and disabling the regulator. This is *very* odd, both the changelog and the code need to explain what's going on and why it's safe in a lot more detail here.
On 20 February 2013 14:19, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Fri, Feb 15, 2013 at 12:56:32PM +0000, Lee Jones wrote: > >> +static int ab8500_gpadc_suspend(struct device *dev) >> +{ >> + struct ab8500_gpadc *gpadc = dev_get_drvdata(dev); >> + >> + mutex_lock(&gpadc->ab8500_gpadc_lock); >> + >> + pm_runtime_get_sync(dev); >> + >> + regulator_disable(gpadc->regu); >> + return 0; >> +} > > This doesn't look especially sane... You're doing a runtime get, taking > the lock without releasing it and disabling the regulator. This is > *very* odd, both the changelog and the code need to explain what's going > on and why it's safe in a lot more detail here. You need to do pm_runtime_get_sync to be able to make sure resources (which seems to be only the regulator) are safe to switch off. To my understanding this is a generic way to use for being able to switch off resources at a device suspend when runtime pm is used in conjunction. Regarding the mutex, I can't tell the reason behind it. It seems strange but not sure. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ Kind regards Ulf Hansson
On Thu, 21 Feb 2013, Ulf Hansson wrote: > On 20 February 2013 14:19, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: > > On Fri, Feb 15, 2013 at 12:56:32PM +0000, Lee Jones wrote: > > > >> +static int ab8500_gpadc_suspend(struct device *dev) > >> +{ > >> + struct ab8500_gpadc *gpadc = dev_get_drvdata(dev); > >> + > >> + mutex_lock(&gpadc->ab8500_gpadc_lock); > >> + > >> + pm_runtime_get_sync(dev); > >> + > >> + regulator_disable(gpadc->regu); > >> + return 0; > >> +} > > > > This doesn't look especially sane... You're doing a runtime get, taking > > the lock without releasing it and disabling the regulator. This is > > *very* odd, both the changelog and the code need to explain what's going > > on and why it's safe in a lot more detail here. > > You need to do pm_runtime_get_sync to be able to make sure resources > (which seems to be only the regulator) are safe to switch off. To my > understanding this is a generic way to use for being able to switch > off resources at a device suspend when runtime pm is used in > conjunction. > > Regarding the mutex, I can't tell the reason behind it. It seems > strange but not sure. Daniel, any thoughts? I'm happy to fixup, once I have the full story.
On Thu, Feb 21, 2013 at 11:45:08PM +0100, Ulf Hansson wrote: > On 20 February 2013 14:19, Mark Brown > > This doesn't look especially sane... You're doing a runtime get, taking > > the lock without releasing it and disabling the regulator. This is > > *very* odd, both the changelog and the code need to explain what's going > > on and why it's safe in a lot more detail here. > You need to do pm_runtime_get_sync to be able to make sure resources > (which seems to be only the regulator) are safe to switch off. To my > understanding this is a generic way to use for being able to switch > off resources at a device suspend when runtime pm is used in > conjunction. Are you sure this actually does what you think it does, especially when run on modern kernels?
On 22 February 2013 11:38, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Thu, Feb 21, 2013 at 11:45:08PM +0100, Ulf Hansson wrote: >> On 20 February 2013 14:19, Mark Brown > >> > This doesn't look especially sane... You're doing a runtime get, taking >> > the lock without releasing it and disabling the regulator. This is >> > *very* odd, both the changelog and the code need to explain what's going >> > on and why it's safe in a lot more detail here. > >> You need to do pm_runtime_get_sync to be able to make sure resources >> (which seems to be only the regulator) are safe to switch off. To my >> understanding this is a generic way to use for being able to switch >> off resources at a device suspend when runtime pm is used in >> conjunction. > > Are you sure this actually does what you think it does, especially when > run on modern kernels? Not sure, what you are thinking of more precisely here. Runtime pm has been in the kernel for quite some time now. Anyway, to make it a bit clearer, we switch the regulator on/off at the runtime suspend/resume callbacks. We want to take similar actions in device suspend/resume. To accomplish this a pm_runtime_get_sync is done in suspend and vice verse in resume, otherwise you can not safely handle the regulator. Kind regards Ulf Hansson
On Mon, Feb 25, 2013 at 10:27:36AM +0100, Ulf Hansson wrote: > On 22 February 2013 11:38, Mark Brown > > Are you sure this actually does what you think it does, especially when > > run on modern kernels? > Not sure, what you are thinking of more precisely here. Runtime pm has > been in the kernel for quite some time now. Yes, thanks - I was aware of that. The integration between runtime and system PM has been an area that's had some development though. > Anyway, to make it a bit clearer, we switch the regulator on/off at > the runtime suspend/resume callbacks. We want to take similar actions > in device suspend/resume. > To accomplish this a pm_runtime_get_sync is done in suspend and vice > verse in resume, otherwise you can not safely handle the regulator. Are you absolutely positive that with modern kernels your get actually resumes the device?
On 25 February 2013 14:51, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Feb 25, 2013 at 10:27:36AM +0100, Ulf Hansson wrote: >> On 22 February 2013 11:38, Mark Brown > >> > Are you sure this actually does what you think it does, especially when >> > run on modern kernels? > >> Not sure, what you are thinking of more precisely here. Runtime pm has >> been in the kernel for quite some time now. > > Yes, thanks - I was aware of that. The integration between runtime and > system PM has been an area that's had some development though. > >> Anyway, to make it a bit clearer, we switch the regulator on/off at >> the runtime suspend/resume callbacks. We want to take similar actions >> in device suspend/resume. >> To accomplish this a pm_runtime_get_sync is done in suspend and vice >> verse in resume, otherwise you can not safely handle the regulator. > > Are you absolutely positive that with modern kernels your get actually > resumes the device? Yes, runtime resume is always ok, But you can not runtime suspend the device, since the device suspend layer prevent this with a pm_runtime_get_noresume before calling the device suspend callback. Kind regards Ulf Hansson
diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c index b1f3561..9ed3afc 100644 --- a/drivers/mfd/ab8500-gpadc.c +++ b/drivers/mfd/ab8500-gpadc.c @@ -605,6 +605,31 @@ static int ab8500_gpadc_runtime_idle(struct device *dev) return 0; } +static int ab8500_gpadc_suspend(struct device *dev) +{ + struct ab8500_gpadc *gpadc = dev_get_drvdata(dev); + + mutex_lock(&gpadc->ab8500_gpadc_lock); + + pm_runtime_get_sync(dev); + + regulator_disable(gpadc->regu); + return 0; +} + +static int ab8500_gpadc_resume(struct device *dev) +{ + struct ab8500_gpadc *gpadc = dev_get_drvdata(dev); + + regulator_enable(gpadc->regu); + + pm_runtime_mark_last_busy(gpadc->dev); + pm_runtime_put_autosuspend(gpadc->dev); + + mutex_unlock(&gpadc->ab8500_gpadc_lock); + return 0; +} + static int ab8500_gpadc_probe(struct platform_device *pdev) { int ret = 0; @@ -698,6 +723,9 @@ static const struct dev_pm_ops ab8500_gpadc_pm_ops = { SET_RUNTIME_PM_OPS(ab8500_gpadc_runtime_suspend, ab8500_gpadc_runtime_resume, ab8500_gpadc_runtime_idle) + SET_SYSTEM_SLEEP_PM_OPS(ab8500_gpadc_suspend, + ab8500_gpadc_resume) + }; static struct platform_driver ab8500_gpadc_driver = {