[i2c] [patch 2/5] i2c can remove()

[Jean Delvare]

Hi David,

On Thu, 15 Feb 2007 00:41:59 -0800, David Brownell wrote:
> More update for new style driver support:  add a remove() method, and
> use it in the relevant code paths.  Again, nothing will use this yet
> since there's nothing to create devices feeding this infrastructure.
> 
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> 
> ---
>  drivers/i2c/i2c-core.c |   77 +++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/i2c.h    |    1 
>  2 files changed, 72 insertions(+), 6 deletions(-)
> 
> Index: i2c/include/linux/i2c.h
> ===================================================================
> --- i2c.orig/include/linux/i2c.h	2007-02-15 00:00:03.000000000 -0800
> +++ i2c/include/linux/i2c.h	2007-02-15 00:00:04.000000000 -0800
> @@ -130,6 +130,7 @@ struct i2c_driver {
>  	 * it's done by infrastructure.  (NEW STYLE DRIVERS ONLY)
>  	 */
>  	int (*probe)(struct i2c_client *);
> +	int (*remove)(struct i2c_client *);
>  
>  	/* driver model interfaces that don't relate to enumeration  */
>  	void (*shutdown)(struct i2c_client *);
> Index: i2c/drivers/i2c/i2c-core.c
> ===================================================================
> --- i2c.orig/drivers/i2c/i2c-core.c	2007-02-15 00:00:03.000000000 -0800
> +++ i2c/drivers/i2c/i2c-core.c	2007-02-15 00:00:04.000000000 -0800
> @@ -99,7 +99,19 @@ static int i2c_device_probe(struct devic
>  
>  static int i2c_device_remove(struct device *dev)
>  {
> -	return 0;
> +	struct i2c_client	*client = to_i2c_client(dev);
> +	struct i2c_driver	*driver;
> +	int			status;
> +
> +	if (!dev->driver)
> +		return 0;
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->remove)
> +		return 0;
> +	status = driver->remove(client);
> +	if (status == 0)
> +		client->driver = NULL;
> +	return status;
>  }
>  
>  static void i2c_device_shutdown(struct device *dev)
> @@ -168,6 +180,43 @@ struct bus_type i2c_bus_type = {
>  	.resume		= i2c_device_resume,
>  };
>  
> +static void i2c_unregister_device(struct i2c_client *client)
> +{
> +	struct i2c_adapter	*adap = client->adapter;
> +	struct i2c_driver	*driver = client->driver;
> +	int			status;
> +
> +	if (driver && !driver->remove) {
> +		dev_err(&client->dev, "can't unregister devices "
> +			"with legacy drivers\n");
> +		WARN_ON(1);
> +		return;
> +	}

I'm not sure I agree with this. Isn't it valid for a (device driver
model compliant) driver not to have a remove() method? Also, I don't
quite see how this method would ever be called for a legacy driver, as
you check explicitely against that at registration time.

> +
> +	/* unbind driver */
> +	if (driver) {
> +		/* REVISIT driver model core would handle this for us, and
> +		 * once i2c_detach_client() stops updating lists, it should.
> +		 */

Actually my current i2c stack no longer has the faulty i2c_adapter
driver, so it should be possible to remove the duplicate list
management. Could you provide a patch doing this? Would it let you
make this one patch cleaner?

> +		status = i2c_device_remove(&client->dev);
> +		if (status < 0) {
> +			dev_err(&client->dev, "unbind failed (%d)\n", status);
> +			return;
> +		}
> +	}
> +
> +	/* now remove the device, and free its memory */
> +	status = i2c_detach_client(client);
> +	if (status < 0)
> +		dev_err(&adap->dev, "detach failed (%d) for client [%s] "
> +			"at address 0x%02x\n",
> +			status, client->name, client->addr);
> +	else
> +		put_device(&client->dev);

Am I right assuming that we're in big trouble if i2c_detach_client()
actually fails?

> +

Extra blank line.

> +}
> +
> +
>  /* ------------------------------------------------------------------------- */
>  
>  void i2c_adapter_dev_release(struct device *dev)
> @@ -321,9 +370,19 @@ int i2c_del_adapter(struct i2c_adapter *
>  	/* detach any active clients. This must be done first, because
>  	 * it can fail; in which case we give up. */
>  	list_for_each_safe(item, _n, &adap->clients) {
> +		struct i2c_driver	*driver;
> +
>  		client = list_entry(item, struct i2c_client, list);
> +		driver = client->driver;
> +
> +		/* new style, follow standard driver model */
> +		if (!driver || driver->remove) {
> +			i2c_unregister_device(client);
> +			continue;
> +		}
>  
> -		if ((res=client->driver->detach_client(client))) {
> +		/* legacy drivers create and remove clients themselves */
> +		if ((res = driver->detach_client(client))) {
>  			dev_err(&adap->dev, "detach_client failed for client "
>  				"[%s] at address 0x%02x\n", client->name,
>  				client->addr);
> @@ -367,7 +426,7 @@ int i2c_register_driver(struct module *o
>  	int res;
>  
>  	/* new style driver methods can't mix with legacy ones */
> -	if (driver->probe) {
> +	if (driver->probe || driver->remove) {
>  		if (driver->attach_adapter || driver->detach_adapter
>  				|| driver->detach_client) {
>  			pr_debug("i2c-core: driver [%s] is confused\n",
> @@ -411,12 +470,17 @@ int i2c_del_driver(struct i2c_driver *dr
>  
>  	int res = 0;
>  
> -	mutex_lock(&core_lists);
> +	/* For new-style drivers, driver model unregistration handles
> +	 * all the unbind-from-device logic.  Just Do It.

I would avoid the extra caps. Don't want to be sued by some company for
using their trademark without permission ;)

> +	 */
> +	if (driver->remove)
> +		goto unregister;
>  
>  	/* Have a look at each adapter, if clients of this driver are still
>  	 * attached. If so, detach them to be able to kill the driver
>  	 * afterwards.
>  	 */
> +	mutex_lock(&core_lists);
>  	list_for_each(item1,&adapters) {
>  		adap = list_entry(item1, struct i2c_adapter, list);
>  		if (driver->detach_adapter) {
> @@ -444,12 +508,13 @@ int i2c_del_driver(struct i2c_driver *dr
>  			}
>  		}
>  	}
> +out_unlock:
> +	mutex_unlock(&core_lists);
>  
> +unregister:
>  	driver_unregister(&driver->driver);
>  	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
>  
> - out_unlock:
> -	mutex_unlock(&core_lists);
>  	return 0;
>  }

It's not correct for legacy drivers, I think. The original code did
_not_ call driver_unregister on error, and now it does. I don't think
your patch is supposed to change anything for legacy drivers, is it?

When the patches are ready, I think I'll fold this patch into the first
one (i2c stack can probe()). Each doesn't make much sense without the
other.

-- 
Jean Delvare