[PATCH 0/6] i2c: Get rid of the legacy binding model

[PATCH 0/6] i2c: Get rid of the legacy binding model

[Jean Delvare]

As the last few remaining legacy i2c drivers are being converted, we
can finally get rid of the legacy binding model supporting code in
i2c-core. Here come 6 patches which do exactly that:

[1/6] i2c: Kill client_register and client_unregister methods
[2/6] i2c: Get rid of the legacy binding model
[3/6] i2c: Drop i2c_probe function
[4/6] i2c: Merge i2c_attach_client into i2c_new_device
[5/6] i2c: Kill is_newstyle_driver
[6/6] i2c: Kill the redundant client list

As one would expect, the size reduction is very significant:

 Documentation/feature-removal-schedule.txt |   10 -
 Documentation/i2c/writing-clients          |   16 -
 drivers/i2c/i2c-core.c                     |  366 +++--------------------------
 include/linux/i2c.h                        |   57 +----
 4 files changed, 48 insertions(+), 401 deletions(-)

>From a binary perspective, "size" announces an 18% shrink.

There are two things which should still be investigated.

1* In i2c_device_uevent() there is the following piece of code:

	/* by definition, legacy drivers can't hotplug */
	if (dev->driver)
		return 0;

David, I presume this can go away?

2* Core locking should be checked. I am not completely sure what
core_lock was supposed to protect, nor what it should protect now. I
think it should protect at least i2c_adapter_idr, driver->detect()
calls (which may add a device to the driver->clients list) and
driver->attach_adapter() calls (for the same reason), although for the
latter two, a per-driver lock may be more appropriate.

At the moment, we also hold the lock for all of i2c_register_adapter(),
i2c_del_adapter() and i2c_del_driver(), but not i2c_register_driver(),
which seems a little inconsistent. David, do you know if we need to
serialize the calls to device_register() and device_unregister(), or if
the driver core takes care of this for us?

Anything else needs to be protected?

-- 
Jean Delvare

[PATCH 1/6] i2c: Kill client_register and client_unregister methods

[Jean Delvare]

These methods were useful in the legacy binding model but no longer in
the new (standard) binding model. There are no users left so we can
drop them.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 Documentation/feature-removal-schedule.txt |    3 --
 drivers/i2c/i2c-core.c                     |   30 ----------------------------
 include/linux/i2c.h                        |    4 ---
 3 files changed, 2 insertions(+), 35 deletions(-)

--- linux-2.6.30-rc3.orig/Documentation/feature-removal-schedule.txt	2009-04-29 16:52:07.000000000 +0200
+++ linux-2.6.30-rc3/Documentation/feature-removal-schedule.txt	2009-04-29 17:10:36.000000000 +0200
@@ -354,8 +354,7 @@ Who:  Krzysztof Piotr Oledzki <ole@ans.p
 
 ---------------------------
 
-What:	i2c_attach_client(), i2c_detach_client(), i2c_driver->detach_client(),
-	i2c_adapter->client_register(), i2c_adapter->client_unregister
+What:	i2c_attach_client(), i2c_detach_client(), i2c_driver->detach_client()
 When:	2.6.30
 Check:	i2c_attach_client i2c_detach_client
 Why:	Deprecated by the new (standard) device driver binding model. Use
--- linux-2.6.30-rc3.orig/drivers/i2c/i2c-core.c	2009-04-29 16:52:07.000000000 +0200
+++ linux-2.6.30-rc3/drivers/i2c/i2c-core.c	2009-04-29 17:37:34.000000000 +0200
@@ -309,14 +309,6 @@ void i2c_unregister_device(struct i2c_cl
 		return;
 	}
 
-	if (adapter->client_unregister) {
-		if (adapter->client_unregister(client)) {
-			dev_warn(&client->dev,
-				 "client_unregister [%s] failed\n",
-				 client->name);
-		}
-	}
-
 	mutex_lock(&adapter->clist_lock);
 	list_del(&client->list);
 	mutex_unlock(&adapter->clist_lock);
@@ -867,14 +859,6 @@ int i2c_attach_client(struct i2c_client
 	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
 		client->name, dev_name(&client->dev));
 
-	if (adapter->client_register)  {
-		if (adapter->client_register(client)) {
-			dev_dbg(&adapter->dev, "client_register "
-				"failed for client [%s] at 0x%02x\n",
-				client->name, client->addr);
-		}
-	}
-
 	return 0;
 
 out_err:
