On Fri, Nov 07, 2014 at 04:38:04PM +0100, Javier Martinez Canillas wrote:
> On 11/07/2014 03:58 PM, Mark Brown wrote:
> > On Fri, Nov 07, 2014 at 02:00:01PM +0100, Javier Martinez Canillas wrote:
> >> + The "regulator-mode" property only takes effect if the regulator is
> >> + enabled for the given suspend state using "regulator-on-in-suspend".
> > Why?
> I saw that the regulator core only call the .set_suspend_mode callback if
> the regulator is either enabled or disabled explicitly...
That's the current implementation. Why are you adding it to a
specification?
> static int suspend_set_state(struct regulator_dev *rdev,
> struct regulator_state *rstate)
> {
> ....
> /* If we have no suspend mode configration don't set anything;
> * only warn if the driver implements set_suspend_voltage or
> * set_suspend_mode callback.
> */
> if (!rstate->enabled && !rstate->disabled) {
For example why couldn't these get set by reading the current state?
> >> + If the regulator has not been explicitly disabled for the given state
> >> + with "regulator-off-in-suspend", then setting the operating mode
> >> + will also have no effect.
> > This seems surprising, I'd expect mode setting to be paid attention to
> > even if the regulator is off - we may add other ways to control the
> > enable state in suspend for example.
> ...and I thought that setting a mode if the regulator was disabled in suspend
> was not a possible configuration, that's why I documented that.
Like I say this is at the very least making it impossible for us to in
future add other ways of setting if the regulator is enabled or disabled
in suspend.
> I know that there is both a .set_suspend_disable and .set_suspend_mode but
> at least in the hardware that I'm interested in (max77802), the same hw
> register is used for setting a suspend mode and disable on suspend.
That's your device, other hardware exists which uses seprate bitfields.
> >> +If no mode is defined, then the OS will not manage the modes and the hardware
> >> +default values will be used instead.
> > Again that seems surprising, it precludes any future changes and isn't
> > going to be true for devices where we can't read the current state.
> I saw that you asked Chanwoo on an early version of his suspend states
> series, to point out that in the absence of a initial state, the state is
> the hardware default [0] so I thought that it should be the case for the
> regulator suspend mode too.
You're also saying that the OS won't manage the mode here, that's a step
further.
> So should I just explain what each property is about without trying to make
> assumptions about the limitations that different devices could have and let
> each device DT binding to specify those?
More towards that direction, yes. Don't overspecify.
Attachment:
signature.asc
Description: Digital signature
