[PATCH 0/4] i2c: Introduce i2c listeners

[PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

Hi all,

This patch set demonstrates a new concept which I would like to add to
the i2c subsystem. It is named "i2c listeners" and is based on the
following structure:

/**
 * struct i2c_listener - Be informed of adapter addition and removal
 * @class: What kind of i2c device the listener may instantiate
 * @attach_adapter: Called after adapter addition (unimplemented)
 * @detach_adapter: Called before adapter removal (unimplemented)
 * @address_data: The I2C addresses to probe, ignore or force
 * @detect: Callback for device detection
 * @clients: List of detected clients we created
 *
 * The @detect callback returns an i2c_board_info structure for device
 * creation if the detection was successful, NULL otherwise.
 *
 * The @clients list is handled by i2c-core.
 */
struct i2c_listener {
	unsigned int class;

	int (*attach_adapter)(struct i2c_adapter *);
	int (*detach_adapter)(struct i2c_adapter *);

	const struct i2c_client_address_data *address_data;
	int (*detect)(struct i2c_adapter *, int address, int kind,
		      struct i2c_board_info *);
	struct list_head clients;

	struct list_head list;
};

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, outside of
the drivers, 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.

There are two distinct parts in i2c_listener: the notification
callbacks, and the device detection callback. The notification
callbacks are exactly the same as what legacy i2c drivers already have.
I did not implement them yet, as I have no immediate need for them, but
I suspect I will have to for the V4L/DVB drivers (if not I'll just
remove them.) The device detection part is implemented in patch 1/4.

The mechanism behind the device detection callback is as follows:
* When a new i2c_notifier 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 detect 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 i2c_notifier, 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-listener list.
* When an i2c_listener is removed, all devices that originate from it
  are destroyed.
* When an i2c_adapter is removed, all devices on it that were created
  by an i2c_listener 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 new-style i2c_driver and an i2c_listener. 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 an i2c_listener 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 ~35 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: Introduce i2c listeners

[Jean Delvare]

Add an i2c listener mechanism to the i2c-subsystem. This mechanism
lets device drivers be notified of addition and removal of adapters,
and gives them a chance to detect devices they would support and to ask
i2c-core to instantiate them.

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

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 thus is no
long-term duplication.

 drivers/i2c/i2c-core.c |  235 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |   34 ++++++
 2 files changed, 269 insertions(+)

--- linux-2.6.26-rc4.orig/include/linux/i2c.h	2008-06-04 08:26:05.000000000 +0200
+++ linux-2.6.26-rc4/include/linux/i2c.h	2008-06-04 08:26:05.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
@@ -92,6 +93,34 @@ extern s32 i2c_smbus_write_i2c_block_dat
 					  u8 command, u8 length,
 					  const u8 *values);
 
+/**
+ * struct i2c_listener - Be informed of adapter addition and removal
+ * @class: What kind of i2c device the listener may instantiate
+ * @attach_adapter: Called after adapter addition (unimplemented)
+ * @detach_adapter: Called before adapter removal (unimplemented)
+ * @address_data: The I2C addresses to probe, ignore or force
+ * @detect: Callback for device detection
+ * @clients: List of detected clients we created
+ *
+ * The @detect callback returns an i2c_board_info structure for device
+ * creation if the detection was successful, NULL otherwise.
+ *
+ * The @clients list is handled by i2c-core.
+ */
+struct i2c_listener {
+	unsigned int class;
+
+	int (*attach_adapter)(struct i2c_adapter *);
+	int (*detach_adapter)(struct i2c_adapter *);
+
+	const struct i2c_client_address_data *address_data;
+	int (*detect)(struct i2c_adapter *, int address, int kind,
+		      struct i2c_board_info *);
+	struct list_head clients;
+
+	struct list_head list;
+};
+
 /*
  * A driver is capable of handling one or more physical devices present on
  * I2C adapters. This information is used to inform the driver of adapter
@@ -155,6 +184,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 i2c_listener.clients
  * @released: used to synchronize client releases & detaches and references
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
@@ -172,6 +202,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)
@@ -382,6 +413,9 @@ extern int i2c_add_adapter(struct i2c_ad
 extern int i2c_del_adapter(struct i2c_adapter *);
 extern int i2c_add_numbered_adapter(struct i2c_adapter *);
 
+extern void i2c_add_listener(struct i2c_listener *);
+extern void i2c_del_listener(struct i2c_listener *);
+
 extern int i2c_register_driver(struct module *, struct i2c_driver *);
 extern void i2c_del_driver(struct i2c_driver *);
 
--- linux-2.6.26-rc4.orig/drivers/i2c/i2c-core.c	2008-06-04 08:26:05.000000000 +0200
+++ linux-2.6.26-rc4/drivers/i2c/i2c-core.c	2008-06-04 11:38:02.000000000 +0200
@@ -42,8 +42,16 @@
 static DEFINE_MUTEX(core_lock);
 static DEFINE_IDR(i2c_adapter_idr);
 
+/* listeners_lock protects the listeners list. If you need both core_lock
+   and listeners_lock, core_lock must be taken first. */
+static DEFINE_MUTEX(listeners_lock);
+static LIST_HEAD(listeners);
+
 #define is_newstyle_driver(d) ((d)->probe || (d)->remove)
 
+static int i2c_detect(struct i2c_adapter *adapter,
+		      struct i2c_listener *listener);
+
 /* ------------------------------------------------------------------------- */
 
 static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
@@ -418,6 +426,7 @@ static int i2c_do_add_adapter(struct dev
 
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
+	struct i2c_listener *listener;
 	int res = 0, dummy;
 
 	mutex_init(&adap->bus_lock);
@@ -448,6 +457,13 @@ static int i2c_register_adapter(struct i
 	if (adap->nr < __i2c_first_dynamic_bus_num)
 		i2c_scan_static_board_info(adap);
 
+	/* let listeners know that a bus has arrived */
+	mutex_lock(&listeners_lock);
+	list_for_each_entry(listener, &listeners, list) {
+		i2c_detect(adap, listener);
+	}
+	mutex_unlock(&listeners_lock);
+
 	/* let legacy drivers scan this bus for matching devices */
 	dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap,
 				 i2c_do_add_adapter);
@@ -576,6 +592,7 @@ static int i2c_do_del_adapter(struct dev
 int i2c_del_adapter(struct i2c_adapter *adap)
 {
 	struct i2c_client *client, *_n;
+	struct i2c_listener *listener;
 	int res = 0;
 
 	mutex_lock(&core_lock);
@@ -588,6 +605,21 @@ int i2c_del_adapter(struct i2c_adapter *
 		goto out_unlock;
 	}
 
+	/* Walk the listeners to remove the devices we created ourselves */
+	mutex_lock(&listeners_lock);
+	list_for_each_entry(listener, &listeners, list) {
+		list_for_each_entry_safe(client, _n, &listener->clients,
+					 detected) {
+			if (client->adapter == adap) {
+				dev_dbg(&adap->dev, "Removing %s at 0x%x\n",
+					client->name, client->addr);
+				list_del(&client->detected);
+				i2c_unregister_device(client);
+			}
+		}
+	}
+	mutex_unlock(&listeners_lock);
+
 	/* Tell drivers about this removal */
 	res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
 			       i2c_do_del_adapter);
@@ -752,6 +784,53 @@ EXPORT_SYMBOL(i2c_del_driver);
 
 /* ------------------------------------------------------------------------- */
 
+static int __i2c_detect(struct device *dev, void *data)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(dev);
+	struct i2c_listener *listener = data;
+
+	i2c_detect(adapter, listener);
+
+	return 0;
+}
+
+void i2c_add_listener(struct i2c_listener *listener)
+{
+	mutex_lock(&core_lock);
+	mutex_lock(&listeners_lock);
+	INIT_LIST_HEAD(&listener->clients);
+	list_add_tail(&listener->list, &listeners);
+	pr_debug("i2c: Listener %p added\n", listener);
+
+	/* let this listener know about buses that are already there */
+	class_for_each_device(&i2c_adapter_class, listener, __i2c_detect);
+	mutex_unlock(&listeners_lock);
+	mutex_unlock(&core_lock);
+}
+EXPORT_SYMBOL_GPL(i2c_add_listener);
+
+void i2c_del_listener(struct i2c_listener *listener)
+{
+	struct i2c_client *client, *_n;
+
+	mutex_lock(&listeners_lock);
+	/* remove all clients that originate from this listener */
+	list_for_each_entry_safe(client, _n, &listener->clients, detected) {
+		dev_dbg(&client->adapter->dev, "Removing %s at 0x%x\n",
+			client->name, client->addr);
+		list_del(&client->detected);
+		i2c_unregister_device(client);
+	}
+
+	list_del(&listener->list);
+	pr_debug("i2c: Listener %p deleted\n", listener);
+	mutex_unlock(&listeners_lock);
+
+}
+EXPORT_SYMBOL_GPL(i2c_del_listener);
+
+/* ------------------------------------------------------------------------- */
+
 static int __i2c_check_addr(struct device *dev, void *addrp)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -1196,6 +1275,162 @@ int i2c_probe(struct i2c_adapter *adapte
 }
 EXPORT_SYMBOL(i2c_probe);
 
+/* Separate detection function for listeners */
+static int i2c_detect_address(struct i2c_adapter *adapter,
+			      int addr, int kind,
+			      struct i2c_listener *listener)
+{
+	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 = listener->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, &listener->clients);
+	}
+	return 0;
+}
+
+static int i2c_detect(struct i2c_adapter *adapter,
+		      struct i2c_listener *listener)
+{
+	const struct i2c_client_address_data *address_data;
+	int i, err;
+	int adap_id = i2c_adapter_id(adapter);
+
+	address_data = listener->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, listener);
+					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, listener);
+			if (err)
+				return err;
+		}
+	}
+
+	/* Stop here if the classes do not match */
+	if (!(adapter->class & listener->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, listener);
+		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]

