| Message ID | 1415365205-27630-2-git-send-email-javier.martinez@collabora.co.uk |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? > + 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. > +- regulator-initial-mode: initial operating mode. The set of possible operating > + modes is the same used for the regulator-mode property and the device binding > + documentation explains which property each regulator supports. > +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.
Hello Mark, Thanks a lot for your feedback. 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... 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) { if (rdev->desc->ops->set_suspend_voltage || rdev->desc->ops->set_suspend_mode) rdev_warn(rdev, "No configuration\n"); return 0; } ... }; >> + 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. 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. So if this is allowed and a user specifies both a disable on suspend and a suspend mode, the later will overwrite the configuration of the former. But now that I think about it, this is only a constraint of the max77802 and probably needs to be specified in the max77802 DT binding and not in the regulator generic one since otherwise limits what other devices can do. >> +- regulator-initial-mode: initial operating mode. The set of possible operating >> + modes is the same used for the regulator-mode property and the device binding >> + documentation explains which property each regulator supports. >> +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. 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? Best regards, Javier [0]: https://lkml.org/lkml/2014/9/4/652 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Hello Mark, On 11/07/2014 05:10 PM, Mark Brown wrote: > > You're also saying that the OS won't manage the mode here, that's a step > further. > I see >> 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. > Got it, thanks a lot for your valuable feedback. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt index 4e7ed76..3fffa3b 100644 --- a/Documentation/devicetree/bindings/regulator/regulator.txt +++ b/Documentation/devicetree/bindings/regulator/regulator.txt @@ -30,6 +30,20 @@ Optional properties: - regulator-off-in-suspend: regulator should be off in suspend state. - regulator-suspend-microvolt: regulator should be set to this voltage in suspend. + - regulator-mode: operating mode in the given suspend state. + The set of possible operating modes depends on the capabilities of + every hardware so the valid modes are documented on each regulator + device tree binding document. + The "regulator-mode" property only takes effect if the regulator is + enabled for the given suspend state using "regulator-on-in-suspend". + 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. +- regulator-initial-mode: initial operating mode. The set of possible operating + modes is the same used for the regulator-mode property and the device binding + documentation explains which property each regulator supports. +If no mode is defined, then the OS will not manage the modes and the hardware +default values will be used instead. Deprecated properties: - regulator-compatible: If a regulator chip contains multiple
Some regulators can run on different operating modes (opmodes). This allows systems to choose the most efficient opmode for each regulator. This patch builds on top of (291d761 regulator: Document binding for regulator suspend state for PM state) adding a regulator-initial-mode DT property to configure at startup the operating mode for regulators that support changing its mode during normal operation and a property regulator-mode to be used in the regulator-state-[mem/disk] nodes for regulators that supports changing its operating mode when the system enters in a suspend state. The set of possible modes that a regulator can operate depends on the hardware capabilities so a list of generic operating modes can't be provided. Instead, each hardware binding should define the list of valid operating modes for the regulators found on that device. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- Changes in v5: None Changes in v4: None Changes in v3: - Rebased on top of regulator suspend voltage series Documentation/devicetree/bindings/regulator/regulator.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+)