Patchwork [v2,2/3] regulator: core: add helper to check if regulator is disabled in suspend

login
register
mail settings
Submitter Claudiu Beznea
Date Jan. 8, 2019, 10:56 a.m.
Message ID <1546944944-13911-3-git-send-email-claudiu.beznea@microchip.com>
Download mbox | patch
Permalink /patch/694673/
State New
Headers show

Comments

Claudiu Beznea - Jan. 8, 2019, 10:56 a.m.
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>
---
 drivers/regulator/core.c           | 17 +++++++++++++++++
 include/linux/regulator/consumer.h |  7 +++++++
 2 files changed, 24 insertions(+)
Mark Brown - Jan. 9, 2019, 4:57 p.m.
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.
Claudiu Beznea - Jan. 10, 2019, 10:24 a.m.
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
Mark Brown - Jan. 11, 2019, 12:39 p.m.
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.
Claudiu Beznea - Jan. 11, 2019, 2:08 p.m.
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.
>
Mark Brown - Jan. 14, 2019, 10:53 p.m.
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.
Claudiu Beznea - Jan. 15, 2019, 9:19 a.m.
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.  
>

Patch

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,