Convert the lm90 hardware monitoring driver to a new-style i2c driver,
including an i2c_listener for automatic device detection and creation.

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

--- linux-2.6.26-rc4.orig/drivers/hwmon/lm90.c	2008-06-04 16:20:40.000000000 +0200
+++ linux-2.6.26-rc4/drivers/hwmon/lm90.c	2008-06-04 16:20:53.000000000 +0200
@@ -187,23 +187,43 @@ 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 = {
 	.driver = {
 		.name	= "lm90",
 	},
-	.attach_adapter	= lm90_attach_adapter,
-	.detach_client	= lm90_detach_client,
+	.probe		= lm90_probe,
+	.remove		= lm90_remove,
+	.id_table	= lm90_id,
+};
+
+static struct i2c_listener lm90_listener = {
+	.class		= I2C_CLASS_HWMON,
+	.address_data	= &addr_data,
+	.detect		= lm90_detect,
 };
 
 /*
@@ -211,7 +231,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 +496,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 +618,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 +629,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 +641,41 @@ static int lm90_detect(struct i2c_adapte
 	} else if (kind == adt7461) {
 		name = "adt7461";
 	}
+	strlcpy(info->type, name, I2C_NAME_SIZE);
 
-	/* We can fill in the remaining client fields */
-	strlcpy(new_client->name, name, I2C_NAME_SIZE);
-	data->valid = 0;
-	data->kind = kind;
+exit_free:
+	kfree(new_client);
+exit:
+	return err;
+}
+
+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 +698,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 +734,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 +745,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;
 }
@@ -794,12 +814,14 @@ static struct lm90_data *lm90_update_dev
 
 static int __init sensors_lm90_init(void)
 {
+	i2c_add_listener(&lm90_listener);
 	return i2c_add_driver(&lm90_driver);
 }
 
 static void __exit sensors_lm90_exit(void)
 {
 	i2c_del_driver(&lm90_driver);
+	i2c_del_listener(&lm90_listener);
 }
 
 MODULE_AUTHOR("Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>");

-- 
Jean Delvare

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

[PATCH 3/4] i2c: Use i2c_listener in driver f75375s

[Jean Delvare]

Replace the legacy f75375s i2c_driver by an i2c_listener.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
Riku, any chance you could test this one? Thanks.

 drivers/hwmon/f75375s.c |   67 +++++++++--------------------------------------
 1 file changed, 14 insertions(+), 53 deletions(-)

--- linux-2.6.26-rc4.orig/drivers/hwmon/f75375s.c	2008-05-04 09:49:47.000000000 +0200
+++ linux-2.6.26-rc4/drivers/hwmon/f75375s.c	2008-06-04 11:32:22.000000000 +0200
@@ -114,19 +114,16 @@ 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 struct i2c_listener f75375_listener = {
+	.class		= I2C_CLASS_HWMON,
+	.address_data	= &addr_data,
+	.detect		= f75375_detect,
 };
 
 static const struct i2c_device_id f75375_id[] = {
@@ -607,22 +604,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 +681,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 +696,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 +712,7 @@ static int f75375_detect(struct i2c_adap
 			goto exit_free;
 		}
 	}
+	err = 0;	/* detection OK */
 
 	if (kind == f75375) {
 		name = "f75375";
@@ -746,20 +720,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);
-
-	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;
+	strlcpy(info->type, name, I2C_NAME_SIZE);
+	info->addr = address;
 
-	return 0;
-
-exit_detach:
-	i2c_detach_client(client);
 exit_free:
 	kfree(client);
 exit:
