Re: [PATCHv4 1/2] regulator: of: Add support for parsing regulator_state for suspend state

[Mark Brown <broonie@kernel.org>]

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

On Mon, Aug 18, 2014 at 03:27:02PM +0900, Chanwoo Choi wrote:

> +		suspend_uV = of_get_property(suspend_np, "regulator-volt",
> +						NULL);
> +		if (suspend_uV) {
> +			suspend_state->uV = be32_to_cpu(*suspend_uV);
> +
> +			if (suspend_state->uV < constraints->min_uV)
> +				suspend_state->uV = constraints->min_uV;
> +			if (suspend_state->uV > constraints->max_uV)
> +				suspend_state->uV = constraints->max_uV;
> +		}

A few things here.  One is that this will fail if we don't have a
voltage range specified at runtime.  It is possible that the user needs
to override the voltage in suspend mode but not at runtime.  Perhaps we
want to make them specify the runtime voltage anyway but if we do then
the binding needs to say that this is mandatory even if it's just
restating the hardware default.

It's also not clear to me that the suspend voltage needs to be in the
range that we can vary at runtime, especially if we're not giving
permission for runtime variation.  From that point of view we should
probably just not check, but if we are going to check we should print a
warning that we're doing it so people know that what they set in the DT
isn't being followed.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]