| Message ID | 1546944944-13911-3-git-send-email-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | add support for power off check in suspend | expand |
On Tue, Jan 08, 2019 at 10:56:32AM +0000, Claudiu.Beznea@microchip.com wrote: > From: Claudiu Beznea <claudiu.beznea@microchip.com> > > Add helper to check if regulator will be disabled in suspend. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> This feels like it's the wrong way round - if this is configurable I'd expect something to configure the suspend mode and then for that to arrange to configure the regulator appropriately (along with anything else that needs doing) rather than to infer the configuration from the regulator state which feels fragile. But based on the cover letter that's kind of like what the initial proposal about target states was so perhaps this is the way we end up going... this certainly looks a lot less impactful that the target state stuff though.
On 09.01.2019 18:57, Mark Brown wrote: > On Tue, Jan 08, 2019 at 10:56:32AM +0000, Claudiu.Beznea@microchip.com wrote: >> From: Claudiu Beznea <claudiu.beznea@microchip.com> >> >> Add helper to check if regulator will be disabled in suspend. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > This feels like it's the wrong way round - if this is configurable I'd > expect something to configure the suspend mode and then for that to > arrange to configure the regulator appropriately (along with anything > else that needs doing) rather than to infer the configuration from the > regulator state which feels fragile. But based on the cover letter > that's kind of like what the initial proposal about target states was so > perhaps this is the way we end up going... Are you talking about [1] ? this certainly looks a lot > less impactful that the target state stuff though. > For the moment, the patches which describes the regulators states in suspend for SAMA5D2 Xplained board (which we are trying to address here) are in pending [2] (they were introduced with patches for act8945a suspend/resume stuff). Probably they will be introduced after one more Linux version. I can get rid of this patch, take advantage of [3] and [4] and introduce also the regulator standby states. In this case, no matter the mapping b/w Linux power saving modes and AT91 SoC's power saving modes, we will be covered on misconfiguration (at least on SAMA5D2 Xplained board). And in patch 3/3 I could get rid of regulator checks and rely on DT (bad thing would be that in case of no input for regulator's state in mem/standby the board could not properly suspended/resumed), if any. What do you think about this? Thank you, Claudiu Beznea [1] https://patchwork.kernel.org/patch/9458445/ [2] https://lore.kernel.org/lkml/1544543768-2066-6-git-send-email-claudiu.beznea@microchip.com/ [3] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?h=for-next&id=f2b4076988a9c229dab373470b4b108ef0e106c8 [4] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?h=for-next&id=5279e96ff8033500b6008be5925ae2d20f42c434
On Thu, Jan 10, 2019 at 10:24:26AM +0000, Claudiu.Beznea@microchip.com wrote: > On 09.01.2019 18:57, Mark Brown wrote: > > regulator state which feels fragile. But based on the cover letter > > that's kind of like what the initial proposal about target states was so > > perhaps this is the way we end up going... > Are you talking about [1] ? I can't follow that link right now, I'm working offline. > I can get rid of this patch, take advantage of [3] and [4] and introduce > also the regulator standby states. In this case, no matter the mapping b/w > Linux power saving modes and AT91 SoC's power saving modes, we will be > covered on misconfiguration (at least on SAMA5D2 Xplained board). > And in patch 3/3 I could get rid of regulator checks and rely on DT (bad > thing would be that in case of no input for regulator's state in > mem/standby the board could not properly suspended/resumed), if any. > What do you think about this? Like I say I'm working offline so I can't check the links but it sounds like you're saying that the existing suspend mode configuration features are enough for your systems? If so that's great - certainly what you're saying above sounds sensible to me but it's possible I misunderstood something based on not having the links.
On 11.01.2019 14:39, Mark Brown wrote: > On Thu, Jan 10, 2019 at 10:24:26AM +0000, Claudiu.Beznea@microchip.com wrote: >> On 09.01.2019 18:57, Mark Brown wrote: > >>> regulator state which feels fragile. But based on the cover letter >>> that's kind of like what the initial proposal about target states was so >>> perhaps this is the way we end up going... > >> Are you talking about [1] ? > > I can't follow that link right now, I'm working offline. > >> I can get rid of this patch, take advantage of [3] and [4] and introduce >> also the regulator standby states. In this case, no matter the mapping b/w >> Linux power saving modes and AT91 SoC's power saving modes, we will be >> covered on misconfiguration (at least on SAMA5D2 Xplained board). > >> And in patch 3/3 I could get rid of regulator checks and rely on DT (bad >> thing would be that in case of no input for regulator's state in >> mem/standby the board could not properly suspended/resumed), if any. > >> What do you think about this? > > Like I say I'm working offline so I can't check the links but it sounds > like you're saying that the existing suspend mode configuration features > are enough for your systems? Yes, if we rely on the fact that core's regulator device tree bindings for suspend-to-mem/suspend-to-standby were filled correctly. The function I added here was to double check that core's regulator will be off in suspend/standby based on what was parsed from DT. Thank you, Claudiu Beznea > so that's great - certainly what you're > saying above sounds sensible to me but it's possible I misunderstood > something based on not having the links. >
On Fri, Jan 11, 2019 at 02:08:19PM +0000, Claudiu.Beznea@microchip.com wrote: > On 11.01.2019 14:39, Mark Brown wrote: > > Like I say I'm working offline so I can't check the links but it sounds > > like you're saying that the existing suspend mode configuration features > > are enough for your systems? > Yes, if we rely on the fact that core's regulator device tree bindings for > suspend-to-mem/suspend-to-standby were filled correctly. > The function I added here was to double check that core's regulator will be > off in suspend/standby based on what was parsed from DT. Ah, so it's being used as a consistency check? OK, that does make sense to me.
On 15.01.2019 00:53, Mark Brown wrote: > On Fri, Jan 11, 2019 at 02:08:19PM +0000, Claudiu.Beznea@microchip.com wrote: >> On 11.01.2019 14:39, Mark Brown wrote: > >>> Like I say I'm working offline so I can't check the links but it sounds >>> like you're saying that the existing suspend mode configuration features >>> are enough for your systems? > >> Yes, if we rely on the fact that core's regulator device tree bindings for >> suspend-to-mem/suspend-to-standby were filled correctly. > >> The function I added here was to double check that core's regulator will be >> off in suspend/standby based on what was parsed from DT. > > Ah, so it's being used as a consistency check? Yes, that was the idea. Thank you, Claudiu Beznea > OK, that does make sense > to me. >
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index b9d7b45c7295..3b761969732a 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3786,6 +3786,23 @@ int regulator_suspend_disable(struct regulator_dev *rdev, } EXPORT_SYMBOL_GPL(regulator_suspend_disable); +bool regulator_is_disabled_in_suspend(struct regulator *regulator, + suspend_state_t state) +{ + struct regulator_state *rstate; + + if (!regulator->rdev->constraints) + return false; + + rstate = regulator_get_suspend_state(regulator->rdev, state); + if (!rstate) + return false; + + return regulator->rdev->desc->ops->set_suspend_disable && + rstate->enabled == DISABLE_IN_SUSPEND; +} +EXPORT_SYMBOL_GPL(regulator_is_disabled_in_suspend); + static int _regulator_set_suspend_voltage(struct regulator *regulator, int min_uV, int max_uV, suspend_state_t state) diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h index f3f76051e8b0..cf5e71dc63ce 100644 --- a/include/linux/regulator/consumer.h +++ b/include/linux/regulator/consumer.h @@ -36,6 +36,7 @@ #define __LINUX_REGULATOR_CONSUMER_H_ #include <linux/err.h> +#include <linux/suspend.h> struct device; struct notifier_block; @@ -285,6 +286,9 @@ void devm_regulator_unregister_notifier(struct regulator *regulator, void *regulator_get_drvdata(struct regulator *regulator); void regulator_set_drvdata(struct regulator *regulator, void *data); +/* regulator suspend/resume state */ +bool regulator_is_disabled_in_suspend(struct regulator *regulator, + suspend_state_t state); #else /* @@ -579,6 +583,9 @@ static inline int regulator_list_voltage(struct regulator *regulator, unsigned s return -EINVAL; } +bool regulator_is_disabled_in_suspend(struct regulator *regulator, + suspend_state_t state); + #endif static inline int regulator_set_voltage_triplet(struct regulator *regulator,