[i2c] [patch 2.6.20-rc1 6/6] i2c driver suspend()/resume()/shutdown() support

Jean Delvare khali at linux-fr.org
Thu Jan 4 18:38:41 CET 2007


Hi David,

On Fri, 29 Dec 2006 01:13:50 -0800, David Brownell wrote:
> Driver model updates for the I2C core:
> 
>  - Add new suspend(), resume(), and shutdown() methods.  Use them in the
>    standard driver model style; document them.

Is there any driver using this? Usually we want at least one driver to
use a feature before we add that feature to the kernel. Although here I
understand that what you're adding is pretty standard.

> 
>  - Minor doc updates to highlight zero-initialized fields in drivers, and
>    the driver model accessors for "clientdata".
> 
> If any i2c drivers were previously using the old suspend/resume calls
> in "struct driver", they were getting warning messages ... and will
> now no longer work.  Other than that, this patch changes no behaviors;
> and it lets I2C drivers use conventional PM and shutdown support.
> 
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> 
> ---
> This is a refresh of the previous version of this patch, updated to apply
> without the "good parts" of patch #5 ("#5B") ... so it applies to a 2.6.20-rc2
> kernel.
> 
>  Documentation/i2c/porting-clients |    9 +++++
>  Documentation/i2c/writing-clients |   58 ++++++++++++++++++++++++++++++++------
>  drivers/i2c/i2c-core.c            |   56 ++++++++++++++++++++++++++----------
>  include/linux/i2c.h               |    7 +++-
>  4 files changed, 104 insertions(+), 26 deletions(-)
> 
> Index: pxa/Documentation/i2c/writing-clients
> ===================================================================
> --- pxa.orig/Documentation/i2c/writing-clients	2006-12-10 01:30:38.000000000 -0800
> +++ pxa/Documentation/i2c/writing-clients	2006-12-29 00:15:31.000000000 -0800
> @@ -21,20 +21,26 @@ The driver structure
>  
>  Usually, you will implement a single driver structure, and instantiate
>  all clients from it. Remember, a driver structure contains general access 
> -routines, a client structure specific information like the actual I2C
> -address.
> +routines, and should be zero-initialized except for fields with data you
> +provide.  A client structure holds device-specific information like the
> +driver model device node, and its I2C address.
>  
>  static struct i2c_driver foo_driver = {
>  	.driver = {
>  		.name	= "foo",
>  	},
> -	.attach_adapter	= &foo_attach_adapter,
> -	.detach_client	= &foo_detach_client,
> -	.command	= &foo_command /* may be NULL */
> +	.attach_adapter	= foo_attach_adapter,
> +	.detach_client	= foo_detach_client,
> +	.shutdown	= foo_shutdown,	/* optional */
> +	.suspend	= foo_suspend,	/* optional */
> +	.resume		= foo_resume,	/* optional */
> +	.command	= foo_command,	/* optional */
>  }
>   
> -The name field must match the driver name, including the case. It must not
> -contain spaces, and may be up to 31 characters long.
> +The name field is the driver name, and must not contain spaces.  It
> +should match the module name (if the driver can be compiled as a module),
> +although you can use MODULE_ALIAS (passing "foo" in this example) to add
> +another name for the module.
>  
>  All other fields are for call-back functions which will be explained 
>  below.
> @@ -43,11 +49,18 @@ below.
>  Extra client data
>  =================
>  
> -The client structure has a special `data' field that can point to any
> -structure at all. You can use this to keep client-specific data. You
> +Each client structure has a special `data' field that can point to any
> +structure at all.  You should use this to keep device-specific data,
> +especially in drivers that handle multiple I2C or SMBUS devices.  You
>  do not always need this, but especially for `sensors' drivers, it can
>  be very useful.
>  
> +	/* store the value */
> +	void i2c_set_clientdata(struct i2c_client *client, void *data);
> +
> +	/* retrieve the value */
> +	void *i2c_get_clientdata(struct i2c_client *client);
> +
>  An example structure is below.
>  
>    struct foo_data {
> @@ -493,6 +506,33 @@ by `__init_data'.  Hose functions and st
>  kernel booting (or module loading) is completed.
>  
>  
> +Power Management
> +================
> +
> +If your I2C device needs special handling when entering a system low
> +power state -- like putting a transceiver into a low power mode, or
> +activating a system wakeup mechanism -- do that in the suspend() method.
> +The resume() method should reverse what the suspend() method does.
> +
> +These are standard driver model calls, and they work just like they
> +would for any other driver stack.  The calls can sleep, and can use
> +I2C messaging to the device being suspended or resumed (since their
> +parent I2C adapter is active when these calls are issued, and IRQs
> +are still enabled).
> +
> +
> +System Shutdown
> +===============
> +
> +If your I2C device needs special handling when the system shuts down
> +or reboots (including kexec) -- like turning something off -- use a
> +shutdown() method.
> +
> +Again, this is a standard driver model call, working just like it
> +would for any other driver stack:  the calls can sleep, and can use
> +I2C messaging.
> +
> +
>  Command function
>  ================
>  
> Index: pxa/Documentation/i2c/porting-clients
> ===================================================================
> --- pxa.orig/Documentation/i2c/porting-clients	2006-12-10 01:30:38.000000000 -0800
> +++ pxa/Documentation/i2c/porting-clients	2006-12-29 00:15:31.000000000 -0800
> @@ -129,9 +129,16 @@ Technical changes:
>    structure, those name member should be initialized to a driver name
>    string. i2c_driver itself has no name member anymore.
>  
> +* [Driver model] Instead of shutdown or reboot notifiers, provide a
> +  shutdown() method in your driver.
> +
> +* [Power management] Use the driver model suspend() and resume()
> +  callbacks instead of the obsolete pm_register() calls.
> +
>  Coding policy:
>  
> -* [Copyright] Use (C), not (c), for copyright.
> +* [Copyright] Use (C), not (c), for copyright.  Provide dates and names:
> +  "Copyright (C) 2006 you", "Copyright (C) 1998,2004-2006 company", etc

Please back this one out, it's really not specific to i2c drivers and
at any rate doesn't belong to this patch.

>  
>  * [Debug/log] Get rid of #ifdef DEBUG/#endif constructs whenever you
>    can. Calls to printk for debugging purposes are replaced by calls to
> Index: pxa/include/linux/i2c.h
> ===================================================================
> --- pxa.orig/include/linux/i2c.h	2006-12-27 13:38:46.000000000 -0800
> +++ pxa/include/linux/i2c.h	2006-12-29 00:15:31.000000000 -0800
> @@ -125,7 +125,12 @@ struct i2c_driver {
>  	 * it must be freed here.
>  	 */
>  	int (*detach_client)(struct i2c_client *);
> -	
> +
> +	/* driver model interfaces that don't relate to enumeration  */
> +	void (*shutdown)(struct i2c_client *);
> +	int (*suspend)(struct i2c_client *, pm_message_t mesg);
> +	int (*resume)(struct i2c_client *);
> +
>  	/* a ioctl like command that can be used to perform specific functions
>  	 * with the device.
>  	 */
> Index: pxa/drivers/i2c/i2c-core.c
> ===================================================================
> --- pxa.orig/drivers/i2c/i2c-core.c	2006-12-28 20:07:50.000000000 -0800
> +++ pxa/drivers/i2c/i2c-core.c	2006-12-29 00:20:37.000000000 -0800
> @@ -40,28 +40,39 @@ static LIST_HEAD(drivers);
>  static DEFINE_MUTEX(core_lists);
>  static DEFINE_IDR(i2c_adapter_idr);
>  
> -/* match always succeeds, as we want the probe() to tell if we really accept this match */
> +
> +/* ------------------------------------------------------------------------- */
> +
>  static int i2c_device_match(struct device *dev, struct device_driver *drv)
>  {
> -	return 1;
> +	/* (for now) bypass driver model probing entirely; drivers
> +	 * scan each i2c adapter/bus themselves.
> +	 */
> +	return 0;
>  }

I'm a bit confused by this one, the match was always succeeding, now
it's never succeeding and... it doesn't make any difference? This
change doesn't appear to belong to this patch, if it makes any sense at
all.

>  
> -static int i2c_bus_suspend(struct device * dev, pm_message_t state)
> +static int (struct device * dev, pm_message_t mesg)
>  {
> -	int rc = 0;
> +	struct i2c_driver	*driver;
>  
> -	if (dev->driver && dev->driver->suspend)
> -		rc = dev->driver->suspend(dev, state);
> -	return rc;
> +	if (!dev->driver)
> +		return 0;
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->suspend)
> +		return 0;
> +	return driver->suspend(to_i2c_client(dev), mesg);
>  }
>  
> -static int i2c_bus_resume(struct device * dev)
> +static int i2c_device_resume(struct device * dev)
>  {
> -	int rc = 0;
> -	
> -	if (dev->driver && dev->driver->resume)
> -		rc = dev->driver->resume(dev);
> -	return rc;
> +	struct i2c_driver	*driver;
> +
> +	if (!dev->driver)
> +		return 0;
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->resume)
> +		return 0;
> +	return driver->resume(to_i2c_client(dev));
>  }

What about moving i2c_device_suspend() and i2c_device_resume() down
after i2c_device_shutdown() to group related functions together?

>  
>  static int i2c_device_probe(struct device *dev)
> @@ -74,15 +85,29 @@ static int i2c_device_remove(struct devi
>  	return 0;
>  }
>  
> +static void i2c_device_shutdown(struct device *dev)
> +{
> +	struct i2c_driver	*driver;
> +
> +	if (!dev->driver)
> +		return;
> +	driver = to_i2c_driver(dev->driver);
> +	if (driver->shutdown)
> +		driver->shutdown(to_i2c_client(dev));
> +}
> +
>  struct bus_type i2c_bus_type = {
>  	.name =		"i2c",
>  	.match =	i2c_device_match,
>  	.probe =	i2c_device_probe,
>  	.remove =	i2c_device_remove,
> -	.suspend =      i2c_bus_suspend,
> -	.resume =       i2c_bus_resume,
> +	.shutdown =	i2c_device_shutdown,
> +	.suspend =      i2c_device_suspend,
> +	.resume =       i2c_device_resume,
>  };
>  
> +/* ------------------------------------------------------------------------- */
> +
>  void i2c_adapter_dev_release(struct device *dev)
>  {
>  	struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
>  

Thanks,
-- 
Jean Delvare



More information about the i2c mailing list