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.
signature.asc
Description: Digital signature