@@ -773,16 +736,14 @@ static int __init sensors_f75375_init(vo
 	if (status)
 		return status;
 
-	status = i2c_add_driver(&f75375_legacy_driver);
-	if (status)
-		i2c_del_driver(&f75375_driver);
+	i2c_add_listener(&f75375_listener);
 
 	return status;
 }
 
 static void __exit sensors_f75375_exit(void)
 {
-	i2c_del_driver(&f75375_legacy_driver);
+	i2c_del_listener(&f75375_listener);
 	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: Use i2c_listener in driver f75375s

[Riku Voipio]

Jean Delvare wrote:
> Replace the legacy f75375s i2c_driver by an i2c_listener.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
> Riku, any chance you could test this one? Thanks.
>   
I can test that the new style driver still works after these tests,
but I don't have testing if the driver still works  in
cases were f75375_legacy driver used to work (which I guess is what
you are interested in) is tricker. I do however like the concept.
>  drivers/hwmon/f75375s.c |   67 +++++++++--------------------------------------
>  1 file changed, 14 insertions(+), 53 deletions(-)
>
> --- linux-2.6.26-rc4.orig/drivers/hwmon/f75375s.c	2008-05-04 09:49:47.000000000 +0200
> +++ linux-2.6.26-rc4/drivers/hwmon/f75375s.c	2008-06-04 11:32:22.000000000 +0200
> @@ -114,19 +114,16 @@ 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 struct i2c_listener f75375_listener = {
> +	.class		= I2C_CLASS_HWMON,
> +	.address_data	= &addr_data,
> +	.detect		= f75375_detect,
>  };
>  
>  static const struct i2c_device_id f75375_id[] = {
> @@ -607,22 +604,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 +681,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 +696,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 +712,7 @@ static int f75375_detect(struct i2c_adap
>  			goto exit_free;
>  		}
>  	}
> +	err = 0;	/* detection OK */
>  
>  	if (kind == f75375) {
>  		name = "f75375";
> @@ -746,20 +720,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);
> -
> -	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;
> +	strlcpy(info->type, name, I2C_NAME_SIZE);
> +	info->addr = address;
>  
> -	return 0;
> -
> -exit_detach:
> -	i2c_detach_client(client);
>  exit_free:
>  	kfree(client);
>  exit:
> @@ -773,16 +736,14 @@ static int __init sensors_f75375_init(vo
>  	if (status)
>  		return status;
>  
> -	status = i2c_add_driver(&f75375_legacy_driver);
> -	if (status)
> -		i2c_del_driver(&f75375_driver);
> +	i2c_add_listener(&f75375_listener);
>  
>  	return status;
>  }
>  
>  static void __exit sensors_f75375_exit(void)
>  {
> -	i2c_del_driver(&f75375_legacy_driver);
> +	i2c_del_listener(&f75375_listener);
>  	i2c_del_driver(&f75375_driver);
>  }
>  
>
>   


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

Re: [PATCH 3/4] i2c: Use i2c_listener in driver f75375s

[Jean Delvare]

On Thu, 05 Jun 2008 11:33:38 +0300, Riku Voipio wrote:
> Jean Delvare wrote:
> > Replace the legacy f75375s i2c_driver by an i2c_listener.
> >
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > ---
> > Riku, any chance you could test this one? Thanks.
> >   
> I can test that the new style driver still works after these tests,
> but I don't have testing if the driver still works  in
> cases were f75375_legacy driver used to work (which I guess is what
> you are interested in) is tricker. I do however like the concept.

I have already tested the latter, using i2c-stub + a dump of a F75375S
chip. So if you can test the former (new-style) we cover the whole set
of possibilities and that's alright :)

Thanks,
-- 
Jean Delvare

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

Re: [PATCH 3/4] i2c: Use i2c_listener in driver f75375s

[Riku Voipio]

Jean Delvare wrote:
> I have already tested the latter, using i2c-stub + a dump of a F75375S
> chip. So if you can test the former (new-style) we cover the whole set
> of possibilities and that's alright :)
>
> Thanks,
>   
I failed to get the patchset against Linux 2.6.26-rc5, and didn't have time
to fight:

 > patch -p1 --dry-run < 
../p2/\[PATCH_1_4\]_i2c__Introduce_i2c_listeners.txt
patching file include/linux/i2c.h
Hunk #2 succeeded at 94 (offset 1 line).
Hunk #3 succeeded at 185 (offset 1 line).
Hunk #4 succeeded at 203 (offset 1 line).
patching file drivers/i2c/i2c-core.c
Hunk #1 succeeded at 43 (offset 1 line).
Hunk #2 succeeded at 432 (offset 6 lines).
Hunk #3 succeeded at 463 (offset 6 lines).
Hunk #4 FAILED at 598.
Hunk #5 succeeded at 612 (offset 7 lines).
Hunk #6 succeeded at 796 (offset 12 lines).
Hunk #7 succeeded at 1240 (offset -35 lines).
1 out of 7 hunks FAILED -- saving rejects to file drivers/i2c/i2c-core.c.rej

However, none of the functions you touched are used in F75375S driver
with new-style loading path, so it would seem fine for me.


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

Re: [PATCH 3/4] i2c: Use i2c_listener in driver f75375s

[Jean Delvare]

Hi Riku,

On Fri, 06 Jun 2008 13:21:23 +0300, Riku Voipio wrote:
> Jean Delvare wrote:
> > I have already tested the latter, using i2c-stub + a dump of a F75375S
> > chip. So if you can test the former (new-style) we cover the whole set
> > of possibilities and that's alright :)
> >
> > Thanks,
> >   
> I failed to get the patchset against Linux 2.6.26-rc5, and didn't have time
> to fight:
> 
>  > patch -p1 --dry-run < 
> ../p2/\[PATCH_1_4\]_i2c__Introduce_i2c_listeners.txt
> patching file include/linux/i2c.h
> Hunk #2 succeeded at 94 (offset 1 line).
> Hunk #3 succeeded at 185 (offset 1 line).
> Hunk #4 succeeded at 203 (offset 1 line).
> patching file drivers/i2c/i2c-core.c
> Hunk #1 succeeded at 43 (offset 1 line).
> Hunk #2 succeeded at 432 (offset 6 lines).
> Hunk #3 succeeded at 463 (offset 6 lines).
> Hunk #4 FAILED at 598.
> Hunk #5 succeeded at 612 (offset 7 lines).
> Hunk #6 succeeded at 796 (offset 12 lines).
> Hunk #7 succeeded at 1240 (offset -35 lines).
> 1 out of 7 hunks FAILED -- saving rejects to file drivers/i2c/i2c-core.c.rej

>From the patch header:

This patch depends on:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-use-class_for_each_device.patch

Did you apply that patch first?

> However, none of the functions you touched are used in F75375S driver
> with new-style loading path, so it would seem fine for me.

The part that needs testing is in i2c-core, not in the f75375s driver
itself.

That being said, I am about to post a second version of the patch set,
you probably want to wait for it before you do your tests.

-- 
Jean Delvare

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

[PATCH 4/4] i2c: Use i2c_listener in driver lm75

[Jean Delvare]

Replace the legacy lm75 i2c_driver by an i2c_listener.

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/

David, any chance you could test this one? Thanks.

 drivers/hwmon/lm75.c |   64 ++++++++++----------------------------------------
 1 file changed, 14 insertions(+), 50 deletions(-)

--- linux-2.6.26-rc4.orig/drivers/hwmon/lm75.c	2008-06-04 11:44:57.000000000 +0200
+++ linux-2.6.26-rc4/drivers/hwmon/lm75.c	2008-06-04 11:49:24.000000000 +0200
@@ -54,11 +54,11 @@ enum lm75_type {		/* keep sorted in alph
 	tmp75,
 };
 
-/* Addresses scanned by legacy style driver binding */
+/* Addresses scanned by listener */
 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 (for listener) */
 I2C_CLIENT_INSMOD_1(lm75);
 
 
@@ -247,16 +247,12 @@ static struct i2c_driver lm75_driver = {
 
 /*-----------------------------------------------------------------------*/
 
-/* "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 +269,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 +307,24 @@ 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 struct i2c_driver lm75_legacy_driver = {
-	.driver = {
-		.name	= "lm75_legacy",
-	},
-	.attach_adapter	= lm75_attach_adapter,
-	.detach_client	= lm75_detach_client,
+static struct i2c_listener lm75_listener = {
+	.class		= I2C_CLASS_HWMON,
+	.address_data	= &addr_data,
+	.detect		= lm75_detect,
 };
 
 /*-----------------------------------------------------------------------*/
@@ -430,16 +396,14 @@ static int __init sensors_lm75_init(void
 	if (status < 0)
 		return status;
 
-	status = i2c_add_driver(&lm75_legacy_driver);
-	if (status < 0)
-		i2c_del_driver(&lm75_driver);
+	i2c_add_listener(&lm75_listener);
 
 	return status;
 }
 
 static void __exit sensors_lm75_exit(void)
 {
-	i2c_del_driver(&lm75_legacy_driver);
+	i2c_del_listener(&lm75_listener);
 	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: Introduce i2c listeners

[Jon Smirl]

On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi all,
>
>  This patch set demonstrates a new concept which I would like to add to
>  the i2c subsystem. It is named "i2c listeners" and is based on the
>  following structure:

How does this fit in with the existing driver bus structure in kernel?
This sounds a lot like hot plug and buses already implement hot plug
events.  This isn't exactly hot plug but it may be close enough that
the existing bus APIs will work. Note that both devices and entire
buses can be hot plugged with existing code.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/4/08, Jon Smirl <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>  > Hi all,
>  >
>  >  This patch set demonstrates a new concept which I would like to add to
>  >  the i2c subsystem. It is named "i2c listeners" and is based on the
>  >  following structure:

Could the existing API be used something like this?

Drivers register with i2c core using standard driver registration.
Class and address ranges are exposed in a structure like PCI IDs. I2C
can make up it's own ID structure equivalent to the PCI IDs one.

When a new adapter/bus is created i2c core gets the event. Core then
scans the registered driver structures looking for class matches. When
a match is encountered it repeatedly calls the driver's probe function
with the addresses from the range. Driver returns true/false on each
probe which triggers creation of the device.

Why do drivers need the bus creation (attach/detach adapter) events?

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

On Wed, 4 Jun 2008 15:28:54 -0400, Jon Smirl wrote:
> On 6/4/08, Jon Smirl <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >  > Hi all,
> >  >
> >  >  This patch set demonstrates a new concept which I would like to add to
> >  >  the i2c subsystem. It is named "i2c listeners" and is based on the
> >  >  following structure:
> 
> Could the existing API be used something like this?
> 
> Drivers register with i2c core using standard driver registration.
> Class and address ranges are exposed in a structure like PCI IDs. I2C
> can make up it's own ID structure equivalent to the PCI IDs one.

No, no, no. I've been repeating it many times already: I2C device
addresses are NOT IDs. If you're even thinking of going in this
direction, you're plain wrong, sorry. Many devices share the same
address, and many devices can use different addresses. The address is
really only one attribute of an I2C device, amongst many others. It's
not an identifier.

Please also realize that we _do_ have "IDs" for I2C devices by now. As
there are no standard numbers as PCI has, we use arbitrary strings
(typically the device name). That's what new-style drivers use for
device binding since kernel 2.6.26. And you can't have more than one ID
format for a given subsystem, and we don't want to break new-style
drivers, so thinking about a radically different form of IDs for the
i2c subsystem is a waste of time: it's simply not going to happen.

> When a new adapter/bus is created i2c core gets the event. Core then

Side question: why would i2c-core want events when adapters are
created, given that it does create them itself?

> scans the registered driver structures looking for class matches. When
> a match is encountered it repeatedly calls the driver's probe function
> with the addresses from the range. Driver returns true/false on each
> probe which triggers creation of the device.

The driver's probe function is, by definition of the device driver
model, called on an existing device. So the probe function cannot
decide whether the device should be created or not - it's too late at
this point.

One thing that we could consider though is merging i2c_listener into
i2c_driver. For some reason I had not considered it - but I somehow
expect funny locking issues if we do that.

> Why do drivers need the bus creation (attach/detach adapter) events?

Originally, to probe the adapters in search of devices to support. I
seem to remember that some V4L/DVB drivers do more than that in their
attach_adapter callback, but I need to take a closer look. And i2c-dev
deserves particular attention, as it needs to do bookkeeping of all
the adapters that are present. In fact we might have to implement
attach/detach adapter just for i2c-dev. More on that after I have read
the code again.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, 4 Jun 2008 15:28:54 -0400, Jon Smirl wrote:
>  > On 6/4/08, Jon Smirl <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>  > > On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>  > >  > Hi all,
>  > >  >
>  > >  >  This patch set demonstrates a new concept which I would like to add to
>  > >  >  the i2c subsystem. It is named "i2c listeners" and is based on the
>  > >  >  following structure:
>  >
>  > Could the existing API be used something like this?
>  >
>  > Drivers register with i2c core using standard driver registration.
>  > Class and address ranges are exposed in a structure like PCI IDs. I2C
>  > can make up it's own ID structure equivalent to the PCI IDs one.
>
>
> No, no, no. I've been repeating it many times already: I2C device
>  addresses are NOT IDs. If you're even thinking of going in this
>  direction, you're plain wrong, sorry. Many devices share the same
>  address, and many devices can use different addresses. The address is
>  really only one attribute of an I2C device, amongst many others. It's
>  not an identifier.

I know these aren't identifiers, I'm proposing that they be ranges of
address to scan. The driver model doesn't force these to be IDs, but
in most cases they are IDs. To the driver model it's just a pointer to
a struct, no interpretation is done of the structure. It's the PCI bus
driver that interprets these structs as identifiers. In our case the
bus driver is inside i2c core and it interprets the structs as address
ranges and then calls the probe function sequentially with the
different value.

I shouldn't have compared this to PCI IDs, I was just thinking of it
as data that gets processed by i2c core. PCI IDs are an example of
data that is processed by the PCI core. Their meaning as ids is
supplied by PCI core, not the kernel driver framework.

>  Please also realize that we _do_ have "IDs" for I2C devices by now. As
>  there are no standard numbers as PCI has, we use arbitrary strings
>  (typically the device name). That's what new-style drivers use for
>  device binding since kernel 2.6.26. And you can't have more than one ID
>  format for a given subsystem, and we don't want to break new-style
>  drivers, so thinking about a radically different form of IDs for the
>  i2c subsystem is a waste of time: it's simply not going to happen.
>
>
>  > When a new adapter/bus is created i2c core gets the event. Core then
>
>
> Side question: why would i2c-core want events when adapters are
>  created, given that it does create them itself?

I used 'event' as a generic term for meaning the driver core was aware
that the adapter was created.

>  > scans the registered driver structures looking for class matches. When
>  > a match is encountered it repeatedly calls the driver's probe function
>  > with the addresses from the range. Driver returns true/false on each
>  > probe which triggers creation of the device.
>
>
> The driver's probe function is, by definition of the device driver
>  model, called on an existing device. So the probe function cannot
>  decide whether the device should be created or not - it's too late at
>  this point.

I don't believe this is has to be true although usually it is. A
driver is passed in information from the bus core on the probe
function. It then looks at that information and decides if it wants to
bind. Drivers return true/false if they want to bind. If they return
false the device structure is destroyed (or reused for the next probe
call).

There is nothing in the model that requires an actual device to be
present for a probe call to happen. A perfectly valid reason for
refusing to bind would be not having hardware present at the address.
This is how older ISA drivers are loaded that predate PNP. They search
for the hardware in the probe function. That's probably why it is
called "probe" but that predates my involvement in Linux.

>  One thing that we could consider though is merging i2c_listener into
>  i2c_driver. For some reason I had not considered it - but I somehow
>  expect funny locking issues if we do that.

We are not far apart on this proposal if you combine the
class/address_data fields into i2c_driver and then use the existing
probe function instead of detect(). The i2c_client struct already has
an address field. A driver would then check to see if there was
hardware at the i2c_client.address and return false if it is not
there. i2c core would just reuse the i2c_client struct with the next
address and do another probe.

Operations on an adapter need to be serialized but I thought the
adapters already supported that. If they don't they need to be fixed
anyway because of the BKL removal.

>
>
>  > Why do drivers need the bus creation (attach/detach adapter) events?
>
>
> Originally, to probe the adapters in search of devices to support. I
>  seem to remember that some V4L/DVB drivers do more than that in their
>  attach_adapter callback, but I need to take a closer look. And i2c-dev
>  deserves particular attention, as it needs to do bookkeeping of all
>  the adapters that are present. In fact we might have to implement
>  attach/detach adapter just for i2c-dev. More on that after I have read
>  the code again.

I don't think believe devices should need attach/detach adapter
functions. If they do, there is probably some other way to achieve the
same effect without needing the functions. i2c-dev may need access to
some i2c core structures to avoid the need for these calls.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Wednesday 04 June 2008, Jon Smirl wrote:
> > The driver's probe function is, by definition of the device driver
> > model, called on an existing device. So the probe function cannot
> > decide whether the device should be created or not - it's too late at
> > this point.
> 
> I don't believe this is has to be true although usually it is. A

For driver model conformant subsystems, it's always true.


> driver is passed in information from the bus core on the probe
> function.

That is, a bus-specific struct wrapping a "struct device" instance.


> It then looks at that information and decides if it wants to 
> bind. Drivers return true/false if they want to bind.

Wrong.  The probe returns zero if it bound, else negative errno.
Those functions don't return booleans.


> If they return 
> false the device structure is destroyed (or reused for the next probe
> call).

No.  The struct device is not destroyed at that time for ANY
driver model code I've seen in Linux.  Destroying it would
prevent driver modules from binding to that device when they
get loaded later.

 
> There is nothing in the model that requires an actual device to be
> present for a probe call to happen.

Well, a "struct device" must be present, but there might not
be hardware backing it.  Each particular bus infrastructure
instantiates those structs in bus-specific ways.


> A perfectly valid reason for 
> refusing to bind would be not having hardware present at the address.

True.


> This is how older ISA drivers are loaded that predate PNP.

Depends somewhat on what flavor of PNP you mean too.


> They search 
> for the hardware in the probe function. That's probably why it is
> called "probe" but that predates my involvement in Linux.

And that legacy model is something that's getting removed
everywhere practical.  Such "legacy drivers" don't support
hotplugging or most of the other mechanisms defined by the
driver model, since they have the driver creating the
device structs (instead of bus infrastructure).

You're right that this is why the name probe() remains.
I prefer the bind()/unbind() names too.

- Dave

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/4/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>  > It then looks at that information and decides if it wants to
>  > bind. Drivers return true/false if they want to bind.
>
>
> Wrong.  The probe returns zero if it bound, else negative errno.
>  Those functions don't return booleans.

You guys are so literal. I should have said return 0 or -ENODEV or
some other error.

>  > If they return
>  > false the device structure is destroyed (or reused for the next probe
>  > call).
>
>
> No.  The struct device is not destroyed at that time for ANY
>  driver model code I've seen in Linux.  Destroying it would
>  prevent driver modules from binding to that device when they
>  get loaded later.

Don't they get destroyed when hotplug devices are removed from the
system? Could this be considered a hotplug, create struct device,
probe to see if it is claimed, unhotplug if not claimed, destroy
struct device.

But I agree with what you meant, that they never get destroyed when
there is known hardware backing them in the system.

>  > They search
>  > for the hardware in the probe function. That's probably why it is
>  > called "probe" but that predates my involvement in Linux.
>
>
> And that legacy model is something that's getting removed
>  everywhere practical.  Such "legacy drivers" don't support
>  hotplugging or most of the other mechanisms defined by the
>  driver model, since they have the driver creating the
>  device structs (instead of bus infrastructure).

Those drivers are getting removed because the hardware is disappearing
and we have ACPI/PNP now to tell if if is present, not because they
misused the driver code. We just don't need the old style probing code
anymore and it was unreliable.

There is is a parallel here. With PowerPC device trees I can put the
exact address of the i2c device into the board specific device tree.
Now there's no need to probe addresses. That's equivalent to what's
happening with ACPI and serial ports. Maybe we'll implement device
trees on all archs and then we can get rid of i2c device probing.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Wednesday 04 June 2008, Jon Smirl wrote:
> On 6/4/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> >  > It then looks at that information and decides if it wants to
> >  > bind. Drivers return true/false if they want to bind.
> >
> >
> > Wrong.  The probe returns zero if it bound, else negative errno.
> >  Those functions don't return booleans.
> 
> You guys are so literal. I should have said return 0 or -ENODEV or
> some other error.

You seemed intent on describing some new and never-seen-before
model for how driver model probing works, though ...


> >  > If they return
> >  > false the device structure is destroyed (or reused for the next probe
> >  > call).
> >
> >
> > No.  The struct device is not destroyed at that time for ANY
> >  driver model code I've seen in Linux.  Destroying it would
> >  prevent driver modules from binding to that device when they
> >  get loaded later.
> 
> Don't they get destroyed when hotplug devices are removed from the
> system?

That's not "at that time".  That's "when the hardware is
removed" ... for busses which support such detection.
USB supports it.  PCI itself doesn't (like I2C, SPI, etc).

CardBus, Hotplug PCI, and hotplug PCIE do report removals,
so they can remove such device structs later.  But they
would never do it as part of enumeration.


> Could this be considered a hotplug, create struct device, 
> probe to see if it is claimed, unhotplug if not claimed, destroy
> struct device.

No.  Think more about what goes into hotplugging.  It's
not just "some device is at that address".  It's also
"device with such-and-such-identifiers is there".


> But I agree with what you meant, that they never get destroyed when
> there is known hardware backing them in the system.
> 
> >  > They search
> >  > for the hardware in the probe function. That's probably why it is
> >  > called "probe" but that predates my involvement in Linux.
> >
> >
> > And that legacy model is something that's getting removed
> >  everywhere practical.  Such "legacy drivers" don't support
> >  hotplugging or most of the other mechanisms defined by the
> >  driver model, since they have the driver creating the
> >  device structs (instead of bus infrastructure).
> 
> Those drivers are getting removed because the hardware is disappearing
> and we have ACPI/PNP now to tell if if is present, not because they
> misused the driver code. We just don't need the old style probing code
> anymore and it was unreliable.

I've seen folk re-inventing such probing for custom hardware,
FWIW.  (Such stuff doesn't get merged upstream very often.)


> There is is a parallel here. With PowerPC device trees I can put the
> exact address of the i2c device into the board specific device tree.
> Now there's no need to probe addresses. That's equivalent to what's
> happening with ACPI and serial ports. Maybe we'll implement device
> trees on all archs and then we can get rid of i2c device probing.

There are surely worse futures to envision!

ACPI would need to expose I2C/SMBus though.  I suspect that's
something many current x86 platform vendors would oppose.

- Dave


> 
> -- 
> Jon Smirl
> jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> 



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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/4/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Wednesday 04 June 2008, Jon Smirl wrote:
>
> > On 6/4/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>  > >  > It then looks at that information and decides if it wants to
>  > >  > bind. Drivers return true/false if they want to bind.
>  > >
>  > >
>  > > Wrong.  The probe returns zero if it bound, else negative errno.
>  > >  Those functions don't return booleans.
>  >
>  > You guys are so literal. I should have said return 0 or -ENODEV or
>  > some other error.
>
>
> You seemed intent on describing some new and never-seen-before
>  model for how driver model probing works, though ...

I don't think it is out of the scope of what already exists. The
alternative is to add a new pre-probe() API which does the function of
the original probe and then calls the current probe(). Why don't we
just stick with probe() and use it like it was before we had things
like ACPI?

Now that we've had this discussion it looks to me that the scan
capability should be added to struct i2c_driver instead of being
standalone.

I guess what is a little strange to me is that the pre-probe()
function is like a static driver function and probe() is an instance
function. Are there other examples of static driver  functions? Which
way you do it is a style question, they will both work.

Doesn't something like this work? (obviously this isn't complete)

for (address = start; address <= stop;) {
	make a new client
	for (; address <=stop; address++) {
		client->address = address;
		res = device_register(&client->dev);
		if (res != -ENODEV)
			break;
	}
}
delete the extra client


Can these driver functions be eliminated?
	int (*attach_adapter)(struct i2c_adapter *);
	int (*detach_adapter)(struct i2c_adapter *);
	int (*detach_client)(struct i2c_client *);

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Wednesday 04 June 2008, Jon Smirl wrote:
> Now that we've had this discussion it looks to me that the scan
> capability should be added to struct i2c_driver instead of being
> standalone.
> 
> I guess what is a little strange to me is that the pre-probe()
> function is like a static driver function and probe() is an instance
> function.

The problem there is:  how do you know which pre-probe()
function(s) to try?  If you could answer that reliably
you'd be able to probe() directly.  If you can't, you'd
need to load all I2C driver modules until one works...
not the desired system model.


> Are there other examples of static driver  functions? 

Module load and unload code; platform_driver_probe() type stuff.

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/5/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Wednesday 04 June 2008, Jon Smirl wrote:
>
> > Now that we've had this discussion it looks to me that the scan
>  > capability should be added to struct i2c_driver instead of being
>  > standalone.
>  >
>  > I guess what is a little strange to me is that the pre-probe()
>  > function is like a static driver function and probe() is an instance
>  > function.
>
>
> The problem there is:  how do you know which pre-probe()
>  function(s) to try?  If you could answer that reliably
>  you'd be able to probe() directly.  If you can't, you'd
>  need to load all I2C driver modules until one works...
>  not the desired system model.
>

I see now that the adapter class should be added to the driver name
alias string. The we could load all modules of a given class.

But I don't think we need to go searching for the right module to load
since i2c is not generally a dynamic bus. If you install a video card
with an i2c bus, the driver for the video card can just load_module
the right i2c driver modules.

I do wonder if we could get hardware monitors to autoload. When the
adapter driver for the motherboard loads, it could load module all
drivers of class hardware_monitor. Then each one could do its
detection thing. Unload the ones that weren't found. That would really
help people who can figure out the right driver module to load. You
could even get fancy and generate a uevent after the right module was
found. A script in the uevent could save the info so you don't have to
repeat on each boot. Of course this assume that the hardware monitor
modules can reliably autodetect.


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

On Thu, 5 Jun 2008 10:55:28 -0400, Jon Smirl wrote:
> I see now that the adapter class should be added to the driver name
> alias string. The we could load all modules of a given class.

Ah ah. Oh wait, you were serious? Come on, why would anyone on earth
want to load all modules of a given class?

> But I don't think we need to go searching for the right module to load
> since i2c is not generally a dynamic bus. If you install a video card
> with an i2c bus, the driver for the video card can just load_module
> the right i2c driver modules.

Sometimes yes, sometimes no. For most V4L/DVB, the main driver should
know which i2c chip drivers it needs, but unfortunately for old drivers
such as bttv this knowledge has never been recorded, so there is no
alternative to (at least partial) probing in practice. For video
adapters, they could load the eeprom driver as they all give access to
EDID EEPROMs, but if the card also has a hardware monitoring chip, we
simply don't know what it is. We just can't record in the kernel
drivers what hardware monitoring chip exists on thousands of cards out
there (same problem as for motherboards.)

> I do wonder if we could get hardware monitors to autoload.

Only if they don't do the device detection/identification themselves.
I've considered it for a moment but now I'm fairly certain that it
would be a dangerous and unmaintainable mess. Better let each driver
include its detection/identification routine, and delegate the
responsibility of loading the right driver to user-space (as is already
the case.)

> When the
> adapter driver for the motherboard loads, it could load module all
> drivers of class hardware_monitor.

No, really, forget about this, that's simply never going to happen.
That's way too costly and dangerous.

> Then each one could do its
> detection thing. Unload the ones that weren't found.

And do it all over again when the graphics adapter driver is loaded?
And when an parport-to-I2C or USB-to-I2C adapter is plugged?

> That would really
> help people who can figure out the right driver module to load.

If people can't run "sensors-detect" there's not much we can do for
them ;)

> You
> could even get fancy and generate a uevent after the right module was
> found. A script in the uevent could save the info so you don't have to
> repeat on each boot. Of course this assume that the hardware monitor
> modules can reliably autodetect.

You mean, instead of just using the sensors-detect script which we
already have and has worked fine for (almost) everyone for 8 years now?
And the few cases where sensors-detect didn't work are a very good
reason to _not_ attempt to do the same in the kernel. SMBus probing is
_very_ tricky and we really want to control everything that happens.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/5/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Thu, 5 Jun 2008 10:55:28 -0400, Jon Smirl wrote:
>  > I see now that the adapter class should be added to the driver name
>  > alias string. The we could load all modules of a given class.
>
>
> Ah ah. Oh wait, you were serious? Come on, why would anyone on earth
>  want to load all modules of a given class?
>
>
>  > But I don't think we need to go searching for the right module to load
>  > since i2c is not generally a dynamic bus. If you install a video card
>  > with an i2c bus, the driver for the video card can just load_module
>  > the right i2c driver modules.
>
>
> Sometimes yes, sometimes no. For most V4L/DVB, the main driver should
>  know which i2c chip drivers it needs, but unfortunately for old drivers
>  such as bttv this knowledge has never been recorded, so there is no
>  alternative to (at least partial) probing in practice. For video
>  adapters, they could load the eeprom driver as they all give access to
>  EDID EEPROMs, but if the card also has a hardware monitoring chip, we
>  simply don't know what it is. We just can't record in the kernel
>  drivers what hardware monitoring chip exists on thousands of cards out
>  there (same problem as for motherboards.)
>
>
>  > I do wonder if we could get hardware monitors to autoload.
>
>
> Only if they don't do the device detection/identification themselves.
>  I've considered it for a moment but now I'm fairly certain that it
>  would be a dangerous and unmaintainable mess. Better let each driver
>  include its detection/identification routine, and delegate the
>  responsibility of loading the right driver to user-space (as is already
>  the case.)
>
>
>  > When the
>  > adapter driver for the motherboard loads, it could load module all
>  > drivers of class hardware_monitor.
>
>
> No, really, forget about this, that's simply never going to happen.
>  That's way too costly and dangerous.
>
>
>  > Then each one could do its
>  > detection thing. Unload the ones that weren't found.
>
>
> And do it all over again when the graphics adapter driver is loaded?
>  And when an parport-to-I2C or USB-to-I2C adapter is plugged?
>
>
>  > That would really
>  > help people who can figure out the right driver module to load.
>
>
> If people can't run "sensors-detect" there's not much we can do for
>  them ;)

What about live CDs and things like that? I do agree that there is no
immediate need to do something like this. But in the long run a scheme
like this could eliminate the need for sensors-detect.

It doesn't take as long to load/unload the modules as you might think,
they would all be on an initrd. The probing time should be the same as
the sensors-detect script. Maintenance would be better since the
detection code would only exist in one place instead of two.

File this away to think about for the future. After the drivers are
converted to the new model and have the ability to detect this can be
experimented with.


>
>
>  > You
>  > could even get fancy and generate a uevent after the right module was
>  > found. A script in the uevent could save the info so you don't have to
>  > repeat on each boot. Of course this assume that the hardware monitor
>  > modules can reliably autodetect.
>
>
> You mean, instead of just using the sensors-detect script which we
>  already have and has worked fine for (almost) everyone for 8 years now?
>  And the few cases where sensors-detect didn't work are a very good
>  reason to _not_ attempt to do the same in the kernel. SMBus probing is
>  _very_ tricky and we really want to control everything that happens.
>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

On Thu, 5 Jun 2008 13:34:59 -0400, Jon Smirl wrote:
> What about live CDs and things like that? I do agree that there is no
> immediate need to do something like this. But in the long run a scheme
> like this could eliminate the need for sensors-detect.

Why do you want to eliminate sensors-detect in the first place? If you
really want to load the sensor drivers automatically, just run
sensors-detect at boot time.

> It doesn't take as long to load/unload the modules as you might think,

I just tested, that's 2.3 s on my system, and then 0.85 s to unload the
drivers. So, total 3 s. I certainly don't want my system to take 3 more
seconds to load. And that's the best case, at HZ=1000. At HZ=100 it
will be worse.

Anyway, if you really want to load all the drivers at boot time, this
should be a user-space decision and not a kernel-space decision. It can
already be done in user-space today (but note that no distribution does
this, thankfully), so you're trying to solve a problem that doesn't
really exist.

> they would all be on an initrd. The probing time should be the same as

2.1 MB of hwmon drivers in initrd? To be added to all the i2c bus
drivers... I doubt anybody wants this. These drivers aren't needed to
boot the system so why put them in initrd? Furthermore, I don't see how
we care if the drivers are in initrd or not.

> the sensors-detect script. Maintenance would be better since the
> detection code would only exist in one place instead of two.

Admittedly it would be nice to have only one set of detection code to
maintain. But there are many drawbacks. For example, how do you detect
devices we don't yet support? Or devices that are supported in a later
kernel? Devices for which the user didn't build the driver yet? It is
very easy to point users to the latest version of sensors-detect and ask
them to run it. Compare this to "you have to upgrade to the latest
kernel and build all the drivers first and only then we can tell you
the one you need".

> File this away to think about for the future. After the drivers are
> converted to the new model and have the ability to detect this can be
> experimented with.

You are free to experiment with whatever you want, of course, but my
feeling is that you are going in the wrong direction. If anything, I'd
rather make sensors-detect safer, faster and more controllable (command
line interface...) than trying to move it inside the kernel.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

On Thu, 5 Jun 2008 00:04:00 -0400, Jon Smirl wrote:
> I don't think it is out of the scope of what already exists. The
> alternative is to add a new pre-probe() API which does the function of
> the original probe and then calls the current probe(). Why don't we
> just stick with probe() and use it like it was before we had things
> like ACPI?

You're thinking of probing the wrong way around, methinks. We want to
add something _before_ the current probe function, not after it. Adding
the actual hardware detection/identification after the driver model's
probe() implies that a device has already been created
(otherwise .probe() can't be called). Chicken and egg problem.

> Now that we've had this discussion it looks to me that the scan
> capability should be added to struct i2c_driver instead of being
> standalone.

After a good night of sleep I've reached the same conclusion. Given
that many drivers would have a new-style i2c driver and an i2c
listener, it's probably easier to merge both. I'll give it a try when I
get time, but as stated elsewhere in this thread, I wouldn't be too
surprised to hit a deadlock or two on the road.

> I guess what is a little strange to me is that the pre-probe()
> function is like a static driver function and probe() is an instance
> function. Are there other examples of static driver  functions? Which
> way you do it is a style question, they will both work.

I guess it depends on what you mean with a "static function" and
"instance function" for a non-object language ;) Put it simple terms,
the hardware detection function has to operate on addresses (i.e.
potential devices) while the device driver model's probe function
operates on actual devices. I fully agree with David that this function
should really be named .bind(), not .probe().

> Doesn't something like this work? (obviously this isn't complete)
> 
> for (address = start; address <= stop;) {
> 	make a new client
> 	for (; address <=stop; address++) {
> 		client->address = address;
> 		res = device_register(&client->dev);
> 		if (res != -ENODEV)
> 			break;
> 	}
> }
> delete the extra client

You don't seriously propose to scan all addresses on every I2C bus for
every i2c chip driver that we load, do you? This would be awfully slow
and dangerous. No way.

> Can these driver functions be eliminated?
> 	int (*attach_adapter)(struct i2c_adapter *);
> 	int (*detach_adapter)(struct i2c_adapter *);
> 	int (*detach_client)(struct i2c_client *);

This is exactly my goal with i2c_notifier (or any similar approach -
again I'm not claiming my proposal is the only way to go). Sorry if it
wasn't clear, maybe I should have mentioned it explicitly in patch 0/4.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/5/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>  > Doesn't something like this work? (obviously this isn't complete)
>  >
>  > for (address = start; address <= stop;) {
>  >       make a new client
>  >       for (; address <=stop; address++) {
>  >               client->address = address;
>  >               res = device_register(&client->dev);
>  >               if (res != -ENODEV)
>  >                       break;
>  >       }
>  > }
>  > delete the extra client
>
>
> You don't seriously propose to scan all addresses on every I2C bus for
>  every i2c chip driver that we load, do you? This would be awfully slow
>  and dangerous. No way.

This is just a small part of the code, you need to add in all the
other parts from the patch.
Exactly the same addresses would be scanned  under each scheme. .My
question is, does the driver need two entry points (detect and probe)
or could only one (probe) be used? I think detect and probe can be the
same function.

With the detect() function it would look somthing like this:

for (address = start; address <= stop;) {
      for (; address <=stop; address++) {
             do a generic detect
              if (driver->detect(address) {
                     make a new client
                     client->address = address;
                     res = device_register(&client->dev);
                     if (!res)
                            delete the extra client
             }
       }
}





-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

Hi Jon,

On Wed, 4 Jun 2008 20:42:00 -0400, Jon Smirl wrote:
> On 6/4/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > Wrong.  The probe returns zero if it bound, else negative errno.
> >  Those functions don't return booleans.
> 
> You guys are so literal. I should have said return 0 or -ENODEV or
> some other error.

Oh, David is several persons now? ;)

> (...)
> There is is a parallel here. With PowerPC device trees I can put the
> exact address of the i2c device into the board specific device tree.
> Now there's no need to probe addresses. That's equivalent to what's
> happening with ACPI and serial ports. Maybe we'll implement device
> trees on all archs and then we can get rid of i2c device probing.

It's the manufacturer who decide, not us. And at the moment, the PC
architecture isn't using device trees for I2C/SMBus. That's an
unfortunate fact we have to live with.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

Hi Jon,

On Wed, 4 Jun 2008 19:11:11 -0400, Jon Smirl wrote:
> On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >  One thing that we could consider though is merging i2c_listener into
> >  i2c_driver. For some reason I had not considered it - but I somehow
> >  expect funny locking issues if we do that.
> 
> We are not far apart on this proposal if you combine the
> class/address_data fields into i2c_driver 

Yes.

> and then use the existing probe function instead of detect().

No. Again, detect() creates the very device that probe() will be called
on, so merging detect() into probe() is, say, like dividing by 0.

(OK, not my most direct metaphor of the day - oh well ;))

> The i2c_client struct already has
> an address field. A driver would then check to see if there was
> hardware at the i2c_client.address and return false if it is not
> there. i2c core would just reuse the i2c_client struct with the next
> address and do another probe.

Out of curiosity, what would be the name of the device backing that
i2c_client? The device's name is what the driver core uses to know
which driver's probe() method needs to be called.

Maybe I'm just not seeing what you want to do. I'll be happy to see a
(tested) patch implementing your idea though. If you can come up with
something that works and has advantages over what I proposed, I'll
certainly consider it.

> Operations on an adapter need to be serialized but I thought the
> adapters already supported that. If they don't they need to be fixed
> anyway because of the BKL removal.

Yes, the operations on each adapter are already serialized, no problem
with this.

> (...)
> I don't think believe devices should need attach/detach adapter
> functions. If they do, there is probably some other way to achieve the
> same effect without needing the functions.

Care to enlighten me on what this other way would be? It's easy to
claim this of pretty much everything, but that's not valuable without
details on the "other way", sorry.

On top of that, you'd also need to explain why adapter_attach and
adapter_detach are bad, before claiming that they should be replaced by
something else. I am worried about the use that is done of these
callbacks at the moment, not by the callbacks themselves. If there's a
legitimate use of them (I still need to check the V4L/DVB drivers) I
see no reason to remove them.

> i2c-dev may need access to
> some i2c core structures to avoid the need for these calls.

That's certainly a possibility, yes, and I'll look into this if it
turns out that i2c-dev is the last user of any part of i2c_driver. But
then I wonder why i2c-dev was implemented the way it was, if all it
needed was access to some i2c-core internals.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/5/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Care to enlighten me on what this other way would be? It's easy to
>  claim this of pretty much everything, but that's not valuable without
>  details on the "other way", sorry.

I'm just proposing optimizations to the scheme, we are in agreement on
the core of the proposal.  Let's go ahead with the way you proposed
(with it moved to i2c_driver). Then after you get everything working
with the new scheme I'll see if I can add optimizations to remove some
of the entry points. It is more important to get the old to new driver
conversion going.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Trent Piepho]

On Wed, 4 Jun 2008, Jean Delvare wrote:
> On Wed, 4 Jun 2008 15:28:54 -0400, Jon Smirl wrote:
> > On 6/4/08, Jon Smirl <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > >  > Hi all,
> > >  >
> > >  >  This patch set demonstrates a new concept which I would like to add to
> > >  >  the i2c subsystem. It is named "i2c listeners" and is based on the
> > >  >  following structure:
> >
> > Could the existing API be used something like this?
> >

> > scans the registered driver structures looking for class matches. When
> > a match is encountered it repeatedly calls the driver's probe function
> > with the addresses from the range. Driver returns true/false on each
> > probe which triggers creation of the device.
>
> The driver's probe function is, by definition of the device driver
> model, called on an existing device. So the probe function cannot
> decide whether the device should be created or not - it's too late at
> this point.

Couldn't you say the probe function is called on a potential device?  The
probe function can return -ENODEV, in which can other driver's probes get
called, and it's perfectly ok if no driver binds to it.

The way PCI works, is that when a new pci bus is created, each address is
probed and a device is created if anything responds.  The generic bus code
tries to match each device to a driver or drivers and calls those drivers'
probe functions.  The drivers don't have to claim the device in the probe
function.  The bus code handles all the cases of a driver or bus getting
added or removed in various orders.

So why can't I2C do this too?

When a new bus is created, the i2c core scans it and creates a device for
each chip that responds.  Each driver's probe function is called in turn on
the device and it gets to device if it wants to drive it or not, probably
based on i2c address range since there is usually little else to go on.

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Wednesday 04 June 2008, Trent Piepho wrote:
> Couldn't you say the probe function is called on a potential device?  The
> probe function can return -ENODEV, in which can other driver's probes get
> called, and it's perfectly ok if no driver binds to it.
> 
> The way PCI works, is that when a new pci bus is created, each address is
> probed

... by config space accessors which all PCI devices support.


> and a device is created if anything responds.  The generic bus code 
> tries to match each device to a driver or drivers

... using a formally managed set of product identifiers.


>	 and calls those drivers' 
> probe functions.  The drivers don't have to claim the device in the probe
> function.  The bus code handles all the cases of a driver or bus getting
> added or removed in various orders.
> 
> So why can't I2C do this too?

No such product identifiers, and in general no way to tell
what's sitting at a given address.  And in fact, there's no
sure way to tell if a device is present there, since when
an I2C device is busy, it's not required to ack its address.



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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1701 bytes --]

