Re: [PATCH] i2c: Fix bus-level power management callbacks

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Rafael,

On Fri, 19 Mar 2010 23:44:08 +0100, Rafael J. Wysocki wrote:
> I assume the lack of responses except for the Alan's one means the patch
> wasn't correct, so below is one that I think is better.  It fixes all of the
> issues described above without breaking backwards compatibility.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> Subject: i2c: Fix bus-level power management callbacks
> 
> There are three issues with the i2c bus type's power management
> callbacks at the moment.  First, they don't include any hibernate
> callbacks, although they should at least include the .restore()
> callback (there's no guarantee that the driver will be present in
> memory before loading the image kernel and we must restore the
> pre-hibernation state of the device).  Second, the "legacy"
> callbacks are not going to be invoked by the PM core since the bus
> type's pm object is not NULL.  Finally, the system sleep PM
> (ie. suspend/resume) callbacks don't check if the device has been
> already suspended at run time, in which case they should skip
> suspending it.  Also, it looks like the i2c bus type can use the
> generic subsystem-level runtime PM callbacks.
> 
> For these reasons, rework the system sleep PM callbacks provided by
> the i2c bus type to handle hibernation correctly and to invoke the
> "legacy" callbacks for drivers that provide them.  In addition to
> that make the i2c bus type use the generic subsystem-level runtime
> PM callbacks.
> 
> Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>

Mark, you contributed the initial runtime PM support for the i2c
subsystem, I thought you would have comments on Rafael's
reimplementation?

> ---
>  drivers/i2c/i2c-core.c     |  168 ++++++++++++++++++++++++++-------------------
>  include/linux/pm_runtime.h |    7 +

I am a little surprised to see changes to a generic header file here,
how is the i2c subsystem so special that we have needs other subsystems
did not?

>  2 files changed, 105 insertions(+), 70 deletions(-)
> 
> Index: linux-2.6/drivers/i2c/i2c-core.c
> ===================================================================
> --- linux-2.6.orig/drivers/i2c/i2c-core.c
> +++ linux-2.6/drivers/i2c/i2c-core.c
> @@ -156,107 +156,131 @@ static void i2c_device_shutdown(struct d
>  		driver->shutdown(client);
>  }
>  
> -#ifdef CONFIG_SUSPEND
> -static int i2c_device_pm_suspend(struct device *dev)
> +static int i2c_legacy_suspend(struct device *dev, pm_message_t mesg)
>  {
> -	const struct dev_pm_ops *pm;
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_driver *driver;
>  
> -	if (!dev->driver)
> +	if (!client || !dev->driver)
>  		return 0;
> -	pm = dev->driver->pm;
> -	if (!pm || !pm->suspend)
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->suspend)
>  		return 0;
> -	return pm->suspend(dev);
> +	return driver->suspend(client, mesg);
>  }
>  
> -static int i2c_device_pm_resume(struct device *dev)
> +static int i2c_legacy_resume(struct device *dev)
>  {
> -	const struct dev_pm_ops *pm;
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_driver *driver;
>  
> -	if (!dev->driver)
> +	if (!client || !dev->driver)
>  		return 0;
> -	pm = dev->driver->pm;
> -	if (!pm || !pm->resume)
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->resume)
>  		return 0;
> -	return pm->resume(dev);
> +	return driver->resume(client);
>  }
> -#else
> -#define i2c_device_pm_suspend	NULL
> -#define i2c_device_pm_resume	NULL
> -#endif

I fail to see why the functions above are outside of the #ifdef
CONFIG_PM_SLEEP scope. They are only called by functions which are
inside the #ifdef CONFIG_PM_SLEEP scope, so you'll get a build-time
warning if CONFIG_PM_SLEEP isn't set.

Is there a plan to get rid of the above legacy functions at some point
in time?

>  
> -#ifdef CONFIG_PM_RUNTIME
> -static int i2c_device_runtime_suspend(struct device *dev)
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_device_pm_suspend(struct device *dev)
>  {
> -	const struct dev_pm_ops *pm;
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> -	if (!dev->driver)
> -		return 0;
> -	pm = dev->driver->pm;
> -	if (!pm || !pm->runtime_suspend)
> +	if (pm_runtime_suspended(dev))
>  		return 0;
> -	return pm->runtime_suspend(dev);
> -}
> (...)

Apart from the above, the code looks sane to me, but then again I don't
know a thing about power management. I'll keep this patch in my i2c
tree, scheduled for merge in 2.6.35. If there are any updates, please
send them over, either as a new patch or as incremental changes which I
will merge myself.

-- 
Jean Delvare