Message-ID: <AANLkTi=iXSMU4yH2xV_dYO5nf4E_zTRww1Y+32U7GQbs@mail.gmail.com>
Date:	Wed, 9 Mar 2011 11:35:09 +0900
From:	MyungJoo Ham <myungjoo.ham@...sung.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	linux-kernel@...r.kernel.org, Liam Girdwood <lrg@...mlogic.co.uk>,
	kyungmin.park@...sung.com
Subject: Re: [PATCH] Regulator: add suspend-finish API for regulator core.

On Tue, Mar 8, 2011 at 10:38 PM, Mark Brown
<broonie@...nsource.wolfsonmicro.com> wrote:
> On Tue, Mar 08, 2011 at 11:03:58AM +0900, MyungJoo Ham wrote:
>> The regulator core had suspend-prepare that turns off the regulators
>> when entering a system-wide suspend. However, it did not have
>> suspend-finish that recovers the change made by suspend-prepare and
>> depends on machine pm code or the regulator device or driver
>> doing so.
>
> This is a good idea, thanks for working on it.

Welcome.

>
> Your commit message is sligtly inaccurate as this isn't what
> suspend_prepare() is for, suspend_prepare() is for matching the suspend
> mode configuration of regulators that support that with the suspend mode
> Linux is using (RAM, disk and so on).  There is no need for this to
> recover the pre-suspend state as hardware implementing suspend mode
> configuration should be able to do so autonomously.
>
> Of course not all hardware supports a distinct suspend mode and for
> hardware that doesn't we should be doing pretty much this - it's a
> bit of a hole in our regulator support at the minute.  It should really
> be coupled with a soft suspend mode implementation which can put the
> regulators into an appropriate state for suspend on the way down.

Yes. It'd be more accurate to describe that the patch is to set states
of the regulators according to their state-related regulator values
(use_count, always_on) at resume.

>
>> +               if ((rdev->use_count > 0  || rdev->constraints->always_on) &&
>> +                               rdev->desc->ops->enable) {
>> +                       error = rdev->desc->ops->enable(rdev);
>> +                       if (error)
>> +                               ret = error;
>
> We should probably also be turning off regulators that shouldn't be on -
> a regulator may default to being enabled when we don't want it.
> Thinking about it we can probably share most if not all of the code with
> regulator_init_complete()...
>
> Ideally we'd also restore voltages but that can always be added later.

Agreed. I'll add rdev->desc->ops->disable(rdev) for the else statement
of that if statement.

>
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(regulator_suspend_finish);
>
> Hrm, I'd really expect the core to be arranging for this to happen
> rather than exporting the function?  Though the sequencing so it gets
> called at the right time might be a bit tricky and I've not actually
> looked at the isues here.
>

Currently, it depends on machine's pm code calling
regulator_suspend_prepare. And, that's why I made
regulator_suspend_finish to be called by someone else. This appears to
be odd to me as well; however, probably, like
regulator_init_complete() is optional, some systems cannot allow
regulators to be controlled by regulator_suspend_prepare by default or
the executed position (after suspend_ops->prepare, right before
suspend_ops->enter, or ...) may vary. If we are to let the core
arrange regulator_suspend_prepare/finish, do you think putting it
right after "suspend_ops->prepare" and right before
"suspend_ops->enter"?


Version-2 Patch is incoming. (with disable feature at suspend_finish)

-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists