[PATCH 0/4] i2c: Add detection capability to new-style drivers

[PATCH 0/4] i2c: Add detection capability to new-style drivers

[Jean Delvare]

Hi all,

This is an update of the patch set named "i2c: Introduce i2c listeners"
which I posted 2 days ago. The only change if that the detect callback
is no longer in a separate i2c_listener structure, but instead is added
to struct i2c_driver (for new-style drivers only.) This makes things
much more simple.

The goal is to replace legacy i2c drivers. The functional differences
between legacy drivers and new-style ones are that legacy drivers get
notified when i2c adapters are created or deleted, they create their
own devices, and they can probe the hardware before they do so. There
are cases where we want to be able to do this (e.g. for hardware
monitoring devices on PC motherboards) so we can't just switch to
new-style everywhere. Still, legacy drivers are a pain to handle, they
cause headaches to developers because their locking model is weird, and
they duplicate a lot of code.

By implementing the needed functionality, almost unchanged, in a way
which is compatible with the new i2c device/driver binding model, my
hope is to be able to convert all legacy drivers to new-style drivers
in a matter of months. Without it, it would take years... or even never
happen at all. This patch set attempts to cover the device detection
part. The notification of additional/removal of adapters will be dealt
with later.

The mechanism behind the device detection callback is as follows:
* When a new-style i2c_driver is added, for each existing i2c_adapter,
  address_data (those format is the same as what i2c_probe() is already
  using for legacy drivers) is processed by i2c-core. For each relevant
  address, the detect callback will be called, with a pointer to an
  empty struct i2c_board_info address as the last parameter. The detect
  callback will attempt to identify a supported device at the given
  address, and if successful, it will fill the struct i2c_board_info
  with the parameters required to instantiate a new-style i2c device.
* When a new i2c_adapter is added, for each existing new-style
  i2c_driver, the same happens.
* When it gets a filled struct i2c_board_info from a detect callback,
  i2c-core will instantiate it. If a new-style driver exists for the
  device, it will be able to bind with the device.
* We keep track of the devices created that way, in a per-driver list.
* When a new-style i2c_driver is removed, all devices that originate
  from it are destroyed.
* When an i2c_adapter is removed, all devices on it that were created
  as the result of calling a detect callback, are destroyed.

So, drivers which currently implement both a new-style i2c_driver and a
legacy i2c_driver (or drivers which would be converted that way soon) can
instead implement a detect callback in their new-style i2c_driver.
There are two major advantages in doing so:
* All i2c drivers become new-style drivers. We get rid of legacy
  drivers altogether, allowing for long awaited cleanups in i2c-core.
* The code needed in each driver to implement a detect callback is way
  smaller than the code needed to implement a legacy driver, even when
  we were sharing as much code as possible between the new-style driver
  and the legacy driver. This is very visible in patches 3/4 and 4/4,
  which remove 50 lines of code per driver.

I would appreciate feedback on both the concept and the implementation.
David, I am particularly interested in your feedback, of course.

Patch 1/4 implements the mechanism, then patches 2/4, 3/4 and 4/4
demonstrate its use in 3 hardware monitoring drivers (lm90, f75375s and
lm75, respectively.) The patches go on top of 2.6.26-rc4 with some
additional dependencies listed in each patch. Testers welcome!

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH 1/4] i2c: Add detection capability to new-style drivers

[Jean Delvare]

Add a mechanism to let new-style i2c drivers auto-detect devices they
would support and ask i2c-core to instantiate them. This is a
replacement for legacy drivers, much cleaner.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
This patch depends on:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-use-class_for_each_device.patch
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-simplify-i2c_del_driver.patch

Implementation note: i2c_detect and i2c_detect_address are basically
clones of i2c_probe and i2c_probe_address but operating on a callback
with a slightly different prototype. This duplicated code accounts
for a large part of the patch, but once legacy drivers are gone,
i2c_probe and i2c_probe_address will be removed, so this is no
long-term duplication.

 drivers/i2c/i2c-core.c |  207 +++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/i2c.h    |   14 +++
 2 files changed, 210 insertions(+), 11 deletions(-)

--- linux-2.6.26-rc5.orig/include/linux/i2c.h	2008-06-05 21:04:35.000000000 +0200
+++ linux-2.6.26-rc5/include/linux/i2c.h	2008-06-05 21:54:49.000000000 +0200
@@ -43,6 +43,7 @@ struct i2c_adapter;
 struct i2c_client;
 struct i2c_driver;
 union i2c_smbus_data;