On Wed, 4 Jun 2008, David Brownell wrote:
> On Wednesday 04 June 2008, Trent Piepho wrote:
> > Couldn't you say the probe function is called on a potential device?  The
> > probe function can return -ENODEV, in which can other driver's probes get
> > called, and it's perfectly ok if no driver binds to it.
> >
> > The way PCI works, is that when a new pci bus is created, each address is
> > probed
>
> ... by config space accessors which all PCI devices support.

PCI is not immune to broken hardware.  It's much better than I2C of course,
which doesn't even have a scanning method devices are supposed to follow.

> > and a device is created if anything responds.  The generic bus code
> > tries to match each device to a driver or drivers
>
> ... using a formally managed set of product identifiers.

There are plenty of PCI devices with broken ids.

> >	 and calls those drivers'
> > probe functions.  The drivers don't have to claim the device in the probe
> > function.  The bus code handles all the cases of a driver or bus getting
> > added or removed in various orders.
> >
> > So why can't I2C do this too?
>
> No such product identifiers, and in general no way to tell
> what's sitting at a given address.  And in fact, there's no
> sure way to tell if a device is present there, since when
> an I2C device is busy, it's not required to ack its address.

So scanning an I2C bus doesn't work as well as with PCI, we all know that.
How does creating a new "i2c listeners" system solve any of these problems?
You still want to probe for a chip and there's no foolproof way to do that,
and there's still no way to know for sure what chip you have found.


