On Friday 17 of May 2013 12:24:04 Doug Anderson wrote:
> Tomasz,
>
> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
> > This patch makes legacy code on suspend/resume path being executed
> > conditionally, on non-DT platforms only, to fix suspend/resume of
> > DT-enabled systems, for which the code is inappropriate.
> >
> > Signed-off-by: Tomasz Figa <t.figa@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> >
> > arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c
> > index 53210ec..8ac2b2d 100644
> > --- a/arch/arm/plat-samsung/pm.c
> > +++ b/arch/arm/plat-samsung/pm.c
> > @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state)
> >
> > * require a full power-cycle)
> >
> > */
> >
> > - if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
> > + if (!of_have_populated_dt() &&
> > + !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
>
> I'd rather see you modify patch set #2 to provide some function to
> retrieve just the eint mask and then use it here. Your patch removes
> this test and doesn't replace it with anything. If you moved this
> test to pinctrl directly you'd lose the test against intallow.
Well, looking from the perspective of status before my patch, it just
bypasses a test that is incorrect on DT-enabled platforms.
I agree that this test is rather reasonable to have, but it would require
defining a new interface and moving all platforms to it, which for now
would be a bit of overkill.
IMHO a separate series that sanitizes the whole PM handling in plat-
samsung, including a rework of this check to make it cover all platforms
in a generic and multiplatform-friendly way. What do you think?
> ...or do you think this test is no longer useful for some reason?
>
> > !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow))
> > {
> >
> > printk(KERN_ERR "%s: No wake-up sources!\n",
> > __func__);
> > printk(KERN_ERR "%s: Aborting sleep\n", __func__);
> >
> > @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state)
> >
> > /* save all necessary core registers not covered by the
> > drivers */
> >
> > - samsung_pm_save_gpios();
> > - samsung_pm_saved_gpios();
> > + if (!of_have_populated_dt()) {
> > + samsung_pm_save_gpios();
> > + samsung_pm_saved_gpios();
> > + }
> > +
>
> Ah, the important part here is the "saved", not the "save"! The
> "save" should get a NULL chip for all GPIOs and effectively be a
> no-op.
>
> Skipping the "saved" is important of s3c64xx and s5p64x0 where the
> "saved" seems to transition things over to powerdown mode. Hopefully
> a future patch of yours adds that back for those platforms in the
> pinmux world. ...same for restore.
S3C64xx can be switched to power down pin configuration manually, but if
you don't do it, it will activate it automatically after entering sleep
mode.
> Summary: I've tested this on exynos5250-snow and it's reasonable but
> I'd love a response about the missing test before adding Reviewed-by.
Thanks.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
