[i2c] [patch 1/5] i2c stack can probe()

[Jean Delvare]

Hi David,

On Thu, 15 Feb 2007 00:41:37 -0800, David Brownell wrote:
> One of a series of I2C infrastructure updates to support enumeration using
> the standard Linux driver model.
> 
> This patch updates probe() and associated hotplug/coldplug support, but
> not remove().  Nothing yet _uses_ it to create I2C devices, so those
> hotplug/coldplug mechanisms will be the only externally visible change.
> This patch will be an overall NOP since the I2C stack doesn't yet create
> clients/devices except as part of binding them to legacy drivers.
> 
> Some code is moved earlier in the source code, helping group more of the
> per-device infrastructure in one place and simplifying handling per-device
> attributes.
> 
> Terminology being adopted:  "legacy drivers" create devices (i2c_client)
> themselves, while "new style" ones follow the driver model (the i2c_client
> is handed to the probe routine).  It's an either/or thing; the two models
> don't mix, and drivers that try mixing them won't even be registered.
> 
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> 
> ---
>  drivers/i2c/i2c-core.c |  146 ++++++++++++++++++++++++++++++++-----------------
>  include/linux/i2c.h    |   10 ++-
>  2 files changed, 105 insertions(+), 51 deletions(-)
> 
> Index: i2c/include/linux/i2c.h
> ===================================================================
> --- i2c.orig/include/linux/i2c.h	2007-02-14 23:59:56.000000000 -0800
> +++ i2c/include/linux/i2c.h	2007-02-15 00:00:03.000000000 -0800
> @@ -113,7 +113,7 @@ struct i2c_driver {
>  	 * can be used by the driver to test if the bus meets its conditions
>  	 * & seek for the presence of the chip(s) it supports. If found, it
>  	 * registers the client(s) that are on the bus to the i2c admin. via
> -	 * i2c_attach_client.
> +	 * i2c_attach_client.  (LEGACY I2C DRIVERS ONLY)
>  	 */
>  	int (*attach_adapter)(struct i2c_adapter *);
>  	int (*detach_adapter)(struct i2c_adapter *);
> @@ -121,10 +121,16 @@ struct i2c_driver {
>  	/* tells the driver that a client is about to be deleted & gives it
>  	 * the chance to remove its private data. Also, if the client struct
>  	 * has been dynamically allocated by the driver in the function above,
> -	 * it must be freed here.
> +	 * it must be freed here.  (LEGACY I2C DRIVERS ONLY)
>  	 */
>  	int (*detach_client)(struct i2c_client *);
>  
> +	/* Standard driver model interfaces, for "new style" i2c drivers.
> +	 * With the driver model, device enumeration is NEVER done by drivers;
> +	 * it's done by infrastructure.  (NEW STYLE DRIVERS ONLY)
> +	 */
> +	int (*probe)(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);
> Index: i2c/drivers/i2c/i2c-core.c
> ===================================================================
> --- i2c.orig/drivers/i2c/i2c-core.c	2007-02-14 23:59:56.000000000 -0800
> +++ i2c/drivers/i2c/i2c-core.c	2007-02-15 00:00:03.000000000 -0800
> @@ -43,15 +43,58 @@ 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;
> +	struct i2c_client	*client = to_i2c_client(dev);
> +	struct i2c_driver	*driver = to_i2c_driver(drv);
> +
> +	/* legacy i2c drivers bypass driver model probing entirely;
> +	 * such drivers scan each i2c adapter/bus themselves.
> +	 */
> +	if (!driver->probe)
> +		return 0;

Just to clarify things... This function will never actually be called
for legacy drivers, will it?

> +
> +	/* new style drivers use the same driver matching policy as
> +	 * platform devices or SPI:  compare device and driver names.
> +	 */
> +	return strcmp(client->name, drv->name) == 0;
>  }
>  
> +#ifdef	CONFIG_HOTPLUG
> +
> +/* uevent helps with hotplug: modprobe -q $(MODALIAS) */
> +static int i2c_device_uevent(struct device *dev, char **envp, int num_envp,
> +		      char *buffer, int buffer_size)
> +{
> +	struct i2c_client	*client = to_i2c_client(dev);
> +	int			i = 0, length = 0;
> +
> +	if (!dev)
> +		return -ENODEV;

Can this realistically happen? I fail to see how.

> +
> +	if (add_uevent_var(envp, num_envp, &i, buffer, buffer_size, &length,
> +			"MODALIAS=%s", client->name))
> +		return -ENOMEM;
> +	envp[i] = NULL;
> +	return 0;
> +}
> +
> +#else
> +#define i2c_device_uevent	NULL
> +#endif	/* CONFIG_HOTPLUG */
> +
>  static int i2c_device_probe(struct device *dev)
>  {
> -	return -ENODEV;
> +	struct i2c_client	*client = to_i2c_client(dev);
> +	struct i2c_driver	*driver;
> +
> +	if (!dev->driver)
> +		return -ENODEV;

Same question here, can this realistically happen?

> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->probe)
> +		return -ENODEV;
> +	client->driver = driver;
> +	return driver->probe(client);
>  }
>  
>  static int i2c_device_remove(struct device *dev)
> @@ -94,9 +137,30 @@ static int i2c_device_resume(struct devi
>  	return driver->resume(to_i2c_client(dev));
>  }
>  
> +static void i2c_client_release(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	complete(&client->released);
> +}
> +
> +static ssize_t show_client_name(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%s\n", client->name);
> +}
> +
> +static /*const*/ struct device_attribute i2c_dev_attrs[] = {

What's this commented out "const"?

> +	__ATTR(name, S_IRUGO, show_client_name, NULL),
> +	/* modalias helps coldplug:  modprobe $(cat .../modalias) */
> +	__ATTR(modalias, S_IRUGO, show_client_name, NULL),
> +	{ },
> +};

