From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
To: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Subject: Re: [PATCH V3 4/9] ARM: tegra: pm: add platform suspend support
Date: Wed, 13 Mar 2013 12:03:06 -0600 [thread overview]
Message-ID: <5140BF5A.3040108@wwwdotorg.org> (raw)
In-Reply-To: <1363161683-20825-5-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 03/13/2013 02:01 AM, Joseph Lo wrote:
> Adding suspend to RAM support for Tegra platform. There are three suspend
> mode for Tegra. The difference were below.
>
> * LP2: CPU voltage off
> * LP1: CPU voltage off, DRAM in self-refresh
> * LP0: CPU + Core voltage off, DRAM in self-refresh
>
> After this patch, the LP2 suspend mode will be supported.
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> +void __init tegra_init_suspend(void)
...
> + /* We only support LP2 suspend mode right now. */
> + if (mode > TEGRA_SUSPEND_LP2)
> + tegra_pmc_set_suspend_mode(TEGRA_SUSPEND_LP2);
I see that in patch 3/9, tegra_pmc_parse_dt() simply assigns directly to
pmc_pm_data.suspend_mode rather than calling a function. If it's useful
to call a function here, isn't doing so also useful in tegra_pmc_parse_dt()?
I guess you did this because pmc_pm_data is static so you can't just
assign it here.
Instead, I would suggest that tegra_pmc_parse_dt() does this validation
itself, either by:
a) Enhancing tegra_pmc_parse_dt() to know which modes are supported itself.
or:
b) Have tegra_pmc_parse_dt() call a function in pm.c to determine
whether the user-selected suspend mode is valid or not. Something like:
pmc_pm_data.suspend_mode = tegra_pm_validate_suspend_mode(suspend_mode);
That way, tegra_pm_validate_suspend_mode() can encapsulate all details
of which suspend modes are supported, any SoC-specific differences (e.g.
not Tegra114 in any mode, but any earlier chip in LP2 mode for example).
(a) is slightly simpler. (b) is probably more flexible. I guess I'd
prefer (b).
WARNING: multiple messages have this Message-ID (diff)
From: swarren@wwwdotorg.org (Stephen Warren)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V3 4/9] ARM: tegra: pm: add platform suspend support
Date: Wed, 13 Mar 2013 12:03:06 -0600 [thread overview]
Message-ID: <5140BF5A.3040108@wwwdotorg.org> (raw)
In-Reply-To: <1363161683-20825-5-git-send-email-josephl@nvidia.com>
On 03/13/2013 02:01 AM, Joseph Lo wrote:
> Adding suspend to RAM support for Tegra platform. There are three suspend
> mode for Tegra. The difference were below.
>
> * LP2: CPU voltage off
> * LP1: CPU voltage off, DRAM in self-refresh
> * LP0: CPU + Core voltage off, DRAM in self-refresh
>
> After this patch, the LP2 suspend mode will be supported.
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> +void __init tegra_init_suspend(void)
...
> + /* We only support LP2 suspend mode right now. */
> + if (mode > TEGRA_SUSPEND_LP2)
> + tegra_pmc_set_suspend_mode(TEGRA_SUSPEND_LP2);
I see that in patch 3/9, tegra_pmc_parse_dt() simply assigns directly to
pmc_pm_data.suspend_mode rather than calling a function. If it's useful
to call a function here, isn't doing so also useful in tegra_pmc_parse_dt()?
I guess you did this because pmc_pm_data is static so you can't just
assign it here.
Instead, I would suggest that tegra_pmc_parse_dt() does this validation
itself, either by:
a) Enhancing tegra_pmc_parse_dt() to know which modes are supported itself.
or:
b) Have tegra_pmc_parse_dt() call a function in pm.c to determine
whether the user-selected suspend mode is valid or not. Something like:
pmc_pm_data.suspend_mode = tegra_pm_validate_suspend_mode(suspend_mode);
That way, tegra_pm_validate_suspend_mode() can encapsulate all details
of which suspend modes are supported, any SoC-specific differences (e.g.
not Tegra114 in any mode, but any earlier chip in LP2 mode for example).
(a) is slightly simpler. (b) is probably more flexible. I guess I'd
prefer (b).
next prev parent reply other threads:[~2013-03-13 18:03 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-13 8:01 [PATCH V3 0/9] ARM: tegra: add platform suspend support Joseph Lo
2013-03-13 8:01 ` Joseph Lo
[not found] ` <1363161683-20825-1-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-03-13 8:01 ` [PATCH V3 1/9] gpio: tegra: add gpio wakeup source handling Joseph Lo
2013-03-13 8:01 ` Joseph Lo
[not found] ` <1363161683-20825-2-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-03-27 8:25 ` Linus Walleij
2013-03-27 8:25 ` Linus Walleij
2013-03-13 8:01 ` [PATCH V3 2/9] ARM: tegra: irq: add wake up handling Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 8:01 ` [PATCH V3 3/9] ARM: dt: tegra: add bindings of power management configurations for PMC Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 8:01 ` [PATCH V3 4/9] ARM: tegra: pm: add platform suspend support Joseph Lo
2013-03-13 8:01 ` Joseph Lo
[not found] ` <1363161683-20825-5-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-03-13 18:03 ` Stephen Warren [this message]
2013-03-13 18:03 ` Stephen Warren
2013-03-13 8:01 ` [PATCH V3 5/9] ARM: dts: tegra: add power gpio keys to DT Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 8:01 ` [PATCH V3 6/9] ARM: dts: tegra: whistler: add wakeup source for KBC Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 8:01 ` [PATCH V3 7/9] ARM: dts: tegra: add non-removable and keep-power-in-suspend property for MMC Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 8:01 ` [PATCH V3 8/9] ARM: tegra: config: defconfig update Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 8:01 ` [PATCH V3 9/9] ARM: dts: tegra: add the PM configurations of PMC Joseph Lo
2013-03-13 8:01 ` Joseph Lo
2013-03-13 17:38 ` [PATCH V3 0/9] ARM: tegra: add platform suspend support Stephen Warren
2013-03-13 17:38 ` Stephen Warren
[not found] ` <5140B996.40400-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-03-14 1:28 ` Joseph Lo
2013-03-14 1:28 ` Joseph Lo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5140BF5A.3040108@wwwdotorg.org \
--to=swarren-3lzwwm7+weoh9zmkesr00q@public.gmane.org \
--cc=josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.