From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757342AbbCCTF5 (ORCPT ); Tue, 3 Mar 2015 14:05:57 -0500 Received: from bhuna.collabora.co.uk ([93.93.135.160]:34983 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756821AbbCCTFz (ORCPT ); Tue, 3 Mar 2015 14:05:55 -0500 Message-ID: <54F60602.3030505@collabora.co.uk> Date: Tue, 03 Mar 2015 20:05:38 +0100 From: Javier Martinez Canillas User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 MIME-Version: 1.0 To: Doug Anderson CC: Mark Brown , Liam Girdwood , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume References: <1425328839-28330-1-git-send-email-javier.martinez@collabora.co.uk> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 >> --- >> 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