[i2c] [patch 5/5] i2c_register_adapter()

[Jean Delvare]

Hi David,

On Thu, 15 Feb 2007 00:46:43 -0800, David Brownell wrote:
> This adds a new call, i2c_register_adapter(), to register an I2C adapter
> using a specific bus number and then create I2C device nodes for any
> pre-declared devices on that bus.  It builds on previous patches adding
> I2C probe() and remove() support, and pre-declaration of devices.
> 
> There's also a minor related cleanup, using class infrastructure to
> create and remove the device attribute and associated code movement.

As usual, I will ask for a separate patch for this. This is not
specifically related to the rest of what the patch does.

> 
> This completes the basic support for "new style" I2C device drivers.
> Those follow the standard driver model for binding devices to drivers
> (using probe and remove methods) rather than a legacy model (where the
> driver tries to autoconfigure each bus, and registers devices itself).
> 
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> 
> ---
>  drivers/i2c/i2c-core.c |  156 +++++++++++++++++++++++++++++++++++--------------
>  include/linux/i2c.h    |    1 
>  2 files changed, 115 insertions(+), 42 deletions(-)
> 
> Index: i2c/include/linux/i2c.h
> ===================================================================
> --- i2c.orig/include/linux/i2c.h	2007-02-15 00:00:08.000000000 -0800
> +++ i2c/include/linux/i2c.h	2007-02-15 00:00:08.000000000 -0800
> @@ -354,6 +354,7 @@ struct i2c_client_address_data {
>   */
>  extern int i2c_add_adapter(struct i2c_adapter *);
>  extern int i2c_del_adapter(struct i2c_adapter *);
> +extern int i2c_register_adapter(struct i2c_adapter *);
>  
>  extern int i2c_register_driver(struct module *, struct i2c_driver *);
>  extern int i2c_del_driver(struct i2c_driver *);
> Index: i2c/drivers/i2c/i2c-core.c
> ===================================================================
> --- i2c.orig/drivers/i2c/i2c-core.c	2007-02-15 00:00:08.000000000 -0800
> +++ i2c/drivers/i2c/i2c-core.c	2007-02-15 00:00:08.000000000 -0800
> @@ -268,28 +268,32 @@ EXPORT_SYMBOL_GPL(i2c_unregister_device)
>  
>  /* ------------------------------------------------------------------------- */
>  
> +/* I2C bus adapters -- one roots each I2C or SMBUS segment */
> +
>  void i2c_adapter_dev_release(struct device *dev)
>  {
>  	struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
>  	complete(&adap->dev_released);
>  }
>  
> -/* ------------------------------------------------------------------------- */
> +static ssize_t
> +show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
> +	return sprintf(buf, "%s\n", adap->name);
> +}
>  
> -/* I2C bus adapters -- one roots each I2C or SMBUS segment */
> +static struct device_attribute i2c_adapter_attrs[] = {
> +	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
> +	{ },
> +};
>  
>  struct class i2c_adapter_class = {
>  	.owner			= THIS_MODULE,
>  	.name			= "i2c-adapter",
> +	.dev_attrs		= i2c_adapter_attrs,
>  };
>  
> -static ssize_t show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
> -{
> -	struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
> -	return sprintf(buf, "%s\n", adap->name);
> -}
> -static DEVICE_ATTR(name, S_IRUGO, show_adapter_name, NULL);
> -
>  
>  static int i2c_do_add_adapter(struct device_driver *d, void *data)
>  {
> @@ -306,35 +310,17 @@ static int i2c_do_add_adapter(struct dev
>  	return res;
>  }
>  
> -/* -----
> - * i2c_add_adapter is called from within the algorithm layer,
> - * when a new hw adapter registers. A new device is register to be
> - * available for clients.
> - */
> -int i2c_add_adapter(struct i2c_adapter *adap)
> +static int __i2c_register_adapter(struct i2c_adapter *adap)
>  {
> -	int id, res = 0;
> -
> -	mutex_lock(&core_lists);
> -
> -	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) {
> -		res = -ENOMEM;
> -		goto out_unlock;
> -	}
> -
> -	res = idr_get_new(&i2c_adapter_idr, adap, &id);
> -	if (res < 0) {
> -		if (res == -EAGAIN)
> -			res = -ENOMEM;
> -		goto out_unlock;
> -	}
> +	int res = 0;
>  
> -	adap->nr =  id & MAX_ID_MASK;
>  	mutex_init(&adap->bus_lock);
>  	mutex_init(&adap->clist_lock);
>  	list_add_tail(&adap->list,&adapters);
>  	INIT_LIST_HEAD(&adap->clients);
>  
> +	mutex_lock(&core_lists);
> +

Looks like a bug to me, I think you must hold the code_lists mutex for
the list_add_tail instruction above.

>  	/* Add the adapter to the driver core.
>  	 * If the parent pointer is not set up,
>  	 * we add this adapter to the host bus.
> @@ -350,9 +336,6 @@ int i2c_add_adapter(struct i2c_adapter *
>  	res = device_register(&adap->dev);
>  	if (res)
>  		goto out_list;
> -	res = device_create_file(&adap->dev, &dev_attr_name);
> -	if (res)
> -		goto out_unregister;
>  
>  	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
>  
> @@ -364,16 +347,50 @@ out_unlock:
>  	mutex_unlock(&core_lists);
>  	return res;
>  
> -out_unregister:
> -	init_completion(&adap->dev_released); /* Needed? */
> -	device_unregister(&adap->dev);
> -	wait_for_completion(&adap->dev_released);
>  out_list:
>  	list_del(&adap->list);
>  	idr_remove(&i2c_adapter_idr, adap->nr);
>  	goto out_unlock;
>  }
>  
> +/**
> + * i2c_add_adapter - declare i2c adapter, use dynamic bus number
> + * @adap: the adapter to add
> + *
> + * This routine is used to declare an I2C adapter when its bus number
> + * doesn't matter.  Examples: for I2C adapters dynamically added by
> + * USB links or PCI plugin cards.
> + *
> + * When this returns zero, a new bus number was allocated and stored
> + * in adap->nr, and the specified adapter became available for clients.
> + * Otherwise, a negative errno value is returned.
> + */
> +int i2c_add_adapter(struct i2c_adapter *adap)
> +{
> +	int	id, res = 0;
> +
> +retry:
> +	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0)
> +		return -ENOMEM;
> +
> +	mutex_lock(&core_lists);
> +	/* "above" here means "above or equal to", sigh */
> +	res = idr_get_new_above(&i2c_adapter_idr, adap,
> +				__i2c_first_dynamic_bus_num, &id);
> +	adap->nr = id;
> +	mutex_unlock(&core_lists);
> +	if (res < 0) {
> +		if (res == -EAGAIN)
> +			goto retry;
> +		return res;
> +	}

You should not set adap->nr before checking that the call to
idr_get_new_above was successful.

> +
> +	if (res == 0)

Isn't this always true?

> +		res = __i2c_register_adapter(adap);
> +	return res;
> +}
> +EXPORT_SYMBOL(i2c_add_adapter);
> +
>  static void i2c_scan_static_board_info(struct i2c_adapter *adap)
>  {
>  	struct boardinfo	*bi;
> @@ -392,6 +409,59 @@ static void i2c_scan_static_board_info(s
>  	mutex_unlock(&__i2c_board_lock);
>  }
>  
> +/**
> + * i2c_register_adapter - declare i2c adapter, use static bus number
> + * @adap: the adapter to register (with adap->nr initialized)
> + *
> + * This routine is used to declare an I2C adapter when its bus number
> + * matters.  Example: for I2C adapters from system-on-chip CPUs, or
> + * otherwise built in to the system's mainboard, and where i2c_board_info
> + * is used to properly configure I2C devices.
> + *
> + * If no devices have pre-been declared for this bus, then be sure to
> + * register the adapter before any dynamically allocated ones.  Otherwise
> + * the required bus ID may not be available.
> + *
> + * When this returns zero, the specified adapter became available for
> + * clients using the bus number provided in adap->nr.  Also, the table
> + * of I2C devices pre-declared using i2c_register_board_info() is scanned,
> + * and the appropriate driver model device nodes are created.  Otherwise, a
> + * negative errno value is returned.
> + */
> +int i2c_register_adapter(struct i2c_adapter *adap)
> +{
> +	int	id;
> +	int	status;
> +
> +	if (adap->nr & ~MAX_ID_MASK)
> +		return -EINVAL;
> +
> +retry:
> +	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0)
> +		return -ENOMEM;
> +
> +	mutex_lock(&core_lists);
> +	/* "above" here means "above or equal to", sigh;
> +	 * we need the "equal to" result to force the result
> +	 */
> +	status = idr_get_new_above(&i2c_adapter_idr, adap, adap->nr, &id);
> +	if (status == 0 && id != adap->nr) {
> +		status = -EBUSY;
> +		idr_remove(&i2c_adapter_idr, id);
> +	}
> +	mutex_unlock(&core_lists);
> +	if (status == -EAGAIN)
> +		goto retry;
> +
> +	if (status == 0)
> +		status = __i2c_register_adapter(adap);
> +	if (status == 0)
> +		i2c_scan_static_board_info(adap);
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(i2c_register_adapter);
> +
> +
>  static int i2c_do_del_adapter(struct device_driver *d, void *data)
>  {
>  	struct i2c_driver *driver = to_i2c_driver(d);
> @@ -459,14 +529,13 @@ int i2c_del_adapter(struct i2c_adapter *
>  
>  	/* clean up the sysfs representation */
>  	init_completion(&adap->dev_released);
> -	device_remove_file(&adap->dev, &dev_attr_name);
>  	device_unregister(&adap->dev);
>  	list_del(&adap->list);
>  
>  	/* wait for sysfs to drop all references */
>  	wait_for_completion(&adap->dev_released);
>  
> -	/* free dynamically allocated bus id */
> +	/* free bus id */
>  	idr_remove(&i2c_adapter_idr, adap->nr);
>  
>  	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
> @@ -506,6 +575,9 @@ int i2c_register_driver(struct module *o
>  	driver->driver.owner = owner;
>  	driver->driver.bus = &i2c_bus_type;
>  
> +	/* for new style drivers, when registration returns the driver core
> +	 * will have called probe() for all matching-but-unbound devices.
> +	 */
>  	res = driver_register(&driver->driver);
>  	pr_debug("i2c-core: register driver [%s] --> %d\n",
>  			driver->driver.name, res);
> @@ -626,7 +698,8 @@ int i2c_attach_client(struct i2c_client 
>  	client->usage_count = 0;
>  
>  	client->dev.parent = &client->adapter->dev;
> -	client->dev.driver = &client->driver->driver;
> +	if (client->driver)
> +		client->dev.driver = &client->driver->driver;
>  	client->dev.bus = &i2c_bus_type;
>  	client->dev.release = &i2c_client_release;
>  
> @@ -1362,7 +1435,6 @@ EXPORT_SYMBOL_GPL(i2c_adapter_dev_releas
>  EXPORT_SYMBOL_GPL(i2c_adapter_class);
>  EXPORT_SYMBOL_GPL(i2c_bus_type);
>  
> -EXPORT_SYMBOL(i2c_add_adapter);
>  EXPORT_SYMBOL(i2c_del_adapter);
>  EXPORT_SYMBOL(i2c_del_driver);
>  EXPORT_SYMBOL(i2c_attach_client);

-- 
Jean Delvare