[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Wednesday 04 June 2008, Trent Piepho wrote:
> So scanning an I2C bus doesn't work as well as with PCI, we all know that.

As I said, not just "scanning" but also "identifying".

PCI was designed explicitly for automatic configuration.
I2C was not.


> How does creating a new "i2c listeners" system solve any of these problems?

That's an entirely different (and unrelated) question.

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Trent Piepho]

On Wed, 4 Jun 2008, David Brownell wrote:
> On Wednesday 04 June 2008, Trent Piepho wrote:
> > So scanning an I2C bus doesn't work as well as with PCI, we all know that.
>
> As I said, not just "scanning" but also "identifying".

No need to be so pedantic, by scanning I meant probing and identifying.

> PCI was designed explicitly for automatic configuration.
> I2C was not.
>
>
> > How does creating a new "i2c listeners" system solve any of these problems?
>
> That's an entirely different (and unrelated) question.

For a thread on the subject "Introduce i2c listeners", I don't think so.
When I first saw the patch, I wondered why exsting driver model couldn't
handle it.  It looks like Jon thought the same thing.  No one ever said
scanning/probing/identifying/whatever an I2C bus worked as well as it does
on a bus like PCI.  But I don't see why that prevents the driver model from
being used.

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Thursday 05 June 2008, Trent Piepho wrote:.
> 
> > PCI was designed explicitly for automatic configuration.
> > I2C was not.
> >
> >
> > > How does creating a new "i2c listeners" system solve any of these problems?
> >
> > That's an entirely different (and unrelated) question.
> 
> For a thread on the subject "Introduce i2c listeners", I don't think so.

