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). -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
