Re: [PATCH v5 1/5] regulator: Document binding for initial and suspend modes

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]