@@ -887,17 +871,6 @@ EXPORT_SYMBOL(i2c_attach_client);
 int i2c_detach_client(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
-	int res = 0;
-
-	if (adapter->client_unregister)  {
-		res = adapter->client_unregister(client);
-		if (res) {
-			dev_err(&client->dev,
-				"client_unregister [%s] failed, "
-				"client not detached\n", client->name);
-			goto out;
-		}
-	}
 
 	mutex_lock(&adapter->clist_lock);
 	list_del(&client->list);
@@ -907,8 +880,7 @@ int i2c_detach_client(struct i2c_client
 	device_unregister(&client->dev);
 	wait_for_completion(&client->released);
 
- out:
-	return res;
+	return 0;
 }
 EXPORT_SYMBOL(i2c_detach_client);
 
--- linux-2.6.30-rc3.orig/include/linux/i2c.h	2009-04-29 16:52:07.000000000 +0200
+++ linux-2.6.30-rc3/include/linux/i2c.h	2009-04-29 17:12:14.000000000 +0200
@@ -352,10 +352,6 @@ struct i2c_adapter {
 	const struct i2c_algorithm *algo; /* the algorithm to access the bus */
 	void *algo_data;
 
-	/* --- administration stuff. */
-	int (*client_register)(struct i2c_client *) __deprecated;
-	int (*client_unregister)(struct i2c_client *) __deprecated;
-
 	/* data fields that are valid for all devices	*/
 	u8 level; 			/* nesting level for lockdep */
 	struct mutex bus_lock;

-- 
Jean Delvare

[PATCH 2/6] i2c: Get rid of the legacy binding model

[Jean Delvare]

We converted all the legacy i2c drivers so we can finally get rid of
the legacy binding model. Hooray!

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 Documentation/feature-removal-schedule.txt |    9 ---
 Documentation/i2c/writing-clients          |   16 +----
 drivers/i2c/i2c-core.c                     |   81 +---------------------------
 include/linux/i2c.h                        |   39 +++----------
 4 files changed, 18 insertions(+), 127 deletions(-)

--- linux-2.6.30-rc4.orig/Documentation/feature-removal-schedule.txt	2009-04-30 09:52:11.000000000 +0200
+++ linux-2.6.30-rc4/Documentation/feature-removal-schedule.txt	2009-04-30 09:52:14.000000000 +0200
@@ -354,15 +354,6 @@ Who:  Krzysztof Piotr Oledzki <ole@ans.p
 
 ---------------------------
 
-What:	i2c_attach_client(), i2c_detach_client(), i2c_driver->detach_client()
-When:	2.6.30
-Check:	i2c_attach_client i2c_detach_client
-Why:	Deprecated by the new (standard) device driver binding model. Use
-	i2c_driver->probe() and ->remove() instead.
-Who:	Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
-
----------------------------
-
 What:	fscher and fscpos drivers
 When:	June 2009
 Why:	Deprecated by the new fschmd driver.
--- linux-2.6.30-rc4.orig/drivers/i2c/i2c-core.c	2009-04-30 09:52:11.000000000 +0200
+++ linux-2.6.30-rc4/drivers/i2c/i2c-core.c	2009-04-30 18:05:25.000000000 +0200
@@ -44,6 +44,7 @@ static DEFINE_IDR(i2c_adapter_idr);
 
 #define is_newstyle_driver(d) ((d)->probe || (d)->remove || (d)->detect)
 
+static int i2c_attach_client(struct i2c_client *client);
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 
 /* ------------------------------------------------------------------------- */
@@ -467,7 +468,7 @@ static int i2c_register_adapter(struct i
 
 	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
 
-	/* create pre-declared device nodes for new-style drivers */
+	/* create pre-declared device nodes */
 	if (adap->nr < __i2c_first_dynamic_bus_num)
 		i2c_scan_static_board_info(adap);
 
@@ -610,7 +611,6 @@ static int i2c_do_del_adapter(struct dev
  */
 int i2c_del_adapter(struct i2c_adapter *adap)
 {
-	struct i2c_client *client, *_n;
 	int res = 0;
 
 	mutex_lock(&core_lock);
@@ -629,28 +629,6 @@ int i2c_del_adapter(struct i2c_adapter *
 	if (res)
 		goto out_unlock;
 
-	/* detach any active clients. This must be done first, because
-	 * it can fail; in which case we give up. */
-	list_for_each_entry_safe_reverse(client, _n, &adap->clients, list) {
-		struct i2c_driver	*driver;
-
-		driver = client->driver;
-
-		/* new style, follow standard driver model */
-		if (!driver || is_newstyle_driver(driver)) {
-			i2c_unregister_device(client);
-			continue;
-		}
-
-		/* 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);
-			goto out_unlock;
-		}
-	}
-
 	/* clean up the sysfs representation */
 	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);
@@ -692,11 +670,7 @@ static int __attach_adapter(struct devic
 
 /*
  * 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.
+ * i2c slave chips, on a bus instance associated with some i2c_adapter.
  */
 
 int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
@@ -707,21 +681,11 @@ int i2c_register_driver(struct module *o
 	if (unlikely(WARN_ON(!i2c_bus_type.p)))
 		return -EAGAIN;
 
-	/* new style driver methods can't mix with legacy ones */
-	if (is_newstyle_driver(driver)) {
-		if (driver->detach_adapter || driver->detach_client) {
-			printk(KERN_WARNING
-					"i2c-core: driver [%s] is confused\n",
-					driver->driver.name);
-			return -EINVAL;
-		}
-	}
-
 	/* add the driver to the list of i2c drivers in the driver core */
 	driver->driver.owner = owner;
 	driver->driver.bus = &i2c_bus_type;
 
-	/* for new style drivers, when registration returns the driver core
+	/* When registration returns, the driver core
 	 * will have called probe() for all matching-but-unbound devices.
 	 */
 	res = driver_register(&driver->driver);
@@ -760,29 +724,11 @@ static int __detach_adapter(struct devic
 	if (is_newstyle_driver(driver))
 		return 0;
 
-	/* 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.
-	 */
 	if (driver->detach_adapter) {
 		if (driver->detach_adapter(adapter))
 			dev_err(&adapter->dev,
 				"detach_adapter failed for driver [%s]\n",
 				driver->driver.name);
-	} else {
-		struct i2c_client *client, *_n;
-
-		list_for_each_entry_safe(client, _n, &adapter->clients, list) {
-			if (client->driver != driver)
-				continue;
-			dev_dbg(&adapter->dev,
-				"detaching client [%s] at 0x%02x\n",
-				client->name, client->addr);
-			if (driver->detach_client(client))
-				dev_err(&adapter->dev, "detach_client "
-					"failed for client [%s] at 0x%02x\n",
-					client->name, client->addr);
-		}
 	}
 
 	return 0;
@@ -824,7 +770,7 @@ static int i2c_check_addr(struct i2c_ada
 	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
 }
 
-int i2c_attach_client(struct i2c_client *client)
+static int i2c_attach_client(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
 	int res;
@@ -866,23 +812,6 @@ out_err:
 		"(%d)\n", client->name, client->addr, res);
 	return res;
 }
-EXPORT_SYMBOL(i2c_attach_client);
-
-int i2c_detach_client(struct i2c_client *client)
-{
-	struct i2c_adapter *adapter = client->adapter;
-
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
-	init_completion(&client->released);
-	device_unregister(&client->dev);
-	wait_for_completion(&client->released);
-
-	return 0;
-}
-EXPORT_SYMBOL(i2c_detach_client);
 
 /**
  * i2c_use_client - increments the reference count of the i2c client structure
--- linux-2.6.30-rc4.orig/include/linux/i2c.h	2009-04-30 09:52:11.000000000 +0200
+++ linux-2.6.30-rc4/include/linux/i2c.h	2009-04-30 18:04:20.000000000 +0200
@@ -100,9 +100,8 @@ extern s32 i2c_smbus_write_i2c_block_dat
  * @class: What kind of i2c device we instantiate (for detect)
  * @attach_adapter: Callback for bus addition (for legacy drivers)
  * @detach_adapter: Callback for bus removal (for legacy drivers)
- * @detach_client: Callback for device removal (for legacy drivers)
- * @probe: Callback for device binding (new-style drivers)
- * @remove: Callback for device unbinding (new-style drivers)
+ * @probe: Callback for device binding
+ * @remove: Callback for device unbinding
  * @shutdown: Callback for device shutdown
  * @suspend: Callback for device suspend
  * @resume: Callback for device resume
@@ -137,26 +136,14 @@ struct i2c_driver {
 	int id;
 	unsigned int class;
 
-	/* Notifies the driver that a new bus has appeared. This routine
-	 * 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.  (LEGACY I2C DRIVERS ONLY)
+	/* Notifies the driver that a new bus has appeared or is about to be
+	 * removed. You should avoid using this if you can, it will probably
+	 * be removed in a near future.
 	 */
 	int (*attach_adapter)(struct i2c_adapter *);
 	int (*detach_adapter)(struct i2c_adapter *);
 
-	/* 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.  (LEGACY I2C DRIVERS ONLY)
-	 */
-	int (*detach_client)(struct i2c_client *) __deprecated;
-
-	/* 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)
-	 */
+	/* Standard driver model interfaces */
 	int (*probe)(struct i2c_client *, const struct i2c_device_id *);
 	int (*remove)(struct i2c_client *);
 
@@ -248,11 +235,10 @@ static inline void i2c_set_clientdata(st
  * that, such as chip type, configuration, associated IRQ, and so on.
  *
  * i2c_board_info is used to build tables of information listing I2C devices
- * that are present.  This information is used to grow the driver model tree
- * for "new style" I2C drivers.  For mainboards this is done statically using
- * i2c_register_board_info(); bus numbers identify adapters that aren't
- * yet available.  For add-on boards, i2c_new_device() does this dynamically
- * with the adapter already known.
+ * that are present.  This information is used to grow the driver model tree.
+ * For mainboards this is done statically using i2c_register_board_info();
+ * bus numbers identify adapters that aren't yet available.  For add-on boards,
+ * i2c_new_device() does this dynamically with the adapter already known.
  */
 struct i2c_board_info {
 	char		type[I2C_NAME_SIZE];
@@ -425,11 +411,6 @@ static inline int i2c_add_driver(struct
 	return i2c_register_driver(THIS_MODULE, driver);
 }
 
-/* These are deprecated, your driver should use the standard .probe()
- * and .remove() methods instead. */
-extern int __deprecated i2c_attach_client(struct i2c_client *);
-extern int __deprecated i2c_detach_client(struct i2c_client *);
-
 extern struct i2c_client *i2c_use_client(struct i2c_client *client);
 extern void i2c_release_client(struct i2c_client *client);
 
--- linux-2.6.30-rc4.orig/Documentation/i2c/writing-clients	2009-04-30 08:37:40.000000000 +0200
+++ linux-2.6.30-rc4/Documentation/i2c/writing-clients	2009-04-30 09:52:14.000000000 +0200
@@ -126,19 +126,9 @@ different) configuration information, as
 that can't be distinguished by protocol probing, or which need some board
 specific information to operate correctly.
 
-Accordingly, the I2C stack now has two models for associating I2C devices
-with their drivers:  the original "legacy" model, and a newer one that's
-fully compatible with the Linux 2.6 driver model.  These models do not mix,
-since the "legacy" model requires drivers to create "i2c_client" device
-objects after SMBus style probing, while the Linux driver model expects
-drivers to be given such device objects in their probe() routines.
 
-The legacy model is deprecated now and will soon be removed, so we no
-longer document it here.
-
-
-Standard Driver Model Binding ("New Style")
--------------------------------------------
+Device/Driver Binding
+---------------------
 
 System infrastructure, typically board-specific initialization code or
 boot firmware, reports what I2C devices exist.  For example, there may be
@@ -201,7 +191,7 @@ a given I2C bus.  This is for example th
 devices on a PC's SMBus.  In that case, you may want to let your driver
 detect supported devices automatically.  This is how the legacy model
 was working, and is now available as an extension to the standard
-driver model (so that we can finally get rid of the legacy model.)
+driver model.
 
 You simply have to define a detect callback which will attempt to
 identify supported devices (returning 0 for supported ones and -ENODEV

-- 
Jean Delvare

Re: [PATCH 2/6] i2c: Get rid of the legacy binding model

[David Brownell]

On Saturday 02 May 2009, Jean Delvare wrote:
> @@ -100,9 +100,8 @@ extern s32 i2c_smbus_write_i2c_block_dat
>   * @class: What kind of i2c device we instantiate (for detect)
>   * @attach_adapter: Callback for bus addition (for legacy drivers)
>   * @detach_adapter: Callback for bus removal (for legacy drivers)

Remnants of the legacy model still remain ...


> - * @detach_client: Callback for device removal (for legacy drivers)
> - * @probe: Callback for device binding (new-style drivers)
> - * @remove: Callback for device unbinding (new-style drivers)
> + * @probe: Callback for device binding
> + * @remove: Callback for device unbinding
>   * @shutdown: Callback for device shutdown
>   * @suspend: Callback for device suspend
>   * @resume: Callback for device resume

Re: [PATCH 2/6] i2c: Get rid of the legacy binding model

[Jean Delvare]

On Sat, 2 May 2009 12:03:43 -0700, David Brownell wrote:
> On Saturday 02 May 2009, Jean Delvare wrote:
> > @@ -100,9 +100,8 @@ extern s32 i2c_smbus_write_i2c_block_dat
> >   * @class: What kind of i2c device we instantiate (for detect)
> >   * @attach_adapter: Callback for bus addition (for legacy drivers)
> >   * @detach_adapter: Callback for bus removal (for legacy drivers)
> 
> Remnants of the legacy model still remain ...

Depends on what exactly you call "the legacy model". As long as the
lifetime of the devices is those of the standard model, I don't much
care much about the rest.

At this point there are still a few drivers using attach_adapter, so we
can't get rid of it: 9 macintosh drivers, and i2c-dev. The fix for the
macintosh drivers would be to convert the powermac to fixed bus
numbers. This is probably not very difficult, but I'd rather let
powerpc people take care of that.

For i2c-dev, I just don't know. We could merge it into i2c-core. Or we
could split the notification mechanism out of i2c_driver. Or maybe we
can leverage the driver core notification mechanism and get rid of
ours.

Either way this is probably not the highest priority change at this
point in time. I'd rather work on the sysfs replacement for
I2C_CLIENT_INSMOD* macros, the multiplexing support and the conversion
of i2c-adapter to bus devices first.

> > - * @detach_client: Callback for device removal (for legacy drivers)
> > - * @probe: Callback for device binding (new-style drivers)
> > - * @remove: Callback for device unbinding (new-style drivers)
> > + * @probe: Callback for device binding
> > + * @remove: Callback for device unbinding
> >   * @shutdown: Callback for device shutdown
> >   * @suspend: Callback for device suspend
> >   * @resume: Callback for device resume

-- 
Jean Delvare

[PATCH 3/6] i2c: Drop i2c_probe function

[Jean Delvare]

The legacy i2c_probe() function has no users left, get rid of it.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/i2c/i2c-core.c |  137 ------------------------------------------------
 include/linux/i2c.h    |    8 --
 2 files changed, 145 deletions(-)

--- linux-2.6.30-rc4.orig/drivers/i2c/i2c-core.c	2009-04-30 18:05:25.000000000 +0200
+++ linux-2.6.30-rc4/drivers/i2c/i2c-core.c	2009-04-30 18:05:54.000000000 +0200
@@ -1042,144 +1042,7 @@ EXPORT_SYMBOL(i2c_master_recv);
  * Will not work for 10-bit addresses!
  * ----------------------------------------------------
  */
-static int i2c_probe_address(struct i2c_adapter *adapter, int addr, int kind,
-			     int (*found_proc) (struct i2c_adapter *, int, int))
-{
-	int err;
-
-	/* Make sure the address is valid */
-	if (addr < 0x03 || addr > 0x77) {
-		dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
-			 addr);
-		return -EINVAL;
-	}
-
-	/* Skip if already in use */
-	if (i2c_check_addr(adapter, addr))
-		return 0;
-
-	/* Make sure there is something at this address, unless forced */
-	if (kind < 0) {
-		if (i2c_smbus_xfer(adapter, addr, 0, 0, 0,
-				   I2C_SMBUS_QUICK, NULL) < 0)
-			return 0;
-
-		/* prevent 24RF08 corruption */
-		if ((addr & ~0x0f) == 0x50)
-			i2c_smbus_xfer(adapter, addr, 0, 0, 0,
-				       I2C_SMBUS_QUICK, NULL);
-	}
-
-	/* Finally call the custom detection function */
-	err = found_proc(adapter, addr, kind);
-	/* -ENODEV can be returned if there is a chip at the given address
-	   but it isn't supported by this chip driver. We catch it here as
-	   this isn't an error. */
-	if (err == -ENODEV)
-		err = 0;
-
-	if (err)
-		dev_warn(&adapter->dev, "Client creation failed at 0x%x (%d)\n",
-			 addr, err);
-	return err;
-}
-
-int i2c_probe(struct i2c_adapter *adapter,
-	      const struct i2c_client_address_data *address_data,
-	      int (*found_proc) (struct i2c_adapter *, int, int))
-{
-	int i, err;
-	int adap_id = i2c_adapter_id(adapter);
-
-	/* Force entries are done first, and are not affected by ignore
-	   entries */
-	if (address_data->forces) {
-		const unsigned short * const *forces = address_data->forces;
-		int kind;
-
-		for (kind = 0; forces[kind]; kind++) {
-			for (i = 0; forces[kind][i] != I2C_CLIENT_END;
-			     i += 2) {
-				if (forces[kind][i] == adap_id
-				 || forces[kind][i] == ANY_I2C_BUS) {
-					dev_dbg(&adapter->dev, "found force "
-						"parameter for adapter %d, "
-						"addr 0x%02x, kind %d\n",
-						adap_id, forces[kind][i + 1],
-						kind);
-					err = i2c_probe_address(adapter,
-						forces[kind][i + 1],
-						kind, found_proc);
-					if (err)
-						return err;
-				}
-			}
-		}
-	}
-
-	/* Stop here if we can't use SMBUS_QUICK */
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_QUICK)) {
-		if (address_data->probe[0] == I2C_CLIENT_END
-		 && address_data->normal_i2c[0] == I2C_CLIENT_END)
-			return 0;
-
-		dev_dbg(&adapter->dev, "SMBus Quick command not supported, "
-			"can't probe for chips\n");
-		return -EOPNOTSUPP;
-	}
-
-	/* Probe entries are done second, and are not affected by ignore
-	   entries either */
-	for (i = 0; address_data->probe[i] != I2C_CLIENT_END; i += 2) {
-		if (address_data->probe[i] == adap_id
-		 || address_data->probe[i] == ANY_I2C_BUS) {
-			dev_dbg(&adapter->dev, "found probe parameter for "
-				"adapter %d, addr 0x%02x\n", adap_id,
-				address_data->probe[i + 1]);
-			err = i2c_probe_address(adapter,
-						address_data->probe[i + 1],
-						-1, found_proc);
-			if (err)
-				return err;
-		}
-	}
-
-	/* Normal entries are done last, unless shadowed by an ignore entry */
-	for (i = 0; address_data->normal_i2c[i] != I2C_CLIENT_END; i += 1) {
-		int j, ignore;
-
-		ignore = 0;
-		for (j = 0; address_data->ignore[j] != I2C_CLIENT_END;
-		     j += 2) {
-			if ((address_data->ignore[j] == adap_id ||
-			     address_data->ignore[j] == ANY_I2C_BUS)
-			 && address_data->ignore[j + 1]
-			    == address_data->normal_i2c[i]) {
-				dev_dbg(&adapter->dev, "found ignore "
-					"parameter for adapter %d, "
-					"addr 0x%02x\n", adap_id,
-					address_data->ignore[j + 1]);
-				ignore = 1;
-				break;
-			}
-		}
-		if (ignore)
-			continue;
-
-		dev_dbg(&adapter->dev, "found normal entry for adapter %d, "
-			"addr 0x%02x\n", adap_id,
-			address_data->normal_i2c[i]);
-		err = i2c_probe_address(adapter, address_data->normal_i2c[i],
-					-1, found_proc);
-		if (err)
-			return err;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL(i2c_probe);
 
-/* Separate detection function for new-style drivers */
 static int i2c_detect_address(struct i2c_client *temp_client, int kind,
 			      struct i2c_driver *driver)
 {
--- linux-2.6.30-rc4.orig/include/linux/i2c.h	2009-04-30 18:04:20.000000000 +0200
+++ linux-2.6.30-rc4/include/linux/i2c.h	2009-04-30 18:05:36.000000000 +0200
@@ -419,14 +419,6 @@ extern void i2c_release_client(struct i2
 extern void i2c_clients_command(struct i2c_adapter *adap,
 				unsigned int cmd, void *arg);
 
-/* Detect function. It iterates over all possible addresses itself.
- * It will only call found_proc if some client is connected at the
- * specific address (unless a 'force' matched);
- */
-extern int i2c_probe(struct i2c_adapter *adapter,
-		const struct i2c_client_address_data *address_data,
-		int (*found_proc) (struct i2c_adapter *, int, int));
-
 extern struct i2c_adapter *i2c_get_adapter(int id);
 extern void i2c_put_adapter(struct i2c_adapter *adap);
 

-- 
Jean Delvare

[PATCH 4/6] i2c: Merge i2c_attach_client into i2c_new_device

[Jean Delvare]

Now that i2c_attach_client is no longer exported, it doesn't need to
be a separate function. Merge it into its only user, i2c_new_device.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/i2c/i2c-core.c |  100 +++++++++++++++++++-----------------------------
 1 file changed, 41 insertions(+), 59 deletions(-)

--- linux-2.6.30-rc4.orig/drivers/i2c/i2c-core.c	2009-04-30 18:02:02.000000000 +0200
+++ linux-2.6.30-rc4/drivers/i2c/i2c-core.c	2009-04-30 18:02:04.000000000 +0200
@@ -44,7 +44,7 @@ static DEFINE_IDR(i2c_adapter_idr);
 
 #define is_newstyle_driver(d) ((d)->probe || (d)->remove || (d)->detect)
 
-static int i2c_attach_client(struct i2c_client *client);
+static int i2c_check_addr(struct i2c_adapter *adapter, int addr);
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 
 /* ------------------------------------------------------------------------- */
@@ -242,15 +242,17 @@ EXPORT_SYMBOL(i2c_verify_client);
 
 
 /**
- * i2c_new_device - instantiate an i2c device for use with a new style driver
+ * i2c_new_device - instantiate an i2c device
  * @adap: the adapter managing the device
  * @info: describes one I2C device; bus_num is ignored
  * Context: can sleep
  *
- * Create a device to work with a new style i2c driver, where binding is
- * handled through driver model probe()/remove() methods.  This call is not
- * appropriate for use by mainboad initialization logic, which usually runs
- * during an arch_initcall() long before any i2c_adapter could exist.
+ * Create an i2c device. Binding is handled through driver model
+ * probe()/remove() methods.  A driver may be bound to this device when we
+ * return from this function, or any later moment (e.g. maybe hotplugging will
+ * load the driver module).  This call is not appropriate for use by mainboard
+ * initialization logic, which usually runs during an arch_initcall() long
+ * before any i2c_adapter could exist.
  *
  * This returns the new i2c client, which may be saved for later use with
  * i2c_unregister_device(); or NULL to indicate an error.
@@ -278,17 +280,40 @@ i2c_new_device(struct i2c_adapter *adap,
 
 	strlcpy(client->name, info->type, sizeof(client->name));
 
-	/* a new style driver may be bound to this device when we
-	 * return from this function, or any later moment (e.g. maybe
-	 * hotplugging will load the driver module).  and the device
-	 * refcount model is the standard driver model one.
-	 */
-	status = i2c_attach_client(client);
-	if (status < 0) {
-		kfree(client);
-		client = NULL;
-	}
+	/* Check for address business */
+	status = i2c_check_addr(adap, client->addr);
+	if (status)
+		goto out_err;
+
+	client->dev.parent = &client->adapter->dev;
+	client->dev.bus = &i2c_bus_type;
+
+	if (client->driver && !is_newstyle_driver(client->driver)) {
+		client->dev.release = i2c_client_release;
+		dev_set_uevent_suppress(&client->dev, 1);
+	} else
+		client->dev.release = i2c_client_dev_release;
+
+	dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap),
+		     client->addr);
+	status = device_register(&client->dev);
+	if (status)
+		goto out_err;
+
+	mutex_lock(&adap->clist_lock);
+	list_add_tail(&client->list, &adap->clients);
+	mutex_unlock(&adap->clist_lock);
+
+	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
+		client->name, dev_name(&client->dev));
+
 	return client;
+
+out_err:
+	dev_err(&adap->dev, "Failed to register i2c client %s at 0x%02x "
+		"(%d)\n", client->name, client->addr, status);
+	kfree(client);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(i2c_new_device);
 
@@ -770,49 +795,6 @@ static int i2c_check_addr(struct i2c_ada
 	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
 }
 
-static int i2c_attach_client(struct i2c_client *client)
-{
-	struct i2c_adapter *adapter = client->adapter;
-	int res;
-
-	/* Check for address business */
-	res = i2c_check_addr(adapter, client->addr);
-	if (res)
-		return res;
-
-	client->dev.parent = &client->adapter->dev;
-	client->dev.bus = &i2c_bus_type;
-
-	if (client->driver)
-		client->dev.driver = &client->driver->driver;
-
-	if (client->driver && !is_newstyle_driver(client->driver)) {
-		client->dev.release = i2c_client_release;
-		dev_set_uevent_suppress(&client->dev, 1);
-	} else
-		client->dev.release = i2c_client_dev_release;
-
-	dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adapter),
-		     client->addr);
-	res = device_register(&client->dev);
-	if (res)
-		goto out_err;
-
-	mutex_lock(&adapter->clist_lock);
-	list_add_tail(&client->list, &adapter->clients);
-	mutex_unlock(&adapter->clist_lock);
-
-	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
-		client->name, dev_name(&client->dev));
-
-	return 0;
-
-out_err:
-	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
-		"(%d)\n", client->name, client->addr, res);
-	return res;
-}
-
 /**
  * i2c_use_client - increments the reference count of the i2c client structure
  * @client: the client being referenced

-- 
Jean Delvare

[PATCH 5/6] i2c: Kill is_newstyle_driver

[Jean Delvare]

Legacy i2c drivers are gone, all drivers are new-style now, so there
is no point to check.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   32 +-------------------------------
 include/linux/i2c.h    |    2 --
 2 files changed, 1 insertion(+), 33 deletions(-)

--- linux-2.6.30-rc4.orig/drivers/i2c/i2c-core.c	2009-04-30 18:02:04.000000000 +0200
+++ linux-2.6.30-rc4/drivers/i2c/i2c-core.c	2009-04-30 18:02:07.000000000 +0200
@@ -42,8 +42,6 @@
 static DEFINE_MUTEX(core_lock);
 static DEFINE_IDR(i2c_adapter_idr);
 
-#define is_newstyle_driver(d) ((d)->probe || (d)->remove || (d)->detect)
-
 static int i2c_check_addr(struct i2c_adapter *adapter, int addr);
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 
@@ -65,12 +63,6 @@ static int i2c_device_match(struct devic
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(drv);
 
-	/* make legacy i2c drivers bypass driver model probing entirely;
-	 * such drivers scan each i2c adapter/bus themselves.
-	 */
-	if (!is_newstyle_driver(driver))
-		return 0;
-
 	/* match on an id table if there is one */
 	if (driver->id_table)
 		return i2c_match_id(driver->id_table, client) != NULL;
@@ -177,12 +169,6 @@ 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 void i2c_client_dev_release(struct device *dev)
 {
 	kfree(to_i2c_client(dev));
@@ -287,12 +273,7 @@ i2c_new_device(struct i2c_adapter *adap,
 
 	client->dev.parent = &client->adapter->dev;
 	client->dev.bus = &i2c_bus_type;
-
-	if (client->driver && !is_newstyle_driver(client->driver)) {
-		client->dev.release = i2c_client_release;
-		dev_set_uevent_suppress(&client->dev, 1);
-	} else
-		client->dev.release = i2c_client_dev_release;
+	client->dev.release = i2c_client_dev_release;
 
 	dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap),
 		     client->addr);
@@ -326,14 +307,6 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
 void i2c_unregister_device(struct i2c_client *client)
 {
 	struct i2c_adapter	*adapter = client->adapter;
-	struct i2c_driver	*driver = client->driver;
-
-	if (driver && !is_newstyle_driver(driver)) {
-		dev_err(&client->dev, "can't unregister devices "
-			"with legacy drivers\n");
-		WARN_ON(1);
-		return;
-	}
 
 	mutex_lock(&adapter->clist_lock);
 	list_del(&client->list);
@@ -746,9 +719,6 @@ static int __detach_adapter(struct devic
 		i2c_unregister_device(client);
 	}
 
-	if (is_newstyle_driver(driver))
-		return 0;
-
 	if (driver->detach_adapter) {
 		if (driver->detach_adapter(adapter))
 			dev_err(&adapter->dev,
--- linux-2.6.30-rc4.orig/include/linux/i2c.h	2009-04-30 18:02:02.000000000 +0200
+++ linux-2.6.30-rc4/include/linux/i2c.h	2009-04-30 18:02:12.000000000 +0200
@@ -180,7 +180,6 @@ struct i2c_driver {
  * @irq: indicates the IRQ generated by this device (if any)
  * @list: list of active/busy clients (DEPRECATED)
  * @detected: member of an i2c_driver.clients list
- * @released: used to synchronize client releases & detaches and references
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
  * i2c bus. The behaviour exposed to Linux is defined by the driver
@@ -198,7 +197,6 @@ struct i2c_client {
 	int irq;			/* irq issued by device		*/
 	struct list_head list;		/* DEPRECATED */
 	struct list_head detected;
-	struct completion released;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
 

-- 
Jean Delvare

[PATCH 6/6] i2c: Kill the redundant client list

[Jean Delvare]

We used to maintain our own per-adapter list of i2c clients, but this
is redundant with what the driver core does, and no longer needed.
Just drop the redundant list.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   12 ------------
 include/linux/i2c.h    |    4 ----
 2 files changed, 16 deletions(-)

--- linux-2.6.30-rc4.orig/drivers/i2c/i2c-core.c	2009-04-30 14:33:14.000000000 +0200
+++ linux-2.6.30-rc4/drivers/i2c/i2c-core.c	2009-04-30 15:35:30.000000000 +0200
@@ -281,10 +281,6 @@ i2c_new_device(struct i2c_adapter *adap,
 	if (status)
 		goto out_err;
 
-	mutex_lock(&adap->clist_lock);
-	list_add_tail(&client->list, &adap->clients);
-	mutex_unlock(&adap->clist_lock);
-
 	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
 		client->name, dev_name(&client->dev));
 
@@ -306,12 +302,6 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
  */
 void i2c_unregister_device(struct i2c_client *client)
 {
-	struct i2c_adapter	*adapter = client->adapter;
-
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
 	device_unregister(&client->dev);
 }
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
@@ -437,8 +427,6 @@ static int i2c_register_adapter(struct i
 		return -EAGAIN;
 
 	mutex_init(&adap->bus_lock);
-	mutex_init(&adap->clist_lock);
-	INIT_LIST_HEAD(&adap->clients);
 
 	mutex_lock(&core_lock);
 
--- linux-2.6.30-rc4.orig/include/linux/i2c.h	2009-04-30 14:34:30.000000000 +0200
+++ linux-2.6.30-rc4/include/linux/i2c.h	2009-04-30 15:34:44.000000000 +0200
@@ -178,7 +178,6 @@ struct i2c_driver {
  * @driver: device's driver, hence pointer to access routines
  * @dev: Driver model device node for the slave.
  * @irq: indicates the IRQ generated by this device (if any)
- * @list: list of active/busy clients (DEPRECATED)
  * @detected: member of an i2c_driver.clients list
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
@@ -195,7 +194,6 @@ struct i2c_client {
 	struct i2c_driver *driver;	/* and our access routines	*/
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device		*/
-	struct list_head list;		/* DEPRECATED */
 	struct list_head detected;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -339,14 +337,12 @@ struct i2c_adapter {
 	/* data fields that are valid for all devices	*/
 	u8 level; 			/* nesting level for lockdep */
 	struct mutex bus_lock;
-	struct mutex clist_lock;
 
 	int timeout;			/* in jiffies */
 	int retries;
 	struct device dev;		/* the adapter device */
 
 	int nr;
-	struct list_head clients;	/* DEPRECATED */
 	char name[48];
 	struct completion dev_released;
 };

-- 
Jean Delvare

Re: [PATCH 0/6] i2c: Get rid of the legacy binding model

[David Brownell]

On Saturday 02 May 2009, Jean Delvare wrote:
> 1* In i2c_device_uevent() there is the following piece of code:
> 
>         /* by definition, legacy drivers can't hotplug */
>         if (dev->driver)
>                 return 0;
> 
> David, I presume this can go away?

Legacy drivers are going away, so ... yes.  :)


> 2* Core locking should be checked. I am not completely sure what
> core_lock was supposed to protect, nor what it should protect now. I
> think it should protect at least i2c_adapter_idr, driver->detect()
> calls (which may add a device to the driver->clients list) and
> driver->attach_adapter() calls (for the same reason), although for the
> latter two, a per-driver lock may be more appropriate.

Protecting the IDR seems right to me.  I'd have to look at the
post-patches code to say more ... but for now I'll just say that
locking driver->*() calls seems less desirable than just making
sure the updates to clients_list are protected.  You should work
to minimize lock scope, so in general *drop* locks before calling
out from one module to another.  (Lots of nastiness comes from
the other module making calls that involve more locking...)


> At the moment, we also hold the lock for all of i2c_register_adapter(),
> i2c_del_adapter() and i2c_del_driver(), but not i2c_register_driver(),
> which seems a little inconsistent. David, do you know if we need to
> serialize the calls to device_register() and device_unregister(), or if
> the driver core takes care of this for us?

Driver core locking should eventually suffice for everything except
data structures directly managed by the I2C framework.

- Dave

Re: [PATCH 0/6] i2c: Get rid of the legacy binding model

[David Brownell]

On Saturday 02 May 2009, Jean Delvare wrote:
>  Documentation/feature-removal-schedule.txt |   10 -
>  Documentation/i2c/writing-clients          |   16 -
>  drivers/i2c/i2c-core.c                     |  366 +++--------------------------
>  include/linux/i2c.h                        |   57 +----

Which makes all but patch #4 *very* easy to review.
All they do is remove code.  Unless that breaks a
build, those removals have a hard time making any
trouble.  ;)


>  4 files changed, 48 insertions(+), 401 deletions(-)
> 
> From a binary perspective, "size" announces an 18% shrink.

And Linux now builds without all those warnings, yes?

Plus from a code complexity perspective, it's even
better ... it removes 100% of the high level confusion
associated with that legacy model, and the bugs and
fragility caused by that.  (Or maybe I should say
that it *finishes* removing that.)

Glad to see this.  I presume this will sit in -next
for a while and merge in 2.6.31-early?

- Dave

Re: [PATCH 0/6] i2c: Get rid of the legacy binding model

[Jean Delvare]

On Sat, 2 May 2009 12:14:02 -0700, David Brownell wrote:
> On Saturday 02 May 2009, Jean Delvare wrote:
> >  Documentation/feature-removal-schedule.txt |   10 -
> >  Documentation/i2c/writing-clients          |   16 -
> >  drivers/i2c/i2c-core.c                     |  366 +++--------------------------
> >  include/linux/i2c.h                        |   57 +----
> 
> Which makes all but patch #4 *very* easy to review.
> All they do is remove code.  Unless that breaks a
> build, those removals have a hard time making any
> trouble.  ;)

Well, removing code can always break things. For example, say I remove
all the locking code in the kernel... ;)

> >  4 files changed, 48 insertions(+), 401 deletions(-)
> > 
> > From a binary perspective, "size" announces an 18% shrink.
> 
> And Linux now builds without all those warnings, yes?

Yes it does :)

> Plus from a code complexity perspective, it's even
> better ... it removes 100% of the high level confusion
> associated with that legacy model, and the bugs and
> fragility caused by that.  (Or maybe I should say
> that it *finishes* removing that.)

Thanks for doing all the hard work :)

> Glad to see this.  I presume this will sit in -next
> for a while and merge in 2.6.31-early?

Exactly. Where "early" is unfortunately bound to the few remaining
legacy drivers, the conversion of which is supposed to be merged by
their respective maintainers. But anyway, the hard deadline is
2.6.31-rc1.

-- 
Jean Delvare