Hello Doug, On 03/03/2015 06:24 PM, Doug Anderson wrote: > Javier, > > On Mon, Mar 2, 2015 at 12:40 PM, Javier Martinez Canillas > <javier.marti...@collabora.co.uk> wrote: >> After leaving from system wide suspend state, regulator_suspend_finish() >> turn on regulators that may be turned off by regulator_suspend_prepare() >> but it tries to enable all regulators that have an enable count > 0 or >> that were marked as "always-on" regardless if those were disabled or not. >> >> Trying to enable an already enabled regulator may cause issues so is >> better to skip enabling regulators that were not disabled before suspend. >> >> Signed-off-by: Javier Martinez Canillas <javier.marti...@collabora.co.uk> >> --- >> drivers/regulator/core.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) > > I've tested this and it also fixes the problem that my patch > (regulator: core: Fix enable GPIO reference counting - > https://patchwork.kernel.org/patch/5903071) fixes. >
Thanks a lot for testing. > As I said in the other conversation I think both patches could land. Agreed that both patches should land. > ...but maybe change your commit message to something like: > > The _regulator_do_enable() call ought to be a no-op when called on an > already-enabled regulator. However, as an optimization > _regulator_enable() doesn't call _regulator_do_enable() on an already > enabled regulator. That means we never test the case of calling > _regulator_do_enable() during normal usage and there may be hidden > bugs or warnings. We have seen warnings issued by the tps65090 driver > and bugs when using the GPIO enable pin. > > Let's match the same optimization that _regulator_enable() in > regulator_suspend_finish(). That may speed up suspend/resume and also > avoids exposing hidden bugs. > Right, I'll change the commit message since your suggestion is more clear. >> >> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c >> index f2452148c8da..8551400d57e4 100644 >> --- a/drivers/regulator/core.c >> +++ b/drivers/regulator/core.c >> @@ -3816,9 +3816,11 @@ int regulator_suspend_finish(void) >> list_for_each_entry(rdev, ®ulator_list, list) { >> mutex_lock(&rdev->mutex); >> if (rdev->use_count > 0 || rdev->constraints->always_on) { >> - error = _regulator_do_enable(rdev); >> - if (error) >> - ret = error; >> + if (!_regulator_is_enabled(rdev)) { > > Looking at _regulator_enable() I see that _regulator_is_enabled() > could return an error. Should we be checking? Maybe we should have a > helper function called by both callers? > Thanks for pointing that out. I'll change it on v2 as well. > > I have tested this on my system and it works. Other than the error > check / updated commit message this looks good to me. > > I guess that means that I can include your Tested-by tag when doing a re-spin? Please let me know otherwise. > -Doug > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/