For the subthread on why I2C can't scan like PCI ... it
sure seemed to me like it was!  But ok, you maybe weren't
focussed on that subthread.


> When I first saw the patch, I wondered why exsting driver model couldn't
> handle it.  It looks like Jon thought the same thing.  No one ever said
> scanning/probing/identifying/whatever an I2C bus worked as well as it does
> on a bus like PCI.  But I don't see why that prevents the driver model from
> being used.

Well, without commenting on these new listeners/notifiers ...

All I can observe is that what the driver model provides is just
the framework through which the device enumeration is exposed.

That device enumeration is bus-specific.  It can build on hardware
for device identification (as with USB and PCI) and maybe hotplug
(USB, and PCI flavors like Cardbus and hotplug PCI).  Or it can
build on software tables, as in PNPACPI and OpenFirmware.  You can
view cases like USB and PCMCIA cards (8/16 bits) as being hybrid:
hotplug hardware, combined with software tables in the device.

The I2C problem has been that hardware conventions can't work in
general, so Linux has had to come up with its own conventions for
software tables.

If nothing else, we have an existence proof (in "new style" I2C
drivers) that we *can* use the driver model with I2C.

- Dave

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

Hi Trent,

On Wed, 4 Jun 2008 17:40:05 -0700 (PDT), Trent Piepho wrote:
> So scanning an I2C bus doesn't work as well as with PCI, we all know that.
> How does creating a new "i2c listeners" system solve any of these problems?

