Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

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.martinez@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.martinez@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, &regulator_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