Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Kevin Hilman <khilman@deeprootsystems.com>]

Paul Walmsley <paul@pwsan.com> writes:

> On Tue, 1 Jun 2010, Kevin Hilman wrote:
>
>> "Nayak, Rajendra" <rnayak@ti.com> writes:
>> 
>> [...]
>> 
>> >> diff --git a/arch/arm/mach-omap2/pm_bus.c 
>> >> b/arch/arm/mach-omap2/pm_bus.c
>> >> index 69acaa5..3787da8 100644
>> >> --- a/arch/arm/mach-omap2/pm_bus.c
>> >> +++ b/arch/arm/mach-omap2/pm_bus.c
>> >> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev)
>> >>  };
>> >>  #endif /* CONFIG_PM_RUNTIME */
>> >>  
>> >> +#ifdef CONFIG_SUSPEND
>> >> +int platform_pm_suspend_noirq(struct device *dev)
>> >> +{
>> >> +	struct device_driver *drv = dev->driver;
>> >> +	struct platform_device *pdev = to_platform_device(dev);
>> >> +	struct omap_device *odev = to_omap_device(pdev);
>> >> +	int ret = 0;
>> >> +
>> >> +	if (!drv)
>> >> +		return 0;
>> >> +
>> >> +	if (drv->pm) {
>> >> +		if (drv->pm->suspend_noirq)
>> >> +			ret = drv->pm->suspend_noirq(dev);
>> >> +	}
>> >> +
>> >> +	if (omap_device_is_valid(odev)) {
>> >> +		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
>> >> +			dev_dbg(dev, "no automatic bus-level 
>> >> system resume.\n");
>> >> +			return 0;
>> >> +		}
>> >> +
>> >> +		dev_dbg(dev, "%s\n", __func__);
>> >> +		omap_device_idle(pdev);
>> >
>> > Is it expected that a device is always in enabled state at this point?
>> > If the device is already in idle a call to omap_device_idle unconditionally
>> > throws up warnings from the omap_device api.
>> 
>> Hmm, good point.  The device may already be idled (via runtime PM, or
>> maybe because it was never enabled.)
>> 
>> There are two options:
>> 
>> 1. fixup the warnings in the omap_device_idle() to allow multiple
>>    calls to _idle()
>> 
>> 2. Add an omap_device_is_idle() check before calling _idle()
>> 
>> I much prefer (1).  
>> 
>> Paul?
>
> As far as I can tell, it's not safe for upper-layer code to idle a device 
> like this.  The driver itself needs to be aware of the device's idle 
> state.  

The driver is made aware using the standard dev_pm_ops callback for
suspend and resume.

Note that this is not just any upper-layer code that is blindly idling
a device.  This is only happening at the system-suspend time, and in
particular this is happening using the _noirq() versions of the
dev_pm_ops hooks.

> For example, if the driver has exported some symbols, and some 
> other code is calling one of those functions, it's the driver code that 
> needs to be aware of its own idle-state and to return some kind of error 
> if the device is quiesced.  I don't think there's an easy way for 
> upper-layer code to do that.

Drivers already have to handle this today.  If they have been
suspended and their functions have been called, they have to return
errors.  This patch doesn't change that.

Kevin