Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
Hello Mark, On 10/20/2014 09:56 PM, Mark Brown wrote: > On Mon, Oct 20, 2014 at 09:50:57PM +0200, Javier Martinez Canillas wrote: >> On 10/20/2014 07:36 PM, Doug Anderson wrote: > >> > I guess I was just trying to follow the suggestion that was in the >> > regulator code: >> > http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699 >> > that says "This will usually be called by machine suspend code prior >> > to supending." > >> I see, but still I feel as if it may be a lot of duplication since most >> platforms will likely want to call the regulator core suspend prepare > > Note that architectures are an example of a platform. It really depends > what's responsible for final poweroff, we want this called as late as we > possibly can. > Got it. Thanks for the explanation. >> and finish functions. Maybe it can be added as a Kconfig option so each >> platform can choose at the config level if they want those to be called? > > No, that's obviously not going to do anything useful for multiplatform. > Ok, then let's keep to do it per-platform as is proposed on $subject for Exynos and what Chris proposed for Rockchip in [0] since it seems that's the place where these calls belong. Best regards, Javier [0]: http://www.spinics.net/lists/devicetree/msg53640.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
On Mon, Oct 20, 2014 at 09:50:57PM +0200, Javier Martinez Canillas wrote: > On 10/20/2014 07:36 PM, Doug Anderson wrote: > > I guess I was just trying to follow the suggestion that was in the > > regulator code: > > http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699 > > that says "This will usually be called by machine suspend code prior > > to supending." > I see, but still I feel as if it may be a lot of duplication since most > platforms will likely want to call the regulator core suspend prepare Note that architectures are an example of a platform. It really depends what's responsible for final poweroff, we want this called as late as we possibly can. > and finish functions. Maybe it can be added as a Kconfig option so each > platform can choose at the config level if they want those to be called? No, that's obviously not going to do anything useful for multiplatform. signature.asc Description: Digital signature
Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
[adding Rafael Wysocki to cc as Suspend-to-RAM maintainer] On 10/20/2014 07:36 PM, Doug Anderson wrote: > Javier, > > On Mon, Oct 20, 2014 at 9:58 AM, Javier Martinez Canillas > wrote: >>> It turns out that regulator_suspend_finish() actually returns an error >>> code. Could you print a warning if you see it? >>> >> >> Yes, I noticed this when looking at Chris patch for Rockchip but didn't >> re-spin >> because I'm not sure anymore if this is the right solution. I mean, if is >> correct to add the same calls on every platform or if the regulator suspend >> prepare and finish functions should be called from the suspend core instead. >> >> For example calling regulator_suspend_prepare() from >> platform_suspend_prepare() >> [0] will have the advantage of passing the correct suspend_state_t state >> instead >> of hard-coding PM_SUSPEND_MEM and will make the regulator suspend states to >> work >> on all platforms. > > Yes. If we can get this added to the core that would be better. > Agreed, let's see what Rafael says about it. > I guess I was just trying to follow the suggestion that was in the > regulator code: > http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699 > that says "This will usually be called by machine suspend code prior > to supending." > > -Doug > I see, but still I feel as if it may be a lot of duplication since most platforms will likely want to call the regulator core suspend prepare and finish functions. Maybe it can be added as a Kconfig option so each platform can choose at the config level if they want those to be called? Best regard, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
Javier, On Mon, Oct 20, 2014 at 9:58 AM, Javier Martinez Canillas wrote: >> It turns out that regulator_suspend_finish() actually returns an error >> code. Could you print a warning if you see it? >> > > Yes, I noticed this when looking at Chris patch for Rockchip but didn't > re-spin > because I'm not sure anymore if this is the right solution. I mean, if is > correct to add the same calls on every platform or if the regulator suspend > prepare and finish functions should be called from the suspend core instead. > > For example calling regulator_suspend_prepare() from > platform_suspend_prepare() > [0] will have the advantage of passing the correct suspend_state_t state > instead > of hard-coding PM_SUSPEND_MEM and will make the regulator suspend states to > work > on all platforms. Yes. If we can get this added to the core that would be better. I guess I was just trying to follow the suggestion that was in the regulator code: http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699 that says "This will usually be called by machine suspend code prior to supending." -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
[adding Chris Zong as cc who posted a similar patch for Rockchip]
Hello Doug,
On 10/20/2014 06:26 PM, Doug Anderson wrote:
> Javier,
>
> On Thu, Oct 16, 2014 at 3:13 AM, Javier Martinez Canillas
> wrote:
>> The regulator framework has a set of helpers functions to be used when
>> the system is entering and leaving from suspend but these are not called
>> on Exynos platforms. This means that the .set_suspend_* function handlers
>> defined by regulator drivers are not called when the system is suspended.
>>
>> Suggested-by: Doug Anderson
>> Signed-off-by: Javier Martinez Canillas
>> ---
>> arch/arm/mach-exynos/suspend.c | 18 ++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
>> index cc8d237..ee9a8e0 100644
>> --- a/arch/arm/mach-exynos/suspend.c
>> +++ b/arch/arm/mach-exynos/suspend.c
>> @@ -20,6 +20,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> @@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
>>
>> static int exynos_suspend_prepare(void)
>> {
>> + int ret;
>> +
>> + /*
>> +* REVISIT: It would be better if struct platform_suspend_ops
>> +* .prepare handler get the suspend_state_t as a parameter to
>> +* avoid hard-coding the suspend to mem state. It's safe to do
>> +* it now only because the suspend_valid_only_mem function is
>> +* used as the .valid callback used to check if a given state
>> +* is supported by the platform anyways.
>> +*/
>> + ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
>> + if (ret) {
>> + pr_err("Failed to prepare regulators for system suspend\n");
>> + return ret;
>> + }
>> +
>> s3c_pm_check_prepare();
>>
>> return 0;
>> @@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
>> static void exynos_suspend_finish(void)
>> {
>> s3c_pm_check_cleanup();
>> + regulator_suspend_finish();
>
> It turns out that regulator_suspend_finish() actually returns an error
> code. Could you print a warning if you see it?
>
Yes, I noticed this when looking at Chris patch for Rockchip but didn't re-spin
because I'm not sure anymore if this is the right solution. I mean, if is
correct to add the same calls on every platform or if the regulator suspend
prepare and finish functions should be called from the suspend core instead.
For example calling regulator_suspend_prepare() from platform_suspend_prepare()
[0] will have the advantage of passing the correct suspend_state_t state instead
of hard-coding PM_SUSPEND_MEM and will make the regulator suspend states to work
on all platforms.
> Other than that, feel free to add my Reviewed-by. Thanks!
>
> -Doug
>
Best regards,
Javier
[0]: http://lxr.free-electrons.com/source/kernel/power/suspend.c#L141
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
Javier,
On Thu, Oct 16, 2014 at 3:13 AM, Javier Martinez Canillas
wrote:
> The regulator framework has a set of helpers functions to be used when
> the system is entering and leaving from suspend but these are not called
> on Exynos platforms. This means that the .set_suspend_* function handlers
> defined by regulator drivers are not called when the system is suspended.
>
> Suggested-by: Doug Anderson
> Signed-off-by: Javier Martinez Canillas
> ---
> arch/arm/mach-exynos/suspend.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index cc8d237..ee9a8e0 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
>
> static int exynos_suspend_prepare(void)
> {
> + int ret;
> +
> + /*
> +* REVISIT: It would be better if struct platform_suspend_ops
> +* .prepare handler get the suspend_state_t as a parameter to
> +* avoid hard-coding the suspend to mem state. It's safe to do
> +* it now only because the suspend_valid_only_mem function is
> +* used as the .valid callback used to check if a given state
> +* is supported by the platform anyways.
> +*/
> + ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
> + if (ret) {
> + pr_err("Failed to prepare regulators for system suspend\n");
> + return ret;
> + }
> +
> s3c_pm_check_prepare();
>
> return 0;
> @@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
> static void exynos_suspend_finish(void)
> {
> s3c_pm_check_cleanup();
> + regulator_suspend_finish();
It turns out that regulator_suspend_finish() actually returns an error
code. Could you print a warning if you see it?
Other than that, feel free to add my Reviewed-by. Thanks!
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
Hi Javier,
On 10/16/2014 07:13 PM, Javier Martinez Canillas wrote:
> The regulator framework has a set of helpers functions to be used when
> the system is entering and leaving from suspend but these are not called
> on Exynos platforms. This means that the .set_suspend_* function handlers
> defined by regulator drivers are not called when the system is suspended.
>
> Suggested-by: Doug Anderson
> Signed-off-by: Javier Martinez Canillas
> ---
> arch/arm/mach-exynos/suspend.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index cc8d237..ee9a8e0 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
>
> static int exynos_suspend_prepare(void)
> {
> + int ret;
> +
> + /*
> + * REVISIT: It would be better if struct platform_suspend_ops
> + * .prepare handler get the suspend_state_t as a parameter to
> + * avoid hard-coding the suspend to mem state. It's safe to do
> + * it now only because the suspend_valid_only_mem function is
> + * used as the .valid callback used to check if a given state
> + * is supported by the platform anyways.
> + */
> + ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
> + if (ret) {
> + pr_err("Failed to prepare regulators for system suspend\n");
> + return ret;
> + }
> +
> s3c_pm_check_prepare();
>
> return 0;
> @@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
> static void exynos_suspend_finish(void)
> {
> s3c_pm_check_cleanup();
> + regulator_suspend_finish();
> }
>
> static const struct platform_suspend_ops exynos_suspend_ops = {
>
Looks good to me.
Reviewed-by: Chanwoo Choi
Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions
The regulator framework has a set of helpers functions to be used when
the system is entering and leaving from suspend but these are not called
on Exynos platforms. This means that the .set_suspend_* function handlers
defined by regulator drivers are not called when the system is suspended.
Suggested-by: Doug Anderson
Signed-off-by: Javier Martinez Canillas
---
arch/arm/mach-exynos/suspend.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index cc8d237..ee9a8e0 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
@@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
static int exynos_suspend_prepare(void)
{
+ int ret;
+
+ /*
+* REVISIT: It would be better if struct platform_suspend_ops
+* .prepare handler get the suspend_state_t as a parameter to
+* avoid hard-coding the suspend to mem state. It's safe to do
+* it now only because the suspend_valid_only_mem function is
+* used as the .valid callback used to check if a given state
+* is supported by the platform anyways.
+*/
+ ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
+ if (ret) {
+ pr_err("Failed to prepare regulators for system suspend\n");
+ return ret;
+ }
+
s3c_pm_check_prepare();
return 0;
@@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
static void exynos_suspend_finish(void)
{
s3c_pm_check_cleanup();
+ regulator_suspend_finish();
}
static const struct platform_suspend_ops exynos_suspend_ops = {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