This means we will create the modalias file for both legacy and new
style i2c clients. Is it OK?

> +
>  struct bus_type i2c_bus_type = {
>  	.name		= "i2c",
> +	.dev_attrs	= i2c_dev_attrs,

Nice cleanup :)

>  	.match		= i2c_device_match,
> +	.uevent		= i2c_device_uevent,
>  	.probe		= i2c_device_probe,
>  	.remove		= i2c_device_remove,
>  	.shutdown	= i2c_device_shutdown,
> @@ -129,31 +193,6 @@ static ssize_t show_adapter_name(struct 
>  static DEVICE_ATTR(name, S_IRUGO, show_adapter_name, NULL);
>  
>  
> -static void i2c_client_release(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	complete(&client->released);
> -}
> -
> -static ssize_t show_client_name(struct device *dev, struct device_attribute *attr, char *buf)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	return sprintf(buf, "%s\n", client->name);
> -}
> -
> -/*
> - * We can't use the DEVICE_ATTR() macro here, as we used the same name for
> - * an i2c adapter attribute (above).
> - */
> -static struct device_attribute dev_attr_client_name =
> -	__ATTR(name, S_IRUGO, &show_client_name, NULL);
> -
> -
> -/* ---------------------------------------------------
> - * registering functions
> - * ---------------------------------------------------
> - */
> -
>  static int i2c_do_add_adapter(struct device_driver *d, void *data)
>  {
>  	struct i2c_driver *driver = to_i2c_driver(d);
> @@ -219,7 +258,7 @@ int i2c_add_adapter(struct i2c_adapter *
>  
>  	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
>  
> -	/* let drivers scan this bus for matching devices */
> +	/* let legacy drivers scan this bus for matching devices */
>  	res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
>  			       i2c_do_add_adapter);
>  
> @@ -312,39 +351,54 @@ int i2c_del_adapter(struct i2c_adapter *
>  }
>  
>  
> -/* -----
> - * What follows is the "upwards" interface: commands for talking to clients,
> - * which implement the functions to access the physical information of the
> - * chips.
> +/* ------------------------------------------------------------------------- */
> +
> +/*
> + * An i2c_driver is used with one or more i2c_client (device) nodes to access
> + * i2c slave chips, on a bus instance associated with some i2c_adapter.  There
> + * are two models for binding the driver to its device:  "new style" drivers
> + * follow the standard Linux driver model and just respond to probe() calls
> + * issued if the driver core sees they match(); "legacy" drivers create device
> + * nodes themselves.
>   */
>  
>  int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
>  {
> -	struct list_head   *item;
> -	struct i2c_adapter *adapter;
>  	int res;
>  
> +	/* new style driver methods can't mix with legacy ones */
> +	if (driver->probe) {
> +		if (driver->attach_adapter || driver->detach_adapter
> +				|| driver->detach_client) {
> +			pr_debug("i2c-core: driver [%s] is confused\n",
> +					driver->driver.name);
> +			return -EINVAL;
> +		}
> +	}

Very good idea, I like it. But I believe this deserves at the very
least a KERN_NOTICE-level message, not just debugging. KERN_WARN or
KERN_ERR might even be more suitable.

> +
>  	/* add the driver to the list of i2c drivers in the driver core */
>  	driver->driver.owner = owner;
>  	driver->driver.bus = &i2c_bus_type;
>  
>  	res = driver_register(&driver->driver);
> +	pr_debug("i2c-core: register driver [%s] --> %d\n",
> +			driver->driver.name, res);
>  	if (res)
>  		return res;
>  
> -	mutex_lock(&core_lists);
> -
> -	pr_debug("i2c-core: driver [%s] registered\n", driver->driver.name);

I don't like this change. I understand that you want errors to be
reported, but having a single string for success and error is somewhat
confusing. I propose that you leave the current debug message in place
for the success case, and add a separate KERN_ERR-level message for the
error case. OK?

> -
> -	/* now look for instances of driver on our adapters */
> +	/* legacy drivers scan i2c busses directly */
>  	if (driver->attach_adapter) {
> +		struct list_head   *item;
> +		struct i2c_adapter *adapter;
> +
> +		mutex_lock(&core_lists);
>  		list_for_each(item,&adapters) {
>  			adapter = list_entry(item, struct i2c_adapter, list);
>  			driver->attach_adapter(adapter);
>  		}
> +		mutex_unlock(&core_lists);
>  	}

I like this locking and local variables cleanup, however it really
doesn't belong to that patch. Can you please provide a separate patch
instead?

>  
> -	mutex_unlock(&core_lists);
>  	return 0;
>  }
>  EXPORT_SYMBOL(i2c_register_driver);
> @@ -399,6 +453,8 @@ int i2c_del_driver(struct i2c_driver *dr
>  	return 0;
>  }
>  
> +/* ------------------------------------------------------------------------- */
> +
>  static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
>  {
>  	struct list_head   *item;
> @@ -449,9 +505,6 @@ int i2c_attach_client(struct i2c_client 
>  	res = device_register(&client->dev);
>  	if (res)
>  		goto out_list;
> -	res = device_create_file(&client->dev, &dev_attr_client_name);
> -	if (res)
> -		goto out_unregister;
>  	mutex_unlock(&adapter->clist_lock);
>  
>  	if (adapter->client_register)  {
> @@ -464,10 +517,6 @@ int i2c_attach_client(struct i2c_client 
>  
>  	return 0;
>  
> -out_unregister:
> -	init_completion(&client->released); /* Needed? */
> -	device_unregister(&client->dev);
> -	wait_for_completion(&client->released);
>  out_list:
>  	list_del(&client->list);
>  	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
> @@ -502,7 +551,6 @@ int i2c_detach_client(struct i2c_client 
>  	mutex_lock(&adapter->clist_lock);
>  	list_del(&client->list);
>  	init_completion(&client->released);
> -	device_remove_file(&client->dev, &dev_attr_client_name);
>  	device_unregister(&client->dev);
>  	mutex_unlock(&adapter->clist_lock);
>  	wait_for_completion(&client->released);

Looks good overall, thanks.

-- 
Jean Delvare