Date:	Wed, 18 May 2011 14:58:36 +0900
From:	MyungJoo Ham <myungjoo.ham@...sung.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	Greg Kroah-Hartman <gregkh@...e.de>, kyungmin.park@...sung.com
Subject: Re: [PATCH v4 2/2] PM / Core: partial resume/suspend API for
 suspend_again users.

2011/5/18 Rafael J. Wysocki <rjw@...k.pl>:
> On Tuesday, May 17, 2011, MyungJoo Ham wrote:
>> A usage example is:
>>
>> bool example_suspend_again(void)
>> {
>>       int error, monitor_result;
>>
>>       if (!wakeup_reason_is_polling())
>>               return false;
>>
>>       dpm_partial_resume(PMSG_RESUME, devs, ARRAY_SIZE(devs));
>
> I'm not sure you need the first argument here.  Also, if the array is
> NULL terminated, you won't need the third one.
>

It was to pass the parameter to device_resume(), pm_dev_err(),
device_complete(), and device_prepare(), which have been using the
state value. However, as it is supposed to be used in the context of
suspend_devices_and_enter(), I'll remove it and assume PMSG_SUSPEND
and PMSG_RESUME are in effect.

For the array style, do we generally prefer to use NULL at the end
rather than supplying the size? If so, I'll change the array style.

>>       resume_console();
>>
>>       monitor_result = monitor_something();
>>
>>       suspend_console();
>>       error = dpm_partial_suspend(PMSG_SUSPEND, devs, ARRAY_SIZE(devs));
>
> Same here.
>
>>       if (error || monitor_result == ERROR)
>>               return false;
>>
>>       return true;
>> }
>
> That said, I don't like this patch.  The reason is that you call
> suspend_enter() is a loop and it calls the _noirq() callbacks for
> _all_ devices and now you're going to only call .resume() and
> .suspend() for several selected devices.  This is not too consistent.
>
> I wonder if those devices needed for .suspend_again() to work on
> your platform could be resumed and suspended by their drivers'
> _noirq() callbacks?

For now, we need to access PMIC, Fuel-Gauge, Charger (implemented as
regulators), RTC, ADC (w/ hwmon), and UART(drivers/tty/serial).
For PMIC, Fuel-Gauge, Charger, and RTC, they will work with no_irq
callbacks without any modification.
ADC will work if we just let it suspend and resume with no_irq callbacks.

However, for UART/serial, although it is only used for debugging, it
wouldn't be easy and clean to enable only with no_irq callbacks. If we
can keep observing the console with suspend_console_enabled=0, it
would be much helpful.
In order to let some UART/serial drivers work with no-irq callbacks,
we'd need reconstruction in drivers/tty/serial/serial_core.c, too.

Anyway, as the effect of not having partial suspend/resume is limited
for now (support for UART/serial can wait), I may defer this part and
wait for better and clean methods. And yes, because of the possibility
of having dependencies between devices, the inconsistency you've
mentioned could be an issue for some systems. However, because such
dependencies are not explicitly expressed to kernel while the platform
should know about them, partial_suspend/resume was supposed to be
called by the platform's suspend_again ops.

>
> Rafael
>
>


Cheers!

- MyungJoo
-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists