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

From: Javier Martinez Canillas
Date: Tue Mar 03 2015 - 14:06:00 EST


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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/