+struct i2c_board_info;
 
 /*
  * The master routines are the ones normally used to transmit data to devices
@@ -99,6 +100,11 @@ extern s32 i2c_smbus_write_i2c_block_dat
  *
  * The driver.owner field should be set to the module owner of this driver.
  * The driver.name field should be set to the name of this driver.
+ *
+ * @class: What kind of i2c device we may instantiate
+ * @detect: Callback for device detection
+ * @address_data: The I2C addresses to probe, ignore or force
+ * @clients: List of detected clients we created
  */
 
 struct i2c_driver {
@@ -140,6 +146,12 @@ struct i2c_driver {
 
 	struct device_driver driver;
 	const struct i2c_device_id *id_table;
+
+	/* Device detection callback for automatic device creation */
+	int (*detect)(struct i2c_adapter *, int address, int kind,
+		      struct i2c_board_info *);
+	const struct i2c_client_address_data *address_data;
+	struct list_head clients;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
@@ -155,6 +167,7 @@ struct i2c_driver {
  * @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 a 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
@@ -172,6 +185,7 @@ struct i2c_client {
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device (or -1) */
 	struct list_head list;		/* DEPRECATED */
+	struct list_head detected;
 	struct completion released;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
--- linux-2.6.26-rc5.orig/drivers/i2c/i2c-core.c	2008-06-05 21:04:35.000000000 +0200
+++ linux-2.6.26-rc5/drivers/i2c/i2c-core.c	2008-06-06 14:04:40.000000000 +0200
@@ -42,7 +42,9 @@
 static DEFINE_MUTEX(core_lock);
 static DEFINE_IDR(i2c_adapter_idr);
 
-#define is_newstyle_driver(d) ((d)->probe || (d)->remove)
+#define is_newstyle_driver(d) ((d)->probe || (d)->remove || (d)->detect)
+
+static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 
 /* ------------------------------------------------------------------------- */
 
@@ -409,6 +411,10 @@ static int i2c_do_add_adapter(struct dev
 	struct i2c_driver *driver = to_i2c_driver(d);
 	struct i2c_adapter *adap = data;
 
+	/* Detect supported devices on that bus, and instantiate them */
+	i2c_detect(adap, driver);
+
+	/* Let legacy drivers scan this bus for matching devices */
 	if (driver->attach_adapter) {
 		/* We ignore the return code; if it fails, too bad */
 		driver->attach_adapter(adap);
@@ -448,7 +454,7 @@ static int i2c_register_adapter(struct i
 	if (adap->nr < __i2c_first_dynamic_bus_num)
 		i2c_scan_static_board_info(adap);
 
-	/* let legacy drivers scan this bus for matching devices */
+	/* Notify drivers */
 	dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap,
 				 i2c_do_add_adapter);
 
@@ -554,14 +560,26 @@ static int i2c_do_del_adapter(struct dev
 {
 	struct i2c_driver *driver = to_i2c_driver(d);
 	struct i2c_adapter *adapter = data;
+	struct i2c_client *client, *_n;
 	int res;
 
+	/* Remove the devices we created ourselves */
+	list_for_each_entry_safe(client, _n, &driver->clients, detected) {
+		if (client->adapter == adapter) {
+			dev_dbg(&adapter->dev, "Removing %s at 0x%x\n",
+				client->name, client->addr);
+			list_del(&client->detected);
+			i2c_unregister_device(client);
+		}
+	}
+
 	if (!driver->detach_adapter)
 		return 0;
 	res = driver->detach_adapter(adapter);
 	if (res)
 		dev_err(&adapter->dev, "detach_adapter failed (%d) "
 			"for driver [%s]\n", res, driver->driver.name);
+
 	return res;
 }
 
@@ -642,7 +660,10 @@ static int __attach_adapter(struct devic
 	struct i2c_adapter *adapter = to_i2c_adapter(dev);
 	struct i2c_driver *driver = data;
 
-	driver->attach_adapter(adapter);
+	i2c_detect(adapter, driver);
+
+	if (driver->attach_adapter)
+		driver->attach_adapter(adapter);
 
 	return 0;
 }
@@ -686,10 +707,10 @@ int i2c_register_driver(struct module *o
 
 	pr_debug("i2c-core: driver [%s] registered\n", driver->driver.name);
 
-	/* legacy drivers scan i2c busses directly */
-	if (driver->attach_adapter)
-		class_for_each_device(&i2c_adapter_class, driver,
-				      __attach_adapter);
+	INIT_LIST_HEAD(&driver->clients);
+	/* Walk the adapters that are already present */
+	class_for_each_device(&i2c_adapter_class, driver,
+			      __attach_adapter);
 
 	mutex_unlock(&core_lock);
 	return 0;
@@ -700,6 +721,17 @@ static int __detach_adapter(struct devic
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(dev);
 	struct i2c_driver *driver = data;
+	struct i2c_client *client, *_n;
+
+	list_for_each_entry_safe(client, _n, &driver->clients, detected) {
+		dev_dbg(&adapter->dev, "Removing %s at 0x%x\n",
+			client->name, client->addr);
+		list_del(&client->detected);
+		i2c_unregister_device(client);
+	}
+
+	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
@@ -738,10 +770,7 @@ void i2c_del_driver(struct i2c_driver *d
 {
 	mutex_lock(&core_lock);
 
-	/* legacy driver? */
-	if (!is_newstyle_driver(driver))
-		class_for_each_device(&i2c_adapter_class, driver,
-				      __detach_adapter);
+	class_for_each_device(&i2c_adapter_class, driver, __detach_adapter);
 
 	driver_unregister(&driver->driver);
 	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
@@ -1196,6 +1225,162 @@ int i2c_probe(struct i2c_adapter *adapte
 }
 EXPORT_SYMBOL(i2c_probe);
 
+/* Separate detection function for new-style drivers */
+static int i2c_detect_address(struct i2c_adapter *adapter,
+			      int addr, int kind, struct i2c_driver *driver)
+{
+	struct i2c_board_info info;
+	struct i2c_client *client;
+	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 */
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	err = driver->detect(adapter, addr, kind, &info);
+	if (err) {
+		/* -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. */
+		return err == -ENODEV ? 0 : err;
+	}
+
+	/* Consistency check */
+	if (info.type[0] == '\0' || info.addr != addr) {
+		dev_err(&adapter->dev, "Detection function returned "
+			"inconsistent data for 0x%x\n", addr);
+	} else {
+		/* Detection succeeded, instantiate the device */
+		dev_dbg(&adapter->dev, "Creating %s at 0x%x\n",
+			info.type, info.addr);
+		client = i2c_new_device(adapter, &info);
+		list_add_tail(&client->detected, &driver->clients);
+	}
+	return 0;
+}
+
+static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
+{
+	const struct i2c_client_address_data *address_data;
+	int i, err;
+	int adap_id = i2c_adapter_id(adapter);
+
+	if (!driver->detect)
+		return 0;
+	address_data = driver->address_data;
+
+	/* 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_detect_address(adapter,
+						forces[kind][i + 1],
+						kind, driver);
+					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_warn(&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_detect_address(adapter,
+						 address_data->probe[i + 1],
+						 -1, driver);
+			if (err)
+				return err;
+		}
+	}
+
+	/* Stop here if the classes do not match */
+	if (!(adapter->class & driver->class))
+		return 0;
+
+	/* 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_detect_address(adapter, address_data->normal_i2c[i],
+					 -1, driver);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 struct i2c_client *
 i2c_new_probed_device(struct i2c_adapter *adap,
 		      struct i2c_board_info *info,

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH 2/4] i2c: Convert the lm90 driver to a new-style i2c driver

[Jean Delvare]

The new-style lm90 driver implements the optional detect() callback
to cover the use cases of the legacy driver.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/hwmon/lm90.c |  103 +++++++++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 43 deletions(-)

--- linux-2.6.26-rc5.orig/drivers/hwmon/lm90.c	2008-06-05 21:04:34.000000000 +0200
+++ linux-2.6.26-rc5/drivers/hwmon/lm90.c	2008-06-05 22:11:23.000000000 +0200
@@ -187,23 +187,40 @@ I2C_CLIENT_INSMOD_7(lm90, adm1032, lm99,
  * Functions declaration
  */
 
-static int lm90_attach_adapter(struct i2c_adapter *adapter);
 static int lm90_detect(struct i2c_adapter *adapter, int address,
-	int kind);
+		       int kind, struct i2c_board_info *info);
+static int lm90_probe(struct i2c_client *client,
+		      const struct i2c_device_id *id);
 static void lm90_init_client(struct i2c_client *client);
-static int lm90_detach_client(struct i2c_client *client);
+static int lm90_remove(struct i2c_client *client);
 static struct lm90_data *lm90_update_device(struct device *dev);
 
 /*
  * Driver data (common to all clients)
  */
 
+static struct i2c_device_id lm90_id[] = {
+	{ "lm90", lm90 },
+	{ "adm1032", adm1032 },
+	{ "lm99", lm99 },
+	{ "lm86", lm86 },
+	{ "max6657", max6657 },
+	{ "adt7461", adt7461 },
+	{ "max6680", max6680 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, lm90_id);
+
 static struct i2c_driver lm90_driver = {
+	.class		= I2C_CLASS_HWMON,
 	.driver = {
 		.name	= "lm90",
 	},
-	.attach_adapter	= lm90_attach_adapter,
-	.detach_client	= lm90_detach_client,
+	.probe		= lm90_probe,
+	.remove		= lm90_remove,
+	.id_table	= lm90_id,
+	.detect		= lm90_detect,
+	.address_data	= &addr_data,
 };
 
 /*
@@ -211,7 +228,6 @@ static struct i2c_driver lm90_driver = {
  */
 
 struct lm90_data {
-	struct i2c_client client;
 	struct device *hwmon_dev;
 	struct mutex update_lock;
 	char valid; /* zero until following fields are valid */
@@ -477,40 +493,26 @@ static int lm90_read_reg(struct i2c_clie
 	return 0;
 }
 
-static int lm90_attach_adapter(struct i2c_adapter *adapter)
-{
-	if (!(adapter->class & I2C_CLASS_HWMON))
-		return 0;
-	return i2c_probe(adapter, &addr_data, lm90_detect);
-}
-
-/*
- * The following function does more than just detection. If detection
- * succeeds, it also registers the new chip.
- */
-static int lm90_detect(struct i2c_adapter *adapter, int address, int kind)
+/* Returns -ENODEV if no supported device is detected */
+static int lm90_detect(struct i2c_adapter *adapter, int address, int kind,
+		       struct i2c_board_info *info)
 {
 	struct i2c_client *new_client;
-	struct lm90_data *data;
-	int err = 0;
+	int err = -ENODEV;
 	const char *name = "";
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		goto exit;
 
-	if (!(data = kzalloc(sizeof(struct lm90_data), GFP_KERNEL))) {
+	new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
+	if (!new_client) {
 		err = -ENOMEM;
 		goto exit;
 	}
 
-	/* The common I2C client data is placed right before the
-	   LM90-specific data. */
-	new_client = &data->client;
-	i2c_set_clientdata(new_client, data);
+	/* Enough to use smbus calls */
 	new_client->addr = address;
 	new_client->adapter = adapter;
-	new_client->driver = &lm90_driver;
-	new_client->flags = 0;
 
 	/*
 	 * Now we do the remaining detection. A negative kind means that
@@ -613,7 +615,10 @@ static int lm90_detect(struct i2c_adapte
 			goto exit_free;
 		}
 	}
+	err = 0;	/* detection OK */
 
+	/* Fill the i2c board info */
+	info->addr = address;
 	if (kind == lm90) {
 		name = "lm90";
 	} else if (kind == adm1032) {
@@ -621,7 +626,7 @@ static int lm90_detect(struct i2c_adapte
 		/* The ADM1032 supports PEC, but only if combined
 		   transactions are not used. */
 		if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
-			new_client->flags |= I2C_CLIENT_PEC;
+			info->flags |= I2C_CLIENT_PEC;
 	} else if (kind == lm99) {
 		name = "lm99";
 	} else if (kind == lm86) {
@@ -633,23 +638,41 @@ static int lm90_detect(struct i2c_adapte
 	} else if (kind == adt7461) {
 		name = "adt7461";
 	}
+	strlcpy(info->type, name, I2C_NAME_SIZE);
+
+exit_free:
+	kfree(new_client);
+exit:
+	return err;
+}
 
-	/* We can fill in the remaining client fields */
-	strlcpy(new_client->name, name, I2C_NAME_SIZE);
-	data->valid = 0;
-	data->kind = kind;
+static int lm90_probe(struct i2c_client *new_client,
+		      const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(new_client->dev.parent);
+	struct lm90_data *data;
+	int err;
+
+	if (!(data = kzalloc(sizeof(struct lm90_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	i2c_set_clientdata(new_client, data);
 	mutex_init(&data->update_lock);
 
-	/* Tell the I2C layer a new client has arrived */
-	if ((err = i2c_attach_client(new_client)))
-		goto exit_free;
+	/* Set the device type */
+	data->kind = id->driver_data;
+	if (data->kind == adm1032) {
+		if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
+			new_client->flags &= ~I2C_CLIENT_PEC;
+	}
 
 	/* Initialize the LM90 chip */
 	lm90_init_client(new_client);
 
 	/* Register sysfs hooks */
 	if ((err = sysfs_create_group(&new_client->dev.kobj, &lm90_group)))
-		goto exit_detach;
+		goto exit_free;
 	if (new_client->flags & I2C_CLIENT_PEC) {
 		if ((err = device_create_file(&new_client->dev,
 					      &dev_attr_pec)))
@@ -672,8 +695,6 @@ static int lm90_detect(struct i2c_adapte
 exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
 	device_remove_file(&new_client->dev, &dev_attr_pec);
-exit_detach:
-	i2c_detach_client(new_client);
 exit_free:
 	kfree(data);
 exit:
@@ -710,10 +731,9 @@ static void lm90_init_client(struct i2c_
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
-static int lm90_detach_client(struct i2c_client *client)
+static int lm90_remove(struct i2c_client *client)
 {
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int err;
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm90_group);
@@ -722,9 +742,6 @@ static int lm90_detach_client(struct i2c
 		device_remove_file(&client->dev,
 				   &sensor_dev_attr_temp2_offset.dev_attr);
 
-	if ((err = i2c_detach_client(client)))
-		return err;
-
 	kfree(data);
 	return 0;
 }

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH 3/4] i2c: Drop legacy f75375s driver

[Jean Delvare]

Drop the legacy f75375s driver, and add a detect callback to the
new-style driver to achieve the same functionality.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/hwmon/f75375s.c |   74 +++++++----------------------------------------
 1 file changed, 12 insertions(+), 62 deletions(-)

--- linux-2.6.26-rc5.orig/drivers/hwmon/f75375s.c	2008-06-05 21:04:26.000000000 +0200
+++ linux-2.6.26-rc5/drivers/hwmon/f75375s.c	2008-06-05 22:14:00.000000000 +0200
@@ -114,21 +114,12 @@ struct f75375_data {
 	s8 temp_max_hyst[2];
 };
 
-static int f75375_attach_adapter(struct i2c_adapter *adapter);
-static int f75375_detect(struct i2c_adapter *adapter, int address, int kind);
-static int f75375_detach_client(struct i2c_client *client);
+static int f75375_detect(struct i2c_adapter *adapter, int address, int kind,
+			 struct i2c_board_info *info);
 static int f75375_probe(struct i2c_client *client,
 			const struct i2c_device_id *id);
 static int f75375_remove(struct i2c_client *client);
 
-static struct i2c_driver f75375_legacy_driver = {
-	.driver = {
-		.name = "f75375_legacy",
-	},
-	.attach_adapter = f75375_attach_adapter,
-	.detach_client = f75375_detach_client,
-};
-
 static const struct i2c_device_id f75375_id[] = {
 	{ "f75373", f75373 },
 	{ "f75375", f75375 },
@@ -137,12 +128,15 @@ static const struct i2c_device_id f75375
 MODULE_DEVICE_TABLE(i2c, f75375_id);
 
 static struct i2c_driver f75375_driver = {
+	.class = I2C_CLASS_HWMON,
 	.driver = {
 		.name = "f75375",
 	},
 	.probe = f75375_probe,
 	.remove = f75375_remove,
 	.id_table = f75375_id,
+	.detect = f75375_detect,
+	.address_data = &addr_data,
 };
 
 static inline int f75375_read8(struct i2c_client *client, u8 reg)
@@ -607,22 +601,6 @@ static const struct attribute_group f753
 	.attrs = f75375_attributes,
 };
 
-static int f75375_detach_client(struct i2c_client *client)
-{
-	int err;
-
-	f75375_remove(client);
-	err = i2c_detach_client(client);
-	if (err) {
-		dev_err(&client->dev,
-			"Client deregistration failed, "
-			"client not detached.\n");
-		return err;
-	}
-	kfree(client);
-	return 0;
-}
-
 static void f75375_init(struct i2c_client *client, struct f75375_data *data,
 		struct f75375s_platform_data *f75375s_pdata)
 {
@@ -700,21 +678,14 @@ static int f75375_remove(struct i2c_clie
 	return 0;
 }
 
-static int f75375_attach_adapter(struct i2c_adapter *adapter)
-{
-	if (!(adapter->class & I2C_CLASS_HWMON))
-		return 0;
-	return i2c_probe(adapter, &addr_data, f75375_detect);
-}
-
 /* This function is called by i2c_probe */
-static int f75375_detect(struct i2c_adapter *adapter, int address, int kind)
+static int f75375_detect(struct i2c_adapter *adapter, int address, int kind,
+			 struct i2c_board_info *info)
 {
 	struct i2c_client *client;
 	u8 version = 0;
-	int err = 0;
+	int err = -ENODEV;
 	const char *name = "";
-	struct i2c_device_id id;
 
 	if (!(client = kzalloc(sizeof(*client), GFP_KERNEL))) {
 		err = -ENOMEM;
@@ -722,7 +693,6 @@ static int f75375_detect(struct i2c_adap
 	}
 	client->addr = address;
 	client->adapter = adapter;
-	client->driver = &f75375_legacy_driver;
 
 	if (kind < 0) {
 		u16 vendid = f75375_read16(client, F75375_REG_VENDOR);
@@ -739,6 +709,7 @@ static int f75375_detect(struct i2c_adap
 			goto exit_free;
 		}
 	}
+	err = 0;	/* detection OK */
 
 	if (kind == f75375) {
 		name = "f75375";
@@ -746,20 +717,9 @@ static int f75375_detect(struct i2c_adap
 		name = "f75373";
 	}
 	dev_info(&adapter->dev, "found %s version: %02X\n", name, version);
-	strlcpy(client->name, name, I2C_NAME_SIZE);
+	strlcpy(info->type, name, I2C_NAME_SIZE);
+	info->addr = address;
 
-	if ((err = i2c_attach_client(client)))
-		goto exit_free;
-
-	strlcpy(id.name, name, I2C_NAME_SIZE);
-	id.driver_data = kind;
-	if ((err = f75375_probe(client, &id)) < 0)
-		goto exit_detach;
-
-	return 0;
-
-exit_detach:
-	i2c_detach_client(client);
 exit_free:
 	kfree(client);
 exit:
@@ -768,21 +728,11 @@ exit:
 
 static int __init sensors_f75375_init(void)
 {
-	int status;
-	status = i2c_add_driver(&f75375_driver);
-	if (status)
-		return status;
-
-	status = i2c_add_driver(&f75375_legacy_driver);
-	if (status)
-		i2c_del_driver(&f75375_driver);
-
-	return status;
+	return i2c_add_driver(&f75375_driver);
 }
 
 static void __exit sensors_f75375_exit(void)
 {
-	i2c_del_driver(&f75375_legacy_driver);
 	i2c_del_driver(&f75375_driver);
 }
 

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 3/4] i2c: Drop legacy f75375s driver

[Voipio Riku]

Hi,

Tested with the following patchset:

i2c-use-class_for_each_device.patch
i2c-simplify-i2c_del_driver.patch
i2c-core-add-detection-capability-to-new-style-drivers.patch
hwmon-f75375s-drop-legacy-driver.patch

and f75375s still works mighty fine when probed new-style. The rtc and led
drivers (both new-style) attached to i2c bus work too.

Acked-by: Riku Voipio <riku.voipio-WgUW+8SLYMv1KXRcyAk9cg@public.gmane.org>

> Drop the legacy f75375s driver, and add a detect callback to the
> new-style driver to achieve the same functionality.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/hwmon/f75375s.c |   74
> +++++++----------------------------------------
>  1 file changed, 12 insertions(+), 62 deletions(-)
>
> --- linux-2.6.26-rc5.orig/drivers/hwmon/f75375s.c	2008-06-05
> 21:04:26.000000000 +0200
> +++ linux-2.6.26-rc5/drivers/hwmon/f75375s.c	2008-06-05 22:14:00.000000000
> +0200
> @@ -114,21 +114,12 @@ struct f75375_data {
>  	s8 temp_max_hyst[2];
>  };
>
> -static int f75375_attach_adapter(struct i2c_adapter *adapter);
> -static int f75375_detect(struct i2c_adapter *adapter, int address, int
> kind);
> -static int f75375_detach_client(struct i2c_client *client);
> +static int f75375_detect(struct i2c_adapter *adapter, int address, int
> kind,
> +			 struct i2c_board_info *info);
>  static int f75375_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id);
>  static int f75375_remove(struct i2c_client *client);
>
> -static struct i2c_driver f75375_legacy_driver = {
> -	.driver = {
> -		.name = "f75375_legacy",
> -	},
> -	.attach_adapter = f75375_attach_adapter,
> -	.detach_client = f75375_detach_client,
> -};
> -
>  static const struct i2c_device_id f75375_id[] = {
>  	{ "f75373", f75373 },
>  	{ "f75375", f75375 },
> @@ -137,12 +128,15 @@ static const struct i2c_device_id f75375
>  MODULE_DEVICE_TABLE(i2c, f75375_id);
>
>  static struct i2c_driver f75375_driver = {
> +	.class = I2C_CLASS_HWMON,
>  	.driver = {
>  		.name = "f75375",
>  	},
>  	.probe = f75375_probe,
>  	.remove = f75375_remove,
>  	.id_table = f75375_id,
> +	.detect = f75375_detect,
> +	.address_data = &addr_data,
>  };
>
>  static inline int f75375_read8(struct i2c_client *client, u8 reg)
> @@ -607,22 +601,6 @@ static const struct attribute_group f753
>  	.attrs = f75375_attributes,
>  };
>
> -static int f75375_detach_client(struct i2c_client *client)
> -{
> -	int err;
> -
> -	f75375_remove(client);
> -	err = i2c_detach_client(client);
> -	if (err) {
> -		dev_err(&client->dev,
> -			"Client deregistration failed, "
> -			"client not detached.\n");
> -		return err;
> -	}
> -	kfree(client);
> -	return 0;
> -}
> -
>  static void f75375_init(struct i2c_client *client, struct f75375_data
> *data,
>  		struct f75375s_platform_data *f75375s_pdata)
>  {
> @@ -700,21 +678,14 @@ static int f75375_remove(struct i2c_clie
>  	return 0;
>  }
>
> -static int f75375_attach_adapter(struct i2c_adapter *adapter)
> -{
> -	if (!(adapter->class & I2C_CLASS_HWMON))
> -		return 0;
> -	return i2c_probe(adapter, &addr_data, f75375_detect);
> -}
> -
>  /* This function is called by i2c_probe */
> -static int f75375_detect(struct i2c_adapter *adapter, int address, int
> kind)
> +static int f75375_detect(struct i2c_adapter *adapter, int address, int
> kind,
> +			 struct i2c_board_info *info)
>  {
>  	struct i2c_client *client;
>  	u8 version = 0;
> -	int err = 0;
> +	int err = -ENODEV;
>  	const char *name = "";
> -	struct i2c_device_id id;
>
>  	if (!(client = kzalloc(sizeof(*client), GFP_KERNEL))) {
>  		err = -ENOMEM;
> @@ -722,7 +693,6 @@ static int f75375_detect(struct i2c_adap
>  	}
>  	client->addr = address;
>  	client->adapter = adapter;
> -	client->driver = &f75375_legacy_driver;
>
>  	if (kind < 0) {
>  		u16 vendid = f75375_read16(client, F75375_REG_VENDOR);
> @@ -739,6 +709,7 @@ static int f75375_detect(struct i2c_adap
>  			goto exit_free;
>  		}
>  	}
> +	err = 0;	/* detection OK */
>
>  	if (kind == f75375) {
>  		name = "f75375";
> @@ -746,20 +717,9 @@ static int f75375_detect(struct i2c_adap
>  		name = "f75373";
>  	}
>  	dev_info(&adapter->dev, "found %s version: %02X\n", name, version);
> -	strlcpy(client->name, name, I2C_NAME_SIZE);
> +	strlcpy(info->type, name, I2C_NAME_SIZE);
> +	info->addr = address;
>
> -	if ((err = i2c_attach_client(client)))
> -		goto exit_free;
> -
> -	strlcpy(id.name, name, I2C_NAME_SIZE);
> -	id.driver_data = kind;
> -	if ((err = f75375_probe(client, &id)) < 0)
> -		goto exit_detach;
> -
> -	return 0;
> -
> -exit_detach:
> -	i2c_detach_client(client);
>  exit_free:
>  	kfree(client);
>  exit:
> @@ -768,21 +728,11 @@ exit:
>
>  static int __init sensors_f75375_init(void)
>  {
> -	int status;
> -	status = i2c_add_driver(&f75375_driver);
> -	if (status)
> -		return status;
> -
> -	status = i2c_add_driver(&f75375_legacy_driver);
> -	if (status)
> -		i2c_del_driver(&f75375_driver);
> -
> -	return status;
> +	return i2c_add_driver(&f75375_driver);
>  }
>
>  static void __exit sensors_f75375_exit(void)
>  {
> -	i2c_del_driver(&f75375_legacy_driver);
>  	i2c_del_driver(&f75375_driver);
>  }
>
>
> --
> Jean Delvare
>


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 3/4] i2c: Drop legacy f75375s driver

[Jean Delvare]

On Sat, 28 Jun 2008 21:46:01 +0300 (EEST), Voipio Riku wrote:
> Hi,
> 
> Tested with the following patchset:
> 
> i2c-use-class_for_each_device.patch
> i2c-simplify-i2c_del_driver.patch
> i2c-core-add-detection-capability-to-new-style-drivers.patch
> hwmon-f75375s-drop-legacy-driver.patch
> 
> and f75375s still works mighty fine when probed new-style. The rtc and led
> drivers (both new-style) attached to i2c bus work too.
> 
> Acked-by: Riku Voipio <riku.voipio-WgUW+8SLYMv1KXRcyAk9cg@public.gmane.org>

Great, thanks for testing Riku!

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH 4/4] i2c: Drop legacy lm75 driver

[Jean Delvare]

Drop the legacy lm75 driver, and add a detect callback to the
new-style driver to achieve the same functionality.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
This goes on top of David Brownell's patches:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-April/022931.html
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023040.html
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023041.html

Also available from:
http://jdelvare.pck.nerim.net/sensors/lm75/

 drivers/hwmon/lm75.c |  120 ++++++++++++++------------------------------------
 1 file changed, 35 insertions(+), 85 deletions(-)

--- linux-2.6.26-rc5.orig/drivers/hwmon/lm75.c	2008-06-05 21:04:26.000000000 +0200
+++ linux-2.6.26-rc5/drivers/hwmon/lm75.c	2008-06-05 22:19:23.000000000 +0200
@@ -54,11 +54,11 @@ enum lm75_type {		/* keep sorted in alph
 	tmp75,
 };
 
-/* Addresses scanned by legacy style driver binding */
+/* Addresses scanned */
 static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, 0x4c,
 					0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
 
-/* Insmod parameters (only for legacy style driver binding) */
+/* Insmod parameters */
 I2C_CLIENT_INSMOD_1(lm75);
 
 
@@ -217,46 +217,12 @@ static int lm75_remove(struct i2c_client
 	return 0;
 }
 
-static const struct i2c_device_id lm75_ids[] = {
-	{ "ds1775", ds1775, },
-	{ "ds75", ds75, },
-	{ "lm75", lm75, },
-	{ "lm75a", lm75a, },
-	{ "max6625", max6625, },
-	{ "max6626", max6626, },
-	{ "mcp980x", mcp980x, },
-	{ "stds75", stds75, },
-	{ "tcn75", tcn75, },
-	{ "tmp100", tmp100, },
-	{ "tmp101", tmp101, },
-	{ "tmp175", tmp175, },
-	{ "tmp275", tmp275, },
-	{ "tmp75", tmp75, },
-	{ /* LIST END */ }
-};
-MODULE_DEVICE_TABLE(i2c, lm75_ids);
-
-static struct i2c_driver lm75_driver = {
-	.driver = {
-		.name	= "lm75",
-	},
-	.probe		= lm75_probe,
-	.remove		= lm75_remove,
-	.id_table	= lm75_ids,
-};
-
-/*-----------------------------------------------------------------------*/
-
-/* "Legacy" I2C driver binding */
-
-static struct i2c_driver lm75_legacy_driver;
-
-/* This function is called by i2c_probe */
-static int lm75_detect(struct i2c_adapter *adapter, int address, int kind)
+static int lm75_detect(struct i2c_adapter *adapter, int address, int kind,
+		       struct i2c_board_info *info)
 {
 	int i;
 	struct i2c_client *new_client;
-	int err = 0;
+	int err = -ENODEV;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
 				     I2C_FUNC_SMBUS_WORD_DATA))
@@ -273,8 +239,6 @@ static int lm75_detect(struct i2c_adapte
 
 	new_client->addr = address;
 	new_client->adapter = adapter;
-	new_client->driver = &lm75_legacy_driver;
-	new_client->flags = 0;
 
 	/* Now, we do the remaining detection. There is no identification-
 	   dedicated register so we have to rely on several tricks:
@@ -313,52 +277,49 @@ static int lm75_detect(struct i2c_adapte
 			 || i2c_smbus_read_word_data(new_client, i + 3) != os)
 				goto exit_free;
 	}
+	err = 0;	/* detection OK */
 
 	/* NOTE: we treat "force=..." and "force_lm75=..." the same.
 	 * Only new-style driver binding distinguishes chip types.
 	 */
-	strlcpy(new_client->name, "lm75", I2C_NAME_SIZE);
-
-	/* Tell the I2C layer a new client has arrived */
-	err = i2c_attach_client(new_client);
-	if (err)
-		goto exit_free;
-
-	err = lm75_probe(new_client, NULL);
-	if (err < 0)
-		goto exit_detach;
+	strlcpy(info->type, "lm75", I2C_NAME_SIZE);
+	info->addr = address;
 
-	return 0;
-
-exit_detach:
-	i2c_detach_client(new_client);
 exit_free:
 	kfree(new_client);
 exit:
 	return err;
 }
 
-static int lm75_attach_adapter(struct i2c_adapter *adapter)
-{
-	if (!(adapter->class & I2C_CLASS_HWMON))
-		return 0;
-	return i2c_probe(adapter, &addr_data, lm75_detect);
-}
-
-static int lm75_detach_client(struct i2c_client *client)
-{
-	lm75_remove(client);
-	i2c_detach_client(client);
-	kfree(client);
-	return 0;
-}
+static const struct i2c_device_id lm75_ids[] = {
+	{ "ds1775", ds1775, },
+	{ "ds75", ds75, },
+	{ "lm75", lm75, },
+	{ "lm75a", lm75a, },
+	{ "max6625", max6625, },
+	{ "max6626", max6626, },
+	{ "mcp980x", mcp980x, },
+	{ "stds75", stds75, },
+	{ "tcn75", tcn75, },
+	{ "tmp100", tmp100, },
+	{ "tmp101", tmp101, },
+	{ "tmp175", tmp175, },
+	{ "tmp275", tmp275, },
+	{ "tmp75", tmp75, },
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(i2c, lm75_ids);
 
-static struct i2c_driver lm75_legacy_driver = {
+static struct i2c_driver lm75_driver = {
+	.class		= I2C_CLASS_HWMON,
 	.driver = {
-		.name	= "lm75_legacy",
+		.name	= "lm75",
 	},
-	.attach_adapter	= lm75_attach_adapter,
-	.detach_client	= lm75_detach_client,
+	.probe		= lm75_probe,
+	.remove		= lm75_remove,
+	.id_table	= lm75_ids,
+	.detect		= lm75_detect,
+	.address_data	= &addr_data,
 };
 
 /*-----------------------------------------------------------------------*/
@@ -424,22 +385,11 @@ static struct lm75_data *lm75_update_dev
 
 static int __init sensors_lm75_init(void)
 {
-	int status;
-
-	status = i2c_add_driver(&lm75_driver);
-	if (status < 0)
-		return status;
-
-	status = i2c_add_driver(&lm75_legacy_driver);
-	if (status < 0)
-		i2c_del_driver(&lm75_driver);
-
-	return status;
+	return i2c_add_driver(&lm75_driver);
 }
 
 static void __exit sensors_lm75_exit(void)
 {
-	i2c_del_driver(&lm75_legacy_driver);
 	i2c_del_driver(&lm75_driver);
 }
 

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 0/4] i2c: Add detection capability to new-style drivers

[Trent Piepho]

On Fri, 6 Jun 2008, Jean Delvare wrote:
> The mechanism behind the device detection callback is as follows:
> * When a new-style i2c_driver is added, for each existing i2c_adapter,
>   address_data (those format is the same as what i2c_probe() is already
>   using for legacy drivers) is processed by i2c-core. For each relevant
>   address, the detect callback will be called, with a pointer to an
>   empty struct i2c_board_info address as the last parameter. The detect
>   callback will attempt to identify a supported device at the given
>   address, and if successful, it will fill the struct i2c_board_info
>   with the parameters required to instantiate a new-style i2c device.

Isn't the detect callback going to be the same for almost every driver?

It seems like you could have the i2c core do the detection.  Take the "to
be probed" addresses listed by the driver, do a standard i2c probe on them,
and pass a device to the driver's probe function if something is detected.
The probe function is of course free to return ENODEV if it has some
additional way to check for device presence.  If an address is on the force
list, then the device is created and passed to the driver's probe without
the core doing any kind of i2c probe.  The driver can do a non-standard
probe if it has one and return ENODEV if nothing is there.

> * When a new i2c_adapter is added, for each existing new-style
>   i2c_driver, the same happens.

It would nice if there was a no-probe option when creating an i2c_adapter.
Some hardware doesn't support probing of any kind, usually because there is
no way to detect a NAK.  In some cases the creator of the i2c_adapter knows
exactly what's on the bus it's creating and doesn't want whatever random
i2c drivers happen to be loaded to probe it.  For instance, the ivtv TV
card driver knows what tuners are on the i2c bus, while the bttv driver
doesn't.  The ivtv driver could use this to prevent an i2c driver used by
bttv from probing ivtv's i2c bus.

> * When it gets a filled struct i2c_board_info from a detect callback,
>   i2c-core will instantiate it. If a new-style driver exists for the
>   device, it will be able to bind with the device.
> * We keep track of the devices created that way, in a per-driver list.
> * When a new-style i2c_driver is removed, all devices that originate
>   from it are destroyed.

Is it necessary to remove the devices?  They are still there after all.
It's perfectly normal to have devices with no drivers loaded for them on
other busses.

> * When an i2c_adapter is removed, all devices on it that were created
>   as the result of calling a detect callback, are destroyed.

Shouldn't all devices on it, no matter how they were created, be removed?
Or is that the case?

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 0/4] i2c: Add detection capability to new-style drivers

[Jean Delvare]

Hi Trent,

On Fri, 6 Jun 2008 16:17:03 -0700 (PDT), Trent Piepho wrote:
> On Fri, 6 Jun 2008, Jean Delvare wrote:
> > The mechanism behind the device detection callback is as follows:
> > * When a new-style i2c_driver is added, for each existing i2c_adapter,
> >   address_data (those format is the same as what i2c_probe() is already
> >   using for legacy drivers) is processed by i2c-core. For each relevant
> >   address, the detect callback will be called, with a pointer to an
> >   empty struct i2c_board_info address as the last parameter. The detect
> >   callback will attempt to identify a supported device at the given
> >   address, and if successful, it will fill the struct i2c_board_info
> >   with the parameters required to instantiate a new-style i2c device.
> 
> Isn't the detect callback going to be the same for almost every driver?

Depends on what you call "the same". Basically every detect callback
will check whether the underlying adapter supports all the bus
transactions they need, then read many register values and compare what
it gets with well-known (device and driver specific) values, and from
there it will decide if it recognized a device it supports, and what
device that is.

However, each driver will read different register values, and compare
them to different constants. Just look at the detect callbacks of the
lm90, f75375s and lm75 drivers which I have posted, you'll see that the
code is very different. That's definitely not something we easily can
put in data structures and let i2c-core operate on.

One thing that pretty much all detect function will have in common is
the allocation of a fake i2c_client structure, to be able to use
i2c_smbus_read_*() functions. I am not totally satisfied with the
current implementation, because allocating a temporary structure
dynamically for every detect call looks inefficient. But having it on
the stack isn't an option, it's too large.

We could have i2c-core allocate it once and for all (detections are
serialized anyway) and pass it to the detect callbacks, but I a am bit
reluctant to make the existence of this fake i2c_client official,
because it looks like a hack.

The alternatives I can think of are:
* Calling i2c_smbus_xfer directly - but this will result in even more
  code duplication in the detect callbacks.
* Duplicating i2c_smbus_read_byte_data() and i2c_smbus_read_word_data()
  (the only callbacks which I expect to be used in a detect callback)
  with new prototypes which do not involve i2c_client. This might
  however become confusing for the driver authors.

None of these solutions make me totally happy, but I guess that the
most reasonable one is the second one (have i2c-core allocate
i2c_client and pass it to detect callbacks.) Maybe if we document
properly that the only thing one is allowed to do with this i2c_client
is call i2c_smbus_read_byte_data() and i2c_smbus_read_word_data(), it's
acceptable. I welcome comments and ideas on this problem.

> It seems like you could have the i2c core do the detection.  Take the "to
> be probed" addresses listed by the driver, do a standard i2c probe on them,

If by "standard i2c probe" you mean issuing an SMBus quick command to
verify if a device is present at all, that's already the case.

> and pass a device to the driver's probe function if something is detected.
> The probe function is of course free to return ENODEV if it has some
> additional way to check for device presence.  If an address is on the force
> list, then the device is created and passed to the driver's probe without
> the core doing any kind of i2c probe.  The driver can do a non-standard
> probe if it has one and return ENODEV if nothing is there.

I've said it several times before and I'll repeat it until everybody
gets it: 1* probe can't be called before the i2c device is created with
a name, and the i2c device can't get its name (in the context of device
auto-detection) before it has been detected, and 2* returning -ENODEV
in a probe function does NOT destroy the device, so we cannot use this
mechanism to validate the creation of a detected device.

So, no, calling probe directly before calling detect, cannot work,
unless you diverge from the standard device driver model once again (but
David will kill you, and I'll help him!)

> > * When a new i2c_adapter is added, for each existing new-style
> >   i2c_driver, the same happens.
> 
> It would nice if there was a no-probe option when creating an i2c_adapter.

This is the default. Probing (actually, detecting) only happens from
drivers which share a class bit with the i2c_adapter. If an i2c_adapter
doesn't define any class then it never gets probed. This is a
significant improvement compared to the previous situation where the
class check was left to the legacy drivers, and not all of them were
checking properly.

> > * When it gets a filled struct i2c_board_info from a detect callback,
> >   i2c-core will instantiate it. If a new-style driver exists for the
> >   device, it will be able to bind with the device.
> > * We keep track of the devices created that way, in a per-driver list.
> > * When a new-style i2c_driver is removed, all devices that originate
> >   from it are destroyed.
> 
> Is it necessary to remove the devices?  They are still there after all.
> It's perfectly normal to have devices with no drivers loaded for them on
> other busses.

What if I the auto-detection fails and and I want to unload and reload
the driver with the proper force parameter? If the devices aren't
destroyed by the driver which created them, then only a reboot will let
me do that. Not good.

Anyway, I don't think it is generally acceptable to have a situation
which is different before and after loading and unloading a driver.
Drivers should always clean up after themselves, as much as possible
and reasonable.

> > * When an i2c_adapter is removed, all devices on it that were created
> >   as the result of calling a detect callback, are destroyed.
> 
> Shouldn't all devices on it, no matter how they were created, be removed?
> Or is that the case?

Yes it is the case. But we must remove the references to them from all
i2c_driver client lists (otherwise we will access i2c_client structs
which have been already freed) so we already have to walk these lists.
Removing the devices at this point is cheap. And I tend to think it's
cleaner to destroy the devices in the same way they were created. But
I'm not insisting, we can remove the call to i2c_unregister_device()
from i2c_do_del_adapter() is you and others feel it's redundant.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 0/4] i2c: Add detection capability to new-style drivers

[Jean Delvare]

On Sat, 7 Jun 2008 09:35:15 +0200, Jean Delvare wrote:
> None of these solutions make me totally happy, but I guess that the
> most reasonable one is the second one (have i2c-core allocate
> i2c_client and pass it to detect callbacks.) Maybe if we document
> properly that the only thing one is allowed to do with this i2c_client
> is call i2c_smbus_read_byte_data() and i2c_smbus_read_word_data(), it's
> acceptable. I welcome comments and ideas on this problem.

Here's what it would look like (patch applies on top of the last series
I posted, for the moment.) That's probably better than my original
proposal. The diffstat clearly shows how it simplifies the detect
callback of each chip driver.

---
 drivers/hwmon/f75375s.c |   24 +++++-----------------
 drivers/hwmon/lm75.c    |   35 ++++++++------------------------
 drivers/hwmon/lm90.c    |   34 +++++++++----------------------
 drivers/i2c/i2c-core.c  |   50 +++++++++++++++++++++++++++++------------------
 include/linux/i2c.h     |    3 --
 5 files changed, 57 insertions(+), 89 deletions(-)

--- linux-2.6.26-rc5.orig/drivers/hwmon/f75375s.c	2008-06-07 10:52:59.000000000 +0200
+++ linux-2.6.26-rc5/drivers/hwmon/f75375s.c	2008-06-07 11:14:47.000000000 +0200
@@ -114,7 +114,7 @@ struct f75375_data {
 	s8 temp_max_hyst[2];
 };
 
-static int f75375_detect(struct i2c_adapter *adapter, int address, int kind,
+static int f75375_detect(struct i2c_client *client, int kind,
 			 struct i2c_board_info *info);
 static int f75375_probe(struct i2c_client *client,
 			const struct i2c_device_id *id);
@@ -679,21 +679,13 @@ static int f75375_remove(struct i2c_clie
 }
 
 /* This function is called by i2c_probe */
-static int f75375_detect(struct i2c_adapter *adapter, int address, int kind,
+static int f75375_detect(struct i2c_client *client, int kind,
 			 struct i2c_board_info *info)
 {
-	struct i2c_client *client;
+	struct i2c_adapter *adapter = client->adapter;
 	u8 version = 0;
-	int err = -ENODEV;
 	const char *name = "";
 
-	if (!(client = kzalloc(sizeof(*client), GFP_KERNEL))) {
-		err = -ENOMEM;
-		goto exit;
-	}
-	client->addr = address;
-	client->adapter = adapter;
-
 	if (kind < 0) {
 		u16 vendid = f75375_read16(client, F75375_REG_VENDOR);
 		u16 chipid = f75375_read16(client, F75375_CHIP_ID);
@@ -706,10 +698,9 @@ static int f75375_detect(struct i2c_adap
 			dev_err(&adapter->dev,
 				"failed,%02X,%02X,%02X\n",
 				chipid, version, vendid);
-			goto exit_free;
+			return -ENODEV;
 		}
 	}
-	err = 0;	/* detection OK */
 
 	if (kind == f75375) {
 		name = "f75375";
@@ -718,12 +709,9 @@ static int f75375_detect(struct i2c_adap
 	}
 	dev_info(&adapter->dev, "found %s version: %02X\n", name, version);
 	strlcpy(info->type, name, I2C_NAME_SIZE);
-	info->addr = address;
+	info->addr = client->addr;
 
-exit_free:
-	kfree(client);
-exit:
-	return err;
+	return 0;
 }
 
 static int __init sensors_f75375_init(void)
--- linux-2.6.26-rc5.orig/drivers/hwmon/lm75.c	2008-06-06 20:25:48.000000000 +0200
+++ linux-2.6.26-rc5/drivers/hwmon/lm75.c	2008-06-07 11:20:37.000000000 +0200
@@ -217,28 +217,15 @@ static int lm75_remove(struct i2c_client
 	return 0;
 }
 
-static int lm75_detect(struct i2c_adapter *adapter, int address, int kind,
+static int lm75_detect(struct i2c_client *new_client, int kind,
 		       struct i2c_board_info *info)
 {
+	struct i2c_adapter *adapter = new_client->adapter;
 	int i;
-	struct i2c_client *new_client;
-	int err = -ENODEV;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
 				     I2C_FUNC_SMBUS_WORD_DATA))
-		goto exit;
-
-	/* OK. For now, we presume we have a valid address. We create the
-	   client structure, even though there may be no sensor present.
-	   But it allows us to use i2c_smbus_read_*_data() calls. */
-	new_client = kzalloc(sizeof *new_client, GFP_KERNEL);
-	if (!new_client) {
-		err = -ENOMEM;
-		goto exit;
-	}
-
-	new_client->addr = address;
-	new_client->adapter = adapter;
+		return -ENODEV;
 
 	/* Now, we do the remaining detection. There is no identification-
 	   dedicated register so we have to rely on several tricks:
@@ -258,37 +245,33 @@ static int lm75_detect(struct i2c_adapte
 		 || i2c_smbus_read_word_data(new_client, 5) != hyst
 		 || i2c_smbus_read_word_data(new_client, 6) != hyst
 		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
-			goto exit_free;
+			return -ENODEV;
 		os = i2c_smbus_read_word_data(new_client, 3);
 		if (i2c_smbus_read_word_data(new_client, 4) != os
 		 || i2c_smbus_read_word_data(new_client, 5) != os
 		 || i2c_smbus_read_word_data(new_client, 6) != os
 		 || i2c_smbus_read_word_data(new_client, 7) != os)
-			goto exit_free;
+			return -ENODEV;
 
 		/* Unused bits */
 		if (conf & 0xe0)
-			goto exit_free;
+			return -ENODEV;
 
 		/* Addresses cycling */
 		for (i = 8; i < 0xff; i += 8)
 			if (i2c_smbus_read_byte_data(new_client, i + 1) != conf
 			 || i2c_smbus_read_word_data(new_client, i + 2) != hyst
 			 || i2c_smbus_read_word_data(new_client, i + 3) != os)
-				goto exit_free;
+				return -ENODEV;
 	}
-	err = 0;	/* detection OK */
 
 	/* NOTE: we treat "force=..." and "force_lm75=..." the same.
 	 * Only new-style driver binding distinguishes chip types.
 	 */
 	strlcpy(info->type, "lm75", I2C_NAME_SIZE);
-	info->addr = address;
+	info->addr = new_client->addr;
 
-exit_free:
-	kfree(new_client);
-exit:
-	return err;
+	return 0;
 }
 
 static const struct i2c_device_id lm75_ids[] = {
--- linux-2.6.26-rc5.orig/drivers/hwmon/lm90.c	2008-06-06 20:25:48.000000000 +0200
+++ linux-2.6.26-rc5/drivers/hwmon/lm90.c	2008-06-07 10:33:03.000000000 +0200
@@ -187,8 +187,8 @@ I2C_CLIENT_INSMOD_7(lm90, adm1032, lm99,
  * Functions declaration
  */
 
-static int lm90_detect(struct i2c_adapter *adapter, int address,
-		       int kind, struct i2c_board_info *info);
+static int lm90_detect(struct i2c_client *new_client, int kind,
+		       struct i2c_board_info *info);
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id);
 static void lm90_init_client(struct i2c_client *client);
@@ -494,25 +494,15 @@ static int lm90_read_reg(struct i2c_clie
 }
 
 /* Returns -ENODEV if no supported device is detected */
-static int lm90_detect(struct i2c_adapter *adapter, int address, int kind,
+static int lm90_detect(struct i2c_client *new_client, int kind,
 		       struct i2c_board_info *info)
 {
-	struct i2c_client *new_client;
-	int err = -ENODEV;
+	struct i2c_adapter *adapter = new_client->adapter;
+	int address = new_client->addr;
 	const char *name = "";
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
-		goto exit;
-
-	new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
-	if (!new_client) {
-		err = -ENOMEM;
-		goto exit;
-	}
-
-	/* Enough to use smbus calls */
-	new_client->addr = address;
-	new_client->adapter = adapter;
+		return -ENODEV;
 
 	/*
 	 * Now we do the remaining detection. A negative kind means that
@@ -540,7 +530,7 @@ static int lm90_detect(struct i2c_adapte
 						LM90_REG_R_CONFIG1)) < 0
 		 || (reg_convrate = i2c_smbus_read_byte_data(new_client,
 						LM90_REG_R_CONVRATE)) < 0)
-			goto exit_free;
+			return -ENODEV;
 		
 		if ((address == 0x4C || address == 0x4D)
 		 && man_id == 0x01) { /* National Semiconductor */
@@ -548,7 +538,7 @@ static int lm90_detect(struct i2c_adapte
 
 			if ((reg_config2 = i2c_smbus_read_byte_data(new_client,
 						LM90_REG_R_CONFIG2)) < 0)
-				goto exit_free;
+				return -ENODEV;
 
 			if ((reg_config1 & 0x2A) == 0x00
 			 && (reg_config2 & 0xF8) == 0x00
@@ -612,10 +602,9 @@ static int lm90_detect(struct i2c_adapte
 			dev_info(&adapter->dev,
 			    "Unsupported chip (man_id=0x%02X, "
 			    "chip_id=0x%02X).\n", man_id, chip_id);
-			goto exit_free;
+			return -ENODEV;
 		}
 	}
-	err = 0;	/* detection OK */
 
 	/* Fill the i2c board info */
 	info->addr = address;
@@ -640,10 +629,7 @@ static int lm90_detect(struct i2c_adapte
 	}
 	strlcpy(info->type, name, I2C_NAME_SIZE);
 
-exit_free:
-	kfree(new_client);
-exit:
-	return err;
+	return 0;
 }
 
 static int lm90_probe(struct i2c_client *new_client,
--- linux-2.6.26-rc5.orig/drivers/i2c/i2c-core.c	2008-06-06 20:25:48.000000000 +0200
+++ linux-2.6.26-rc5/drivers/i2c/i2c-core.c	2008-06-07 10:48:06.000000000 +0200
@@ -1226,11 +1226,12 @@ int i2c_probe(struct i2c_adapter *adapte
 EXPORT_SYMBOL(i2c_probe);
 
 /* Separate detection function for new-style drivers */
-static int i2c_detect_address(struct i2c_adapter *adapter,
-			      int addr, int kind, struct i2c_driver *driver)
+static int i2c_detect_address(struct i2c_client *temp_client, int kind,
+			      struct i2c_driver *driver)
 {
 	struct i2c_board_info info;
-	struct i2c_client *client;
+	struct i2c_adapter *adapter = temp_client->adapter;
+	int addr = temp_client->addr;
 	int err;
 
 	/* Make sure the address is valid */
@@ -1258,7 +1259,7 @@ static int i2c_detect_address(struct i2c
 
 	/* Finally call the custom detection function */
 	memset(&info, 0, sizeof(struct i2c_board_info));
-	err = driver->detect(adapter, addr, kind, &info);
+	err = driver->detect(temp_client, kind, &info);
 	if (err) {
 		/* -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
@@ -1271,6 +1272,8 @@ static int i2c_detect_address(struct i2c
 		dev_err(&adapter->dev, "Detection function returned "
 			"inconsistent data for 0x%x\n", addr);
 	} else {
+		struct i2c_client *client;
+
 		/* Detection succeeded, instantiate the device */
 		dev_dbg(&adapter->dev, "Creating %s at 0x%x\n",
 			info.type, info.addr);
@@ -1283,13 +1286,20 @@ static int i2c_detect_address(struct i2c
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 {
 	const struct i2c_client_address_data *address_data;
-	int i, err;
+	struct i2c_client *temp_client;
+	int i, err = 0;
 	int adap_id = i2c_adapter_id(adapter);
 
 	if (!driver->detect)
 		return 0;
 	address_data = driver->address_data;
 
+	/* Set up a temporary client to help detect callback */
+	temp_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
+	if (!temp_client)
+		return -ENOMEM;
+	temp_client->adapter = adapter;
+
 	/* Force entries are done first, and are not affected by ignore
 	   entries */
 	if (address_data->forces) {
@@ -1306,11 +1316,11 @@ static int i2c_detect(struct i2c_adapter
 						"addr 0x%02x, kind %d\n",
 						adap_id, forces[kind][i + 1],
 						kind);
-					err = i2c_detect_address(adapter,
-						forces[kind][i + 1],
+					temp_client->addr = forces[kind][i + 1];
+					err = i2c_detect_address(temp_client,
 						kind, driver);
 					if (err)
-						return err;
+						goto exit_free;
 				}
 			}
 		}
@@ -1320,11 +1330,12 @@ static int i2c_detect(struct i2c_adapter
 	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;
+			goto exit_free;
 
 		dev_warn(&adapter->dev, "SMBus Quick command not supported, "
 			 "can't probe for chips\n");
-		return -EOPNOTSUPP;
+		err = -EOPNOTSUPP;
+		goto exit_free;
 	}
 
 	/* Probe entries are done second, and are not affected by ignore
@@ -1335,17 +1346,16 @@ static int i2c_detect(struct i2c_adapter
 			dev_dbg(&adapter->dev, "found probe parameter for "
 				"adapter %d, addr 0x%02x\n", adap_id,
 				address_data->probe[i + 1]);
-			err = i2c_detect_address(adapter,
-						 address_data->probe[i + 1],
-						 -1, driver);
+			temp_client->addr = address_data->probe[i + 1];
+			err = i2c_detect_address(temp_client, -1, driver);
 			if (err)
-				return err;
+				goto exit_free;
 		}
 	}
 
 	/* Stop here if the classes do not match */
 	if (!(adapter->class & driver->class))
-		return 0;
+		goto exit_free;
 
 	/* Normal entries are done last, unless shadowed by an ignore entry */
 	for (i = 0; address_data->normal_i2c[i] != I2C_CLIENT_END; i += 1) {
@@ -1372,13 +1382,15 @@ static int i2c_detect(struct i2c_adapter
 		dev_dbg(&adapter->dev, "found normal entry for adapter %d, "
 			"addr 0x%02x\n", adap_id,
 			address_data->normal_i2c[i]);
-		err = i2c_detect_address(adapter, address_data->normal_i2c[i],
-					 -1, driver);
+		temp_client->addr = address_data->normal_i2c[i];
+		err = i2c_detect_address(temp_client, -1, driver);
 		if (err)
-			return err;
+			goto exit_free;
 	}
 
-	return 0;
+ exit_free:
+	kfree(temp_client);
+	return err;
 }
 
 struct i2c_client *
--- linux-2.6.26-rc5.orig/include/linux/i2c.h	2008-06-06 20:25:48.000000000 +0200
+++ linux-2.6.26-rc5/include/linux/i2c.h	2008-06-07 09:48:04.000000000 +0200
@@ -148,8 +148,7 @@ struct i2c_driver {
 	const struct i2c_device_id *id_table;
 
 	/* Device detection callback for automatic device creation */
-	int (*detect)(struct i2c_adapter *, int address, int kind,
-		      struct i2c_board_info *);
+	int (*detect)(struct i2c_client *, int kind, struct i2c_board_info *);
 	const struct i2c_client_address_data *address_data;
 	struct list_head clients;
 };


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c