There are two key properties of the i2c listeners:

1* They live besides the driver model and complement it, rather than
   replacing it. They have to, because the driver model cares about
   device enumeration and device/driver binding, NOT device detection
   and identification in the hardware sense of the terms.

2* Listeners allow the i2c-core to delegate the device detection /
   identification to the drivers themselves (avoiding a fat, slow and
   unmaintainable centralized detection routine) but the drivers are
   still not creating the devices themselves (so the device driver
   model's spirit is respected.)

> You still want to probe for a chip and there's no foolproof way to do that,
> and there's still no way to know for sure what chip you have found.

That's correct, but the fact is that for example all I2C-based hardware
monitoring drivers are already doing their best to identify the chips
they support, with good success. The question is not whether we'll do
that or not - we _have_ to do it in many cases. The question is how we
implement it.

Currently we have a separate binding model for this case, I'm proposing
something that would instead fit in the standard binding model, only
adding the part that the binding model doesn't cover. This is certainly
an improvement, I don't think anyone can deny this. Not to say that my
proposal cannot be improved further - it certainly can.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jon Smirl]

On 6/4/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Wednesday 04 June 2008, Trent Piepho wrote:
>  > Couldn't you say the probe function is called on a potential device?  The
>  > probe function can return -ENODEV, in which can other driver's probes get
>  > called, and it's perfectly ok if no driver binds to it.
>  >
>  > The way PCI works, is that when a new pci bus is created, each address is
>  > probed
>
>
> ... by config space accessors which all PCI devices support.

I made a mistake comparing this to PCI IDs. I should not have used PCI
IDs as an example and gotten us off on the tangent of not having a
global i2c device namespace.

Adding the address range to search is an unrelated problem to the name
space issue.


>
>
>
>  > and a device is created if anything responds.  The generic bus code
>  > tries to match each device to a driver or drivers
>
>
> ... using a formally managed set of product identifiers.
>
>
>
>  >        and calls those drivers'
>  > probe functions.  The drivers don't have to claim the device in the probe
>  > function.  The bus code handles all the cases of a driver or bus getting
>  > added or removed in various orders.
>  >
>  > So why can't I2C do this too?
>
>
> No such product identifiers, and in general no way to tell
>  what's sitting at a given address.  And in fact, there's no
>  sure way to tell if a device is present there, since when
>  an I2C device is busy, it's not required to ack its address.
>
>
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[Jean Delvare]

Hi Jon,

On Wed, 4 Jun 2008 14:55:06 -0400, Jon Smirl wrote:
> On 6/4/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >  This patch set demonstrates a new concept which I would like to add to
> >  the i2c subsystem. It is named "i2c listeners" and is based on the
> >  following structure:
> 
> How does this fit in with the existing driver bus structure in kernel?

It's outside of the bus structure. The device driver model defines how
drivers bind to the devices they support. It doesn't imply much on how
devices are instantiated. For bus types where the present devices are
enumerated (like PCI) the device instantiation is pretty
straightforward, but in the case of I2C/SMBus where you have to either
know in advance or actively probe and guess which devices are there,
you clearly need additional code and strategy to instantiate the
devices.

That's the very reason why the i2c subsystem didn't originally follow
the device driver model (until David converted it to add support for
the standard model in addition to our custom one.)

> This sounds a lot like hot plug and buses already implement hot plug
> events.  This isn't exactly hot plug but it may be close enough that
> the existing bus APIs will work. Note that both devices and entire
> buses can be hot plugged with existing code.

I don't think it is related to hotplug at all. The issue here is not
whether devices are hotplugged or not. It's also not about how the
drivers are notified about devices and how they bind to them. The issue
is the step before that: how do you find and instantiate the devices if
you have no preliminary knowledge on what devices are present.

Now, if I have really been duplicating a mechanism that exists in the
driver core, I'll be happy to simplify my code to take benefit of it.
But then please point me to the exact piece of code you're thinking
about.

-- 
Jean Delvare

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

Re: [PATCH 0/4] i2c: Introduce i2c listeners

[David Brownell]

On Wednesday 04 June 2008, Jean Delvare wrote:
> I would appreciate feedback on both the concept and the implementation.
> David, I am particularly interested in your feedback, of course.

Which, for the record, will be a bit delayed with respect to
the core i2c_listener abstraction.  This deserves a bit more
thought than I can put into it in the immediate future.
(Ditto firing up the rig needed to test the lm75 updates.)

Except for this small bit of instafeedback:  the approach of
splitting the detect() logic out, as an adjunct to a core
"new style" driver, is an interesting way to address some core
problems with legacy drivers (vs the driver model).

It gives a graceful migration model towards new-style code, and
lets drivers get updated without needing to start by abandoning
the "poke at the hardware" style enumeration in those cases
where it seems to act mostly OK today.  While I'd like to see
that poking be largely removed from the kernel, factoring it out
(in i2c_listener or whatever) is clearly useful.

- Dave

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

[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 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