[PATCH 1/2] i2c: Multiplexed I2C bus core support.

[PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Subject: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/i2c/Kconfig        |   10 +++
 drivers/i2c/Makefile       |    3 +-
 drivers/i2c/i2c-core.c     |   73 +++++++++++++++++--
 drivers/i2c/i2c-dev.c      |   32 ++++++++-
 drivers/i2c/i2c-mux.c      |  167
++++++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/muxes/Kconfig  |    8 ++
 drivers/i2c/muxes/Makefile |    6 ++
 include/linux/i2c.h        |   14 ++++
 8 files changed, 303 insertions(+), 10 deletions(-)
 create mode 100755 drivers/i2c/i2c-mux.c
 create mode 100755 drivers/i2c/muxes/Kconfig
 create mode 100755 drivers/i2c/muxes/Makefile

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index d7ece13..0c2f86a 100755
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -35,6 +35,16 @@ config I2C_COMPAT
 	  other user-space package which expects i2c adapters to be class
 	  devices. If you don't know, say Y.

+config I2C_MUX
+	tristate "I2C bus multiplexing support"
+	depends on I2C
+	help
+	  Say Y here if you want the I2C core to support the ability to
+	  handle multiplexed I2C bus topologies, by presenting each
+	  multiplexed segment as a I2C adapter.
+
+source drivers/i2c/muxes/Kconfig
+
 config I2C_CHARDEV
 	tristate "I2C device interface"
 	help
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index ba26e6c..281e2a7 100755
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -4,8 +4,9 @@

 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
 obj-$(CONFIG_I2C)		+= i2c-core.o
+obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
-obj-y				+= busses/ chips/ algos/
+obj-y				+= busses/ chips/ muxes/ algos/

 ifeq ($(CONFIG_I2C_DEBUG_CORE),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2965043..83119ae 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -20,7 +20,9 @@
 /* With some changes from Kyösti Mälkki <kmalkki@cc.hut.fi>.
    All SMBus-related things are written by Frodo Looijaard <frodol@dds.nl>
    SMBus 2.0 support by Mark Studebaker <mdsxyz123-/E1597aS9LQAvxtiuMwx3w@public.gmane.org> and
-   Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> */
+      Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
+   Mux support by Rodolfo Giometti <giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org> and
+      Michael Lawnick <michael.lawnick.ext-OYasijW0DpE@public.gmane.org>*/

 #include <linux/module.h>
 #include <linux/kernel.h>
@@ -937,9 +939,66 @@ static int __i2c_check_addr(struct device *dev,
void *addrp)
 	return 0;
 }

+static int i2c_check_clients(struct i2c_adapter *adapter, int addr)
+{
+	int result = 0;
+
+	result = device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
+
+	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
+		result = i2c_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
+	
+	return result;
+}
+
 static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
 {
-	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
+	int result = 0;
+
+	result = i2c_check_clients(adapter, addr);
+	
+	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
+	{
+		struct i2c_client dummy;
+		char buff;
+
+		dummy.adapter = to_i2c_adapter(adapter->dev.parent);
+		dummy.addr = addr;
+		dummy.flags = 0;
+
+		result = i2c_master_recv(&dummy, &buff, 1) == 1;
+	}
+		
+	return result;
+}
+
+static void i2c_mux_tree_lock(struct i2c_adapter *adap)
+{
+	mutex_lock_nested(&adap->bus_lock, SINGLE_DEPTH_NESTING);
+	if(adap->dev.parent->bus == &i2c_bus_type)
+		i2c_mux_tree_lock(to_i2c_adapter(adap->dev.parent));
+}
+
+static int i2c_mux_tree_trylock(struct i2c_adapter *adap)
+{
+	int ret;
+	
+	ret = mutex_trylock(&adap->bus_lock);
+
+	if(ret && (adap->dev.parent->bus == &i2c_bus_type)) {
+		ret = i2c_mux_tree_trylock(to_i2c_adapter(adap->dev.parent));
+		if (!ret)
+			mutex_unlock(&adap->bus_lock);
+	}
+
+	return ret;
+}
+
+static void i2c_mux_tree_unlock(struct i2c_adapter *adap)
+{
+	if(adap->dev.parent->bus == &i2c_bus_type)
+		i2c_mux_tree_unlock(to_i2c_adapter(adap->dev.parent));
+	mutex_unlock(&adap->bus_lock);
 }

 /**
@@ -1092,12 +1151,12 @@ int i2c_transfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
 #endif

 		if (in_atomic() || irqs_disabled()) {
-			ret = mutex_trylock(&adap->bus_lock);
+			ret = i2c_mux_tree_trylock(adap);
 			if (!ret)
 				/* I2C activity is ongoing. */
 				return -EAGAIN;
 		} else {
-			mutex_lock_nested(&adap->bus_lock, adap->level);
+			i2c_mux_tree_lock(adap);
 		}

 		/* Retry automatically on arbitration loss */
@@ -1109,7 +1168,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
 			if (time_after(jiffies, orig_jiffies + adap->timeout))
 				break;
 		}
-		mutex_unlock(&adap->bus_lock);
+		i2c_mux_tree_unlock(adap);

 		return ret;
 	} else {
@@ -1913,7 +1972,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter,
u16 addr, unsigned short flags,
 	flags &= I2C_M_TEN | I2C_CLIENT_PEC;

 	if (adapter->algo->smbus_xfer) {
-		mutex_lock(&adapter->bus_lock);
+		i2c_mux_tree_lock(adapter);

 		/* Retry automatically on arbitration loss */
 		orig_jiffies = jiffies;
@@ -1927,7 +1986,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter,
u16 addr, unsigned short flags,
 				       orig_jiffies + adapter->timeout))
 				break;
 		}
-		mutex_unlock(&adapter->bus_lock);
+		i2c_mux_tree_unlock(adapter);
 	} else
 		res = i2c_smbus_xfer_emulated(adapter,addr,flags,read_write,
 					      command, protocol, data);
diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 7e13d2d..ce2f1a1 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -190,16 +190,44 @@ static int i2cdev_check(struct device *dev, void
*addrp)

 	if (!client || client->addr != *(unsigned int *)addrp)
 		return 0;
-
+	
 	return dev->driver ? -EBUSY : 0;
 }

+static int i2cdev_check_clients(struct i2c_adapter *adapter, int addr)
+{
+	int result = 0;
+
+	result = device_for_each_child(&adapter->dev, &addr, i2cdev_check);
+
+	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
+		result = i2cdev_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
+	
+	return result;
+}
+
 /* This address checking function differs from the one in i2c-core
    in that it considers an address with a registered device, but no
    driver bound to it, as NOT busy. */
 static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int
addr)
 {
-	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
+	int result = 0;
+
+	result = i2cdev_check_clients(adapter, addr);
+	
+	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
+	{
+		struct i2c_client dummy;
+		char buff;
+
+		dummy.adapter = to_i2c_adapter(adapter->dev.parent);
+		dummy.addr = addr;
+		dummy.flags = 0;
+
+		result = i2c_master_recv(&dummy, &buff, 1) == 1;
+	}
+		
+	return result;
 }

 static noinline int i2cdev_ioctl_rdrw(struct i2c_client *client,
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
new file mode 100755
index 0000000..014d488
--- /dev/null
+++ b/drivers/i2c/i2c-mux.c
@@ -0,0 +1,167 @@
+/*
+ * Multiplexed I2C bus driver.
+ *
+ * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
+ * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
+ *
+ * Simplifies access to complex multiplexed I2C bus topologies, by
presenting
+ * each multiplexed bus segment as a virtual I2C adapter.  Supports
multi-level
+ * mux'ing (mux behind a mux).
+ *
+ * Based on:
+ *    i2c-virt.c from Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ *    i2c-virtual.c from Copyright (c) 2004 Google, Inc. (Ken Harrenstien)
+ *    i2c-virtual.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
+ * which was:
+ *    Adapted from i2c-adap-ibm_ocp.c
+ *    Original file Copyright 2000-2002 MontaVista Software Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/i2c-id.h>
+
+struct i2c_mux_priv {
+	struct i2c_adapter adap;
+	struct i2c_algorithm algo;
+
+	struct i2c_adapter *parent_adap;
+	void *client;			/* The mux chip/device */
+	u32 id;				/* the mux id */
+
+	int (*select)(struct i2c_adapter *, void *, u32);
+	int (*deselect)(struct i2c_adapter *, void *, u32);
+};
+
+#define VIRT_TIMEOUT		(HZ/2)
+#define VIRT_RETRIES		3
+
+static int i2c_mux_master_xfer(struct i2c_adapter *adap,
+				struct i2c_msg msgs[], int num)
+{
+	struct i2c_mux_priv *priv = adap->algo_data;
+	struct i2c_adapter *parent = priv->parent_adap;
+	int ret;
+
+	/* Select the right mux port and perform the transfer. */	
+
+	ret = priv->select(parent, priv->client, priv->id);
+	if (ret >= 0)
+		ret = parent->algo->master_xfer(parent, msgs, num);
+	priv->deselect(parent, priv->client, priv->id);
+
+	return ret;
+}
+
+static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
+				u16 addr, unsigned short flags,
+				char read_write, u8 command,
+				int size, union i2c_smbus_data *data)
+{
+	struct i2c_mux_priv *priv = adap->algo_data;
+	struct i2c_adapter *parent = priv->parent_adap;
+	int ret;
+
+	/* Select the right mux port and perform the transfer. */	
+
+	ret = priv->select(parent, priv->client, priv->id);
+	if (ret == 0)
+		ret = parent->algo->smbus_xfer(parent, addr, flags,
+					   read_write, command, size, data);
+	priv->deselect(parent, priv->client, priv->id);
+
+	return ret;
+}
+
+/* Return the parent's functionality for the virtual adapter */
+static u32 i2c_mux_functionality(struct i2c_adapter *adap)
+{
+	struct i2c_mux_priv *priv = adap->algo_data;
+	struct i2c_adapter *parent = priv->parent_adap;
+
+	return parent->algo->functionality(parent);
+}
+
+struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
+				void *client, u32 force_nr, u32 mux_val,
+				int (*select_cb) (struct i2c_adapter *,
+						void *, u32),
+				int (*deselect_cb) (struct i2c_adapter *,
+						void *, u32))
+{
+	struct i2c_mux_priv *priv;
+	int ret;
+
+	priv = kzalloc(sizeof(struct i2c_mux_priv), GFP_KERNEL);
+	if (!priv)
+		return NULL;
+
+	/* Set up private adapter data */
+	priv->parent_adap = parent;
+	priv->client = client;
+	priv->id = mux_val;
+	priv->select = select_cb;
+	priv->deselect = deselect_cb;
+
+	/* Need to do algo dynamically because we don't know ahead
+	 * of time what sort of physical adapter we'll be dealing with.
+	 */
+	if (parent->algo->master_xfer)
+		priv->algo.master_xfer = i2c_mux_master_xfer;
+	if (parent->algo->smbus_xfer)
+		priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
+	priv->algo.functionality = i2c_mux_functionality;
+
+	/* Now fill out new adapter structure */
+	snprintf(priv->adap.name, sizeof(priv->adap.name),
+			"i2c-%d-mux (mux %02x)",
+			i2c_adapter_id(parent), mux_val);
+	priv->adap.owner = THIS_MODULE;
+	priv->adap.id = i2c_adapter_id(parent);
+	priv->adap.algo = &priv->algo;
+	priv->adap.algo_data = priv;
+	priv->adap.timeout = VIRT_TIMEOUT;
+	priv->adap.retries = VIRT_RETRIES;
+	priv->adap.dev.parent = &parent->dev;
+	
+	if (force_nr) {
+		priv->adap.nr = force_nr;
+		ret = i2c_add_numbered_adapter(&priv->adap);
+	} else {
+		ret = i2c_add_adapter(&priv->adap);
+	}
+	if (ret < 0) {
+		kfree(priv);
+		return NULL;
+	}
+
+	dev_info(&parent->dev, "i2c-mux-%d: Virtual I2C bus - "
+		"physical bus i2c-%d, client %02x, port %d\n",
+		i2c_adapter_id(&priv->adap), i2c_adapter_id(parent), ((struct
i2c_client*)priv->client)->addr, mux_val);
+
+	return &priv->adap;
+}
+EXPORT_SYMBOL_GPL(i2c_add_mux_adapter);
+
+int i2c_del_mux_adapter(struct i2c_adapter *adap)
+{
+	struct i2c_mux_priv *priv = adap->algo_data;
+	int ret;
+
+	ret = i2c_del_adapter(adap);
+	if (ret < 0)
+		return ret;
+	kfree(priv);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_del_mux_adapter);
+
+MODULE_AUTHOR("Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>");
+MODULE_DESCRIPTION("Virtual I2C driver for multiplexed I2C busses");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
new file mode 100755
index 0000000..95b9bde
--- /dev/null
+++ b/drivers/i2c/muxes/Kconfig
@@ -0,0 +1,8 @@
+#
+# Multiplexer I2C chip drivers configuration
+#
+
+menu "Multiplexer I2C Chip support"
+	depends on I2C && I2C_MUX && EXPERIMENTAL
+
+endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
new file mode 100755
index 0000000..0416a52
--- /dev/null
+++ b/drivers/i2c/muxes/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for multiplexer I2C chip drivers.
+
+ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
+EXTRA_CFLAGS += -DDEBUG -DI2C_DEBUG_CORE
+endif
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 7b40cda..24ff2e5 100755
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -580,6 +580,20 @@ union i2c_smbus_data {
 #define I2C_SMBUS_BLOCK_PROC_CALL   7		/* SMBus 2.0 */
 #define I2C_SMBUS_I2C_BLOCK_DATA    8

+/*
+ * Called to create a 'virtual' i2c bus which represents a multiplexed bus
+ * segment.  The client and mux_val are passed to the select and deselect
+ * callback functions to perform hardware-specific mux control.
+ */
+struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
+				void *client, u32 force_nr, u32 mux_val,
+				int (*select_cb) (struct i2c_adapter *,
+						void *, u32),
+				int (*deselect_cb) (struct i2c_adapter *,
+						void *, u32));
+
+int i2c_del_mux_adapter(struct i2c_adapter *adap);
+

 #ifdef __KERNEL__

-- 
1.6.1.2

Re: [PATCH 1/2] i2c: driver for PCA954x I2C multiplexer series.

[Michael Lawnick]

Subject: [PATCH 2/2] i2c: driver for PCA954x I2C multiplexer series.

Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
Acked-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/i2c/muxes/Kconfig   |    9 ++
 drivers/i2c/muxes/Makefile  |    2 +
 drivers/i2c/muxes/pca954x.c |  325
+++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pca954x.h |   13 ++
 4 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100755 drivers/i2c/muxes/pca954x.c
 create mode 100755 include/linux/i2c/pca954x.h

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 95b9bde..72c7055 100755
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -5,4 +5,13 @@
 menu "Multiplexer I2C Chip support"
 	depends on I2C && I2C_MUX && EXPERIMENTAL

+config I2CMUX_PCA954x
+	tristate "Philips PCA953x I2C Mux/switches"
+	help
+	  If you say yes here you get support for the Philips PCA954x
+	  I2C mux/switch devices.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called pca954x.
+
 endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 0416a52..2338d4c 100755
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -1,6 +1,8 @@
 #
 # Makefile for multiplexer I2C chip drivers.

+obj-$(CONFIG_I2CMUX_PCA954x)	+= pca954x.o
+
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG -DI2C_DEBUG_CORE
 endif
diff --git a/drivers/i2c/muxes/pca954x.c b/drivers/i2c/muxes/pca954x.c
new file mode 100755
index 0000000..0440a69
--- /dev/null
+++ b/drivers/i2c/muxes/pca954x.c
@@ -0,0 +1,325 @@
+/*
+ * I2C multiplexer
+ *
+ * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
+ * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
+ *
+ * This module supports the PCA954x series of I2C multiplexer/switch chips
+ * made by Philips Semiconductors.
+ * This includes the:
+ *     PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
+ *     and PCA9548.
+ *
+ * These chips are all controlled via the I2C bus itself, and all have a
+ * single 8-bit register (normally at 0x70).  The upstream "parent" bus
fans
+ * out to two, four, or eight downstream busses or channels; which of these
+ * are selected is determined by the chip type and register contents.  A
+ * mux can select only one sub-bus at a time; a switch can select any
+ * combination simultaneously.
+ *
+ * Based on:
+ *    pca954x.c from Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ * Copyright (C) 2006
+ *
+ * Based on:
+ *    pca954x.c from Ken Harrenstien
+ * Copyright (C) 2004 Google, Inc. (Ken Harrenstien)
+ *
+ * Based on:
+ *    i2c-virtual_cb.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
+ * and
+ *    pca9540.c from Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>, which was
+ *    based on pcf8574.c from the same project by Frodo Looijaard,
+ *    Philip Edelbrock, Dan Eaton and Aurelien Jarno.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/i2c/pca954x.h>
+
+#define PCA954X_MAX_NCHANS 8
+
+enum pca_type {
+	pca_9540,
+	pca_9542,
+	pca_9543,
+	pca_9544,
+	pca_9545,
+	pca_9546,
+	pca_9547,
+	pca_9548,
+};
+
+struct pca954x {
+	struct i2c_client *client;
+	struct i2c_client dev;
+	unsigned int type;
+	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
+
+	u8 last_chan;
+	unsigned int deselect_on_exit:1;
+	int (*is_chan_connected)(struct i2c_adapter *adap,
+			struct i2c_client *client, u32 chan);
+};
+
+struct chip_desc {
+	u8 nchans;
+	u8 enable;		/* used for muxes only */
+	enum muxtype {
+		pca954x_ismux = 0,
+		pca954x_isswi
+	} muxtype;
+};
+
+/* Provide specs for the PCA954x types we know about */
+static struct chip_desc chips[] = {
+	[pca_9540] = {
+		.nchans = 2,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9542] = {
+		.nchans = 2,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9543] = {
+		.nchans = 2,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9544] = {
+		.nchans = 4,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9545] = {
+		.nchans = 4,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9546] = {
+		.nchans = 4,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9547] = {
+		.nchans = 8,
+		.enable = 0x8,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9548] = {
+		.nchans = 8,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+};
+
+static const struct i2c_device_id pca954x_id[] = {
+	{ "pca9540", pca_9540 },
+	{ "pca9542", pca_9542 },
+	{ "pca9543", pca_9543 },
+	{ "pca9544", pca_9544 },
+	{ "pca9545", pca_9545 },
+	{ "pca9546", pca_9546 },
+	{ "pca9547", pca_9547 },
+	{ "pca9548", pca_9548 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pca954x_id);
+
+static int pca954x_xfer(struct i2c_adapter *adap,
+			struct i2c_client *client, int read_write, u8 *val)
+{
+	int ret = -ENODEV;
+
+	if (adap->algo->master_xfer) {
+		struct i2c_msg msg;
+		char buf[1];
+
+		msg.addr = client->addr;
+		msg.flags = (read_write == I2C_SMBUS_READ ? I2C_M_RD : 0);
+		msg.len = 1;
+		buf[0] = *val;
+		msg.buf = buf;
+		ret = adap->algo->master_xfer(adap, &msg, 1);
+	} else if (adap->algo->smbus_xfer) {
+		union i2c_smbus_data data;
+		ret = adap->algo->smbus_xfer(adap, client->addr,
+						client->flags & I2C_M_TEN,
+						read_write,
+						*val, I2C_SMBUS_BYTE, &data);
+	}
+
+	return ret;
+}
+
+static int pca954x_select_chan(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata((struct i2c_client *) client);
+	struct chip_desc *chip = &chips[data->type];
+	u8 regval = 0;
+	int ret = 0;
+
+	/* Callback to check for bus connection */
+	if (data->is_chan_connected) {
+		ret = data->is_chan_connected(adap, client, chan);
+		if (!ret)
+			return -EBUSY;
+	}
+
+	/* we make switches look like muxes, not sure how to be smarter */
+	if (chip->muxtype == pca954x_ismux)
+		regval = chan | chip->enable;
+	else
+		regval = 1 << chan;
+
+	/* Only select the channel if its different from the last channel */
+	if (data->last_chan != chan) {
+		ret = pca954x_xfer(adap, client, I2C_SMBUS_WRITE, &regval);
+		data->last_chan = chan;
+	}
+
+	return ret;
+}
+
+static int pca954x_deselect_mux(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata((struct i2c_client *) client);
+	struct chip_desc *chip = &chips[data->type];
+	u8 regval = 0;
+
+	/* Check if we have to stay on the last channel we selected */
+	if (!data->deselect_on_exit && (chip->muxtype == pca954x_ismux))
+		return 0;
+
+	/* Signal we are deselecting active channel */
+	data->last_chan = -1;
+
+	regval = chan | ~chip->enable;
+	return pca954x_xfer(adap, client, I2C_SMBUS_WRITE, &regval);
+}
+
+/*
+ * I2C init/probing/exit functions
+ */
+static int pca954x_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
+	struct pca954x_platform_data *pdata = client->dev.platform_data;
+	int i, n, f;
+	struct pca954x *data;
+	int ret = -ENODEV;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
+		goto err;
+
+	data = kzalloc(sizeof(struct pca954x), GFP_KERNEL);
+	if (!data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	i2c_set_clientdata(client, data);
+
+	/* Read the mux register at addr.  This does two things: it verifies
+	 * that the mux is in fact present, and fetches its current
+	 * contents for possible use with a future deselect algorithm.
+	 */
+	i = i2c_smbus_read_byte(client);
+	if (i < 0) {
+		dev_warn(&client->dev, "probe failed\n");
+		goto exit_free;
+	}
+
+	data->type = id->driver_data;
+	data->last_chan = -1;			/* force the first selection */
+
+	/* Now create virtual busses and adapters for them */
+	for (i = 0; i < chips[data->type].nchans; i++) {
+		f = 0;				/* dynamic adap number */
+		if (pdata && (i < pdata->num_modes))
+			f = pdata->modes[i].adap_id; /* force static number */
+
+		data->virt_adaps[i] = i2c_add_mux_adapter(adap, client, f, i,
+						&pca954x_select_chan,
+						&pca954x_deselect_mux);
+		if (data->virt_adaps[i] == NULL) {
+			ret = -ENODEV;
+			goto virt_reg_failed;
+		}
+
+		data->deselect_on_exit = pdata ?
+				pdata->modes[i].deselect_on_exit : 1;
+		data->is_chan_connected = pdata ?
+				pdata->modes[i].is_chan_connected : NULL;
+	}
+
+	dev_info(&client->dev, "registered %d virtual busses for I2C %s %s\n",
+			i, chips[data->type].muxtype == pca954x_ismux ?
+					"mux" : "switch", client->name);
+
+	return 0;
+
+virt_reg_failed:
+	for (n = 0; n < i; n++)
+		i2c_del_mux_adapter(data->virt_adaps[n]);
+exit_free:
+	kfree(data);
+err:
+	return ret;
+}
+
+static int __devexit pca954x_remove(struct i2c_client *client)
+{
+	struct pca954x *data = i2c_get_clientdata(client);
+	struct chip_desc *chip = &chips[data->type];
+	int i, err;
+
+	for (i = 0; i < chip->nchans; ++i)
+		if (data->virt_adaps[i]) {
+			err = i2c_del_mux_adapter(data->virt_adaps[i]);
+			if (err)
+				return err;
+			data->virt_adaps[i] = NULL;
+		}
+
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_driver pca954x_driver = {
+	.driver = {
+		.name	= "pca954x",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= pca954x_probe,
+	.remove	= __devexit_p(pca954x_remove),
+	.id_table = pca954x_id,
+};
+
+static int __init pca954x_init(void)
+{
+	return i2c_add_driver(&pca954x_driver);
+}
+
+static void __exit pca954x_exit(void)
+{
+	i2c_del_driver(&pca954x_driver);
+}
+
+module_init(pca954x_init);
+module_exit(pca954x_exit);
+
+MODULE_AUTHOR("Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>");
+MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/i2c/pca954x.h b/include/linux/i2c/pca954x.h
new file mode 100755
index 0000000..18d5e79
--- /dev/null
+++ b/include/linux/i2c/pca954x.h
@@ -0,0 +1,13 @@
+/* Platform data for the PCA954x I2C multiplexers */
+
+struct pca954x_platform_mode {
+	int		adap_id;
+	unsigned int	deselect_on_exit:1;
+	int		(*is_chan_connected)(struct i2c_adapter *adap,
+					struct i2c_client *client, u32 chan);
+};
+
+struct pca954x_platform_data {
+	struct pca954x_platform_mode *modes;
+	int num_modes;
+};
-- 
1.6.1.2

[PATCH 2/2] i2c: driver for PCA954x I2C multiplexer series.

[Michael Lawnick]

Sometimes you check and check and... and then sh*# happens.

Subject: [PATCH 2/2] i2c: driver for PCA954x I2C multiplexer series.

Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
Acked-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/i2c/muxes/Kconfig   |    9 ++
 drivers/i2c/muxes/Makefile  |    2 +
 drivers/i2c/muxes/pca954x.c |  325
+++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pca954x.h |   13 ++
 4 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100755 drivers/i2c/muxes/pca954x.c
 create mode 100755 include/linux/i2c/pca954x.h

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 95b9bde..72c7055 100755
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -5,4 +5,13 @@
 menu "Multiplexer I2C Chip support"
 	depends on I2C && I2C_MUX && EXPERIMENTAL

+config I2CMUX_PCA954x
+	tristate "Philips PCA953x I2C Mux/switches"
+	help
+	  If you say yes here you get support for the Philips PCA954x
+	  I2C mux/switch devices.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called pca954x.
+
 endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 0416a52..2338d4c 100755
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -1,6 +1,8 @@
 #
 # Makefile for multiplexer I2C chip drivers.

+obj-$(CONFIG_I2CMUX_PCA954x)	+= pca954x.o
+
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG -DI2C_DEBUG_CORE
 endif
diff --git a/drivers/i2c/muxes/pca954x.c b/drivers/i2c/muxes/pca954x.c
new file mode 100755
index 0000000..0440a69
--- /dev/null
+++ b/drivers/i2c/muxes/pca954x.c
@@ -0,0 +1,325 @@
+/*
+ * I2C multiplexer
+ *
+ * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
+ * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
+ *
+ * This module supports the PCA954x series of I2C multiplexer/switch chips
+ * made by Philips Semiconductors.
+ * This includes the:
+ *     PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
+ *     and PCA9548.
+ *
+ * These chips are all controlled via the I2C bus itself, and all have a
+ * single 8-bit register (normally at 0x70).  The upstream "parent" bus
fans
+ * out to two, four, or eight downstream busses or channels; which of these
+ * are selected is determined by the chip type and register contents.  A
+ * mux can select only one sub-bus at a time; a switch can select any
+ * combination simultaneously.
+ *
+ * Based on:
+ *    pca954x.c from Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ * Copyright (C) 2006
+ *
+ * Based on:
+ *    pca954x.c from Ken Harrenstien
+ * Copyright (C) 2004 Google, Inc. (Ken Harrenstien)
+ *
+ * Based on:
+ *    i2c-virtual_cb.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
+ * and
+ *    pca9540.c from Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>, which was
+ *    based on pcf8574.c from the same project by Frodo Looijaard,
+ *    Philip Edelbrock, Dan Eaton and Aurelien Jarno.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/i2c/pca954x.h>
+
+#define PCA954X_MAX_NCHANS 8
+
+enum pca_type {
+	pca_9540,
+	pca_9542,
+	pca_9543,
+	pca_9544,
+	pca_9545,
+	pca_9546,
+	pca_9547,
+	pca_9548,
+};
+
+struct pca954x {
+	struct i2c_client *client;
+	struct i2c_client dev;
+	unsigned int type;
+	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
+
+	u8 last_chan;
+	unsigned int deselect_on_exit:1;
+	int (*is_chan_connected)(struct i2c_adapter *adap,
+			struct i2c_client *client, u32 chan);
+};
+
+struct chip_desc {
+	u8 nchans;
+	u8 enable;		/* used for muxes only */
+	enum muxtype {
+		pca954x_ismux = 0,
+		pca954x_isswi
+	} muxtype;
+};
+
+/* Provide specs for the PCA954x types we know about */
+static struct chip_desc chips[] = {
+	[pca_9540] = {
+		.nchans = 2,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9542] = {
+		.nchans = 2,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9543] = {
+		.nchans = 2,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9544] = {
+		.nchans = 4,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9545] = {
+		.nchans = 4,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9546] = {
+		.nchans = 4,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+	[pca_9547] = {
+		.nchans = 8,
+		.enable = 0x8,
+		.muxtype = pca954x_ismux,
+	},
+	[pca_9548] = {
+		.nchans = 8,
+		.enable = 0x0,
+		.muxtype = pca954x_isswi,
+	},
+};
+
+static const struct i2c_device_id pca954x_id[] = {
+	{ "pca9540", pca_9540 },
+	{ "pca9542", pca_9542 },
+	{ "pca9543", pca_9543 },
+	{ "pca9544", pca_9544 },
+	{ "pca9545", pca_9545 },
+	{ "pca9546", pca_9546 },
+	{ "pca9547", pca_9547 },
+	{ "pca9548", pca_9548 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pca954x_id);
+
+static int pca954x_xfer(struct i2c_adapter *adap,
+			struct i2c_client *client, int read_write, u8 *val)
+{
+	int ret = -ENODEV;
+
+	if (adap->algo->master_xfer) {
+		struct i2c_msg msg;
+		char buf[1];
+
+		msg.addr = client->addr;
+		msg.flags = (read_write == I2C_SMBUS_READ ? I2C_M_RD : 0);
+		msg.len = 1;
+		buf[0] = *val;
+		msg.buf = buf;
+		ret = adap->algo->master_xfer(adap, &msg, 1);
+	} else if (adap->algo->smbus_xfer) {
+		union i2c_smbus_data data;
+		ret = adap->algo->smbus_xfer(adap, client->addr,
+						client->flags & I2C_M_TEN,
+						read_write,
+						*val, I2C_SMBUS_BYTE, &data);
+	}
+
+	return ret;
+}
+
+static int pca954x_select_chan(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata((struct i2c_client *) client);
+	struct chip_desc *chip = &chips[data->type];
+	u8 regval = 0;
+	int ret = 0;
+
+	/* Callback to check for bus connection */
+	if (data->is_chan_connected) {
+		ret = data->is_chan_connected(adap, client, chan);
+		if (!ret)
+			return -EBUSY;
+	}
+
+	/* we make switches look like muxes, not sure how to be smarter */
+	if (chip->muxtype == pca954x_ismux)
+		regval = chan | chip->enable;
+	else
+		regval = 1 << chan;
+
+	/* Only select the channel if its different from the last channel */
+	if (data->last_chan != chan) {
+		ret = pca954x_xfer(adap, client, I2C_SMBUS_WRITE, &regval);
+		data->last_chan = chan;
+	}
+
+	return ret;
+}
+
+static int pca954x_deselect_mux(struct i2c_adapter *adap,
+				void *client, u32 chan)
+{
+	struct pca954x *data = i2c_get_clientdata((struct i2c_client *) client);
+	struct chip_desc *chip = &chips[data->type];
+	u8 regval = 0;
+
+	/* Check if we have to stay on the last channel we selected */
+	if (!data->deselect_on_exit && (chip->muxtype == pca954x_ismux))
+		return 0;
+
+	/* Signal we are deselecting active channel */
+	data->last_chan = -1;
+
+	regval = chan | ~chip->enable;
+	return pca954x_xfer(adap, client, I2C_SMBUS_WRITE, &regval);
+}
+
+/*
+ * I2C init/probing/exit functions
+ */
+static int pca954x_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
+	struct pca954x_platform_data *pdata = client->dev.platform_data;
+	int i, n, f;
+	struct pca954x *data;
+	int ret = -ENODEV;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
+		goto err;
+
+	data = kzalloc(sizeof(struct pca954x), GFP_KERNEL);
+	if (!data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	i2c_set_clientdata(client, data);
+
+	/* Read the mux register at addr.  This does two things: it verifies
+	 * that the mux is in fact present, and fetches its current
+	 * contents for possible use with a future deselect algorithm.
+	 */
+	i = i2c_smbus_read_byte(client);
+	if (i < 0) {
+		dev_warn(&client->dev, "probe failed\n");
+		goto exit_free;
+	}
+
+	data->type = id->driver_data;
+	data->last_chan = -1;			/* force the first selection */
+
+	/* Now create virtual busses and adapters for them */
+	for (i = 0; i < chips[data->type].nchans; i++) {
+		f = 0;				/* dynamic adap number */
+		if (pdata && (i < pdata->num_modes))
+			f = pdata->modes[i].adap_id; /* force static number */
+
+		data->virt_adaps[i] = i2c_add_mux_adapter(adap, client, f, i,
+						&pca954x_select_chan,
+						&pca954x_deselect_mux);
+		if (data->virt_adaps[i] == NULL) {
+			ret = -ENODEV;
+			goto virt_reg_failed;
+		}
+
+		data->deselect_on_exit = pdata ?
+				pdata->modes[i].deselect_on_exit : 1;
+		data->is_chan_connected = pdata ?
+				pdata->modes[i].is_chan_connected : NULL;
+	}
+
+	dev_info(&client->dev, "registered %d virtual busses for I2C %s %s\n",
+			i, chips[data->type].muxtype == pca954x_ismux ?
+					"mux" : "switch", client->name);
+
+	return 0;
+
+virt_reg_failed:
+	for (n = 0; n < i; n++)
+		i2c_del_mux_adapter(data->virt_adaps[n]);
+exit_free:
+	kfree(data);
+err:
+	return ret;
+}
+
+static int __devexit pca954x_remove(struct i2c_client *client)
+{
+	struct pca954x *data = i2c_get_clientdata(client);
+	struct chip_desc *chip = &chips[data->type];
+	int i, err;
+
+	for (i = 0; i < chip->nchans; ++i)
+		if (data->virt_adaps[i]) {
+			err = i2c_del_mux_adapter(data->virt_adaps[i]);
+			if (err)
+				return err;
+			data->virt_adaps[i] = NULL;
+		}
+
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_driver pca954x_driver = {
+	.driver = {
+		.name	= "pca954x",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= pca954x_probe,
+	.remove	= __devexit_p(pca954x_remove),
+	.id_table = pca954x_id,
+};
+
+static int __init pca954x_init(void)
+{
+	return i2c_add_driver(&pca954x_driver);
+}
+
+static void __exit pca954x_exit(void)
+{
+	i2c_del_driver(&pca954x_driver);
+}
+
+module_init(pca954x_init);
+module_exit(pca954x_exit);
+
+MODULE_AUTHOR("Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>");
+MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/i2c/pca954x.h b/include/linux/i2c/pca954x.h
new file mode 100755
index 0000000..18d5e79
--- /dev/null
+++ b/include/linux/i2c/pca954x.h
@@ -0,0 +1,13 @@
+/* Platform data for the PCA954x I2C multiplexers */
+
+struct pca954x_platform_mode {
+	int		adap_id;
+	unsigned int	deselect_on_exit:1;
+	int		(*is_chan_connected)(struct i2c_adapter *adap,
+					struct i2c_client *client, u32 chan);
+};
+
+struct pca954x_platform_data {
+	struct pca954x_platform_mode *modes;
+	int num_modes;
+};
-- 1.6.1.2 -- To unsubscribe from this list: send the line "unsubscribe
linux-i2c" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More
majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: driver for PCA954x I2C multiplexer series.

[Michael Lawnick]

ignore this noise please

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Michael Lawnick said the following:
> Subject: [PATCH 1/2] i2c: Multiplexed I2C bus core support.
> 
Ping.

(I dream to see it in .34)

-- 
KR
Michael

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Jean Delvare]

Hi Michael,

On Tue, 02 Mar 2010 15:36:53 +0100, Michael Lawnick wrote:
> Michael Lawnick said the following:
> > Subject: [PATCH 1/2] i2c: Multiplexed I2C bus core support.
>
> Ping.
> 
> (I dream to see it in .34)

This isn't going to happen, sorry. The new i2c feature in 2.6.34 will
be SMBus alert support, which had been waiting for about 2 years.
Multiplexing support will have to wait for the next merge window. Right
now I am working on flushing my patch queues as much as I can. Even if
I had the time to look into your patches this week (and honestly it
doesn't seem realistic) it would be too late to make it into this merge
window. Your patches will need some testing which can't realistically
be achieved in a couple days.

I understand that you are getting impatient and I would be as well if I
were you, but I'm doing the best I can with the limited time I have.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Jean Delvare said the following:
> Hi Michael,
> 
> On Tue, 02 Mar 2010 15:36:53 +0100, Michael Lawnick wrote:
>> Michael Lawnick said the following:
>> > Subject: [PATCH 1/2] i2c: Multiplexed I2C bus core support.
>>
>> Ping.
>> 
>> (I dream to see it in .34)
> 
> This isn't going to happen, sorry. 
> [...]
> 
> I understand that you are getting impatient and I would be as well if I
> were you, but I'm doing the best I can with the limited time I have.
> 
I just had a dream ...

-- 
KR
Michael

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Jean Delvare]

Hi Michael,

Sorry for the long delay. Here finally comes a review of your code.

On Mon, 25 Jan 2010 13:13:48 +0100, Michael Lawnick wrote:
> Subject: [PATCH 1/2] i2c: Multiplexed I2C bus core support.
> 
> Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/i2c/Kconfig        |   10 +++
>  drivers/i2c/Makefile       |    3 +-
>  drivers/i2c/i2c-core.c     |   73 +++++++++++++++++--
>  drivers/i2c/i2c-dev.c      |   32 ++++++++-
>  drivers/i2c/i2c-mux.c      |  167
> ++++++++++++++++++++++++++++++++++++++++++++

Your e-mail client wraps long lines, this caused your patch to not
apply. I had to edit it manually before I was able to apply it. Please
fix your setup so that I don't have to do it again next time.

Please also make sure to run your patch through scripts/checkpatch.pl.
I found many trailing whitespace, and even after removing them, the
script still finds 9 style errors which should be fixed.

>  drivers/i2c/muxes/Kconfig  |    8 ++
>  drivers/i2c/muxes/Makefile |    6 ++
>  include/linux/i2c.h        |   14 ++++
>  8 files changed, 303 insertions(+), 10 deletions(-)
>  create mode 100755 drivers/i2c/i2c-mux.c
>  create mode 100755 drivers/i2c/muxes/Kconfig
>  create mode 100755 drivers/i2c/muxes/Makefile
> 
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index d7ece13..0c2f86a 100755
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -35,6 +35,16 @@ config I2C_COMPAT
>  	  other user-space package which expects i2c adapters to be class
>  	  devices. If you don't know, say Y.
> 
> +config I2C_MUX
> +	tristate "I2C bus multiplexing support"
> +	depends on I2C
> +	help
> +	  Say Y here if you want the I2C core to support the ability to
> +	  handle multiplexed I2C bus topologies, by presenting each
> +	  multiplexed segment as a I2C adapter.
> +
> +source drivers/i2c/muxes/Kconfig
> +
>  config I2C_CHARDEV
>  	tristate "I2C device interface"
>  	help
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index ba26e6c..281e2a7 100755
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -4,8 +4,9 @@
> 
>  obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
>  obj-$(CONFIG_I2C)		+= i2c-core.o
> +obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
>  obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
> -obj-y				+= busses/ chips/ algos/
> +obj-y				+= busses/ chips/ muxes/ algos/

This doesn't work. You can't ask the build system to operate on an
empty directory. You'll have to move the inclusion of this directory to
the patch adding the first mux driver. I know it's not nice, but
there's no other way I know of.

> 
>  ifeq ($(CONFIG_I2C_DEBUG_CORE),y)
>  EXTRA_CFLAGS += -DDEBUG
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 2965043..83119ae 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -20,7 +20,9 @@
>  /* With some changes from Kyösti Mälkki <kmalkki-kf+aQKke1yb1KXRcyAk9cg@public.gmane.org>.
>     All SMBus-related things are written by Frodo Looijaard <frodol@dds.nl>
>     SMBus 2.0 support by Mark Studebaker <mdsxyz123-/E1597aS9LQAvxtiuMwx3w@public.gmane.org> and
> -   Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> */
> +      Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> +   Mux support by Rodolfo Giometti <giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org> and
> +      Michael Lawnick <michael.lawnick.ext-OYasijW0DpE@public.gmane.org>*/

Missing space before end of comment.

> 
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> @@ -937,9 +939,66 @@ static int __i2c_check_addr(struct device *dev,
> void *addrp)
>  	return 0;
>  }
> 
> +static int i2c_check_clients(struct i2c_adapter *adapter, int addr)
> +{
> +	int result = 0;

Useless initialization.

> +
> +	result = device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
> +
> +	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))

All devices don't have a parent, so you should first check that
adapter->dev.parent isn't NULL. Try your code with i2c-stub for
example, it would crash.

> +		result = i2c_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
> +	
> +	return result;
> +}

> +
>  static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
>  {
> -	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
> +	int result = 0;

Useless initialization. There are many other cases below, I won't
comment on each but please fix them all.

> +
> +	result = i2c_check_clients(adapter, addr);
> +	
> +	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))

Here again, you have no guarantee that the adapter device has a parent.
There are many other occurrences of this below. Maybe you want a small
helper function.

> +	{
> +		struct i2c_client dummy;
> +		char buff;
> +

You must zero all the struct i2c_client fields before you use it.

> +		dummy.adapter = to_i2c_adapter(adapter->dev.parent);
> +		dummy.addr = addr;
> +		dummy.flags = 0;
> +
> +		result = i2c_master_recv(&dummy, &buff, 1) == 1;
> +	}

I don't understand this part, please explain, and add a comment before
the code if the above is really needed. I would think that the
recursive address business check would be sufficient, is it not?

We should avoid as much as possible to run arbitrary transactions on
the I2C bus. We are already using such arbitrary transactions for
device detections, but we are trying to move away from this now. Let's
not add more such cases. Some devices may hang on transactions they
don't expect!

If the above is really needed, it will have to be implemented
differently anyway, as you have no guarantee that the adapter supports
raw I2C transactions. Most SMBus controllers don't.

> +		
> +	return result;
> +}
> +
> +static void i2c_mux_tree_lock(struct i2c_adapter *adap)
> +{
> +	mutex_lock_nested(&adap->bus_lock, SINGLE_DEPTH_NESTING);

i2c-core moved to rt_mutex, so your code will need to be updated
accordingly.

> +	if(adap->dev.parent->bus == &i2c_bus_type)
> +		i2c_mux_tree_lock(to_i2c_adapter(adap->dev.parent));
> +}

In the end, each time one wants access to a segment of the tree, they
will have to lock the root segment to succeed. So do we really need to
lock each segment in turn? Can't we just look for the root and lock
only it? This would be faster.

> +
> +static int i2c_mux_tree_trylock(struct i2c_adapter *adap)
> +{
> +	int ret;
> +	
> +	ret = mutex_trylock(&adap->bus_lock);
> +
> +	if(ret && (adap->dev.parent->bus == &i2c_bus_type)) {
> +		ret = i2c_mux_tree_trylock(to_i2c_adapter(adap->dev.parent));
> +		if (!ret)
> +			mutex_unlock(&adap->bus_lock);
> +	}
> +
> +	return ret;
> +}
> +
> +static void i2c_mux_tree_unlock(struct i2c_adapter *adap)
> +{
> +	if(adap->dev.parent->bus == &i2c_bus_type)
> +		i2c_mux_tree_unlock(to_i2c_adapter(adap->dev.parent));
> +	mutex_unlock(&adap->bus_lock);
>  }
> 
>  /**
> @@ -1092,12 +1151,12 @@ int i2c_transfer(struct i2c_adapter *adap,
> struct i2c_msg *msgs, int num)
>  #endif
> 
>  		if (in_atomic() || irqs_disabled()) {
> -			ret = mutex_trylock(&adap->bus_lock);
> +			ret = i2c_mux_tree_trylock(adap);
>  			if (!ret)
>  				/* I2C activity is ongoing. */
>  				return -EAGAIN;
>  		} else {
> -			mutex_lock_nested(&adap->bus_lock, adap->level);
> +			i2c_mux_tree_lock(adap);
>  		}
> 
>  		/* Retry automatically on arbitration loss */
> @@ -1109,7 +1168,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>  			if (time_after(jiffies, orig_jiffies + adap->timeout))
>  				break;
>  		}
> -		mutex_unlock(&adap->bus_lock);
> +		i2c_mux_tree_unlock(adap);
> 
>  		return ret;
>  	} else {
> @@ -1913,7 +1972,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter,
> u16 addr, unsigned short flags,
>  	flags &= I2C_M_TEN | I2C_CLIENT_PEC;
> 
>  	if (adapter->algo->smbus_xfer) {
> -		mutex_lock(&adapter->bus_lock);
> +		i2c_mux_tree_lock(adapter);
> 
>  		/* Retry automatically on arbitration loss */
>  		orig_jiffies = jiffies;
> @@ -1927,7 +1986,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter,
> u16 addr, unsigned short flags,
>  				       orig_jiffies + adapter->timeout))
>  				break;
>  		}
> -		mutex_unlock(&adapter->bus_lock);
> +		i2c_mux_tree_unlock(adapter);
>  	} else
>  		res = i2c_smbus_xfer_emulated(adapter,addr,flags,read_write,
>  					      command, protocol, data);
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 7e13d2d..ce2f1a1 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -190,16 +190,44 @@ static int i2cdev_check(struct device *dev, void
> *addrp)
> 
>  	if (!client || client->addr != *(unsigned int *)addrp)
>  		return 0;
> -
> +	

Please don't include whitespace changes in your patch.

>  	return dev->driver ? -EBUSY : 0;
>  }
> 
> +static int i2cdev_check_clients(struct i2c_adapter *adapter, int addr)
> +{
> +	int result = 0;
> +
> +	result = device_for_each_child(&adapter->dev, &addr, i2cdev_check);
> +
> +	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
> +		result = i2cdev_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
> +	
> +	return result;
> +}
> +
>  /* This address checking function differs from the one in i2c-core
>     in that it considers an address with a registered device, but no
>     driver bound to it, as NOT busy. */
>  static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int
> addr)
>  {
> -	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
> +	int result = 0;
> +
> +	result = i2cdev_check_clients(adapter, addr);
> +	
> +	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
> +	{
> +		struct i2c_client dummy;
> +		char buff;
> +
> +		dummy.adapter = to_i2c_adapter(adapter->dev.parent);
> +		dummy.addr = addr;
> +		dummy.flags = 0;
> +
> +		result = i2c_master_recv(&dummy, &buff, 1) == 1;
> +	}
> +		
> +	return result;
>  }

This duplicated code in i2c-dev starts being a problem, but I guess it
can't be avoided as long as i2c-dev doesn't properly integrate with the
standard device driver binding model :(

> 
>  static noinline int i2cdev_ioctl_rdrw(struct i2c_client *client,
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> new file mode 100755
> index 0000000..014d488
> --- /dev/null
> +++ b/drivers/i2c/i2c-mux.c
> @@ -0,0 +1,167 @@
> +/*
> + * Multiplexed I2C bus driver.
> + *
> + * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> + * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
> + *
> + * Simplifies access to complex multiplexed I2C bus topologies, by
> presenting
> + * each multiplexed bus segment as a virtual I2C adapter.  Supports

I would prefer the term "separate I2C adapter" rather than "virtual I2C
adapter". There's nothing virtual about these bus segments. i2c-stub is
a virtual I2C adapter.

> multi-level
> + * mux'ing (mux behind a mux).
> + *
> + * Based on:
> + *    i2c-virt.c from Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> + *    i2c-virtual.c from Copyright (c) 2004 Google, Inc. (Ken Harrenstien)

"from Copyright" looks bad.

> + *    i2c-virtual.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> + * which was:
> + *    Adapted from i2c-adap-ibm_ocp.c
> + *    Original file Copyright 2000-2002 MontaVista Software Inc.

There's probably no point in going that far in history. I doubt
i2c-adap-ibm_ocp.c was a bus multiplexer driver.

> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-id.h>

You don't need this one.

> +
> +struct i2c_mux_priv {

Would be good to document what this structure represents conceptually.

> +	struct i2c_adapter adap;
> +	struct i2c_algorithm algo;
> +
> +	struct i2c_adapter *parent_adap;

Why not just "parent"?

> +	void *client;			/* The mux chip/device */

"client" is a confusing term, because it is heavily used in the i2c
subsystem and drivers to refer to I2C devices. As the mux chip does not
have to be an I2C device, I'd rather use a more generic term, such as
"mux" or "mux_dev".

> +	u32 id;				/* the mux id */

Comment looks wrong, I think this is a channel ID for the given mux
device and not a mux device ID?

> +
> +	int (*select)(struct i2c_adapter *, void *, u32);
> +	int (*deselect)(struct i2c_adapter *, void *, u32);

Please name the arguments, otherwise it's difficult to figure out what
each argument represents. Not sure why you don't simply pass a struct
i2c_mux_priv, BTW. It contains all the information you need.

I'm unsure why deselect would take what I presume is a mux id argument?
I'm not even sure what this function would do... Not all chips can
deselect all their channels. Some very simple I2C multiplexers are
controlled by a single GPIO pin, and must always have exactly one
channel selected. So the deselect callback should certainly be optional.

> +};
> +
> +#define VIRT_TIMEOUT		(HZ/2)
> +#define VIRT_RETRIES		3
> +
> +static int i2c_mux_master_xfer(struct i2c_adapter *adap,
> +				struct i2c_msg msgs[], int num)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	struct i2c_adapter *parent = priv->parent_adap;
> +	int ret;
> +
> +	/* Select the right mux port and perform the transfer. */	
> +
> +	ret = priv->select(parent, priv->client, priv->id);
> +	if (ret >= 0)

Should be "ret == 0" for consistency with the next function.

> +		ret = parent->algo->master_xfer(parent, msgs, num);
> +	priv->deselect(parent, priv->client, priv->id);

As discussed earlier, I don't think that we want to always deselect the
multiplexer channels after a transfer, for performance reasons.
Probably the implementation of deselect() should be optional, and
reserved to the few cases where it is really desirable (for example
tuner chips behind gates to reduce electrical noise on TV adapters.)

> +
> +	return ret;
> +}
> +
> +static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
> +				u16 addr, unsigned short flags,
> +				char read_write, u8 command,
> +				int size, union i2c_smbus_data *data)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	struct i2c_adapter *parent = priv->parent_adap;
> +	int ret;
> +
> +	/* Select the right mux port and perform the transfer. */	
> +
> +	ret = priv->select(parent, priv->client, priv->id);
> +	if (ret == 0)
> +		ret = parent->algo->smbus_xfer(parent, addr, flags,
> +					   read_write, command, size, data);
> +	priv->deselect(parent, priv->client, priv->id);
> +
> +	return ret;
> +}
> +
> +/* Return the parent's functionality for the virtual adapter */
> +static u32 i2c_mux_functionality(struct i2c_adapter *adap)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	struct i2c_adapter *parent = priv->parent_adap;
> +
> +	return parent->algo->functionality(parent);
> +}
> +
> +struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
> +				void *client, u32 force_nr, u32 mux_val,
> +				int (*select_cb) (struct i2c_adapter *,
> +						void *, u32),
> +				int (*deselect_cb) (struct i2c_adapter *,
> +						void *, u32))

Please drop the trailing _cb, it's already pretty clear that these are
callback functions.

> +{
> +	struct i2c_mux_priv *priv;
> +	int ret;
> +
> +	priv = kzalloc(sizeof(struct i2c_mux_priv), GFP_KERNEL);

You must include <linux/slab.h> for this.

> +	if (!priv)
> +		return NULL;
> +
> +	/* Set up private adapter data */
> +	priv->parent_adap = parent;
> +	priv->client = client;
> +	priv->id = mux_val;
> +	priv->select = select_cb;
> +	priv->deselect = deselect_cb;
> +
> +	/* Need to do algo dynamically because we don't know ahead
> +	 * of time what sort of physical adapter we'll be dealing with.
> +	 */
> +	if (parent->algo->master_xfer)
> +		priv->algo.master_xfer = i2c_mux_master_xfer;
> +	if (parent->algo->smbus_xfer)
> +		priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
> +	priv->algo.functionality = i2c_mux_functionality;
> +
> +	/* Now fill out new adapter structure */
> +	snprintf(priv->adap.name, sizeof(priv->adap.name),
> +			"i2c-%d-mux (mux %02x)",

I don't get the use of hexadecimal to represent a channel number/id.
Decimal would be more readable. I would also use the term "channel" or
"port" before that ID, rather than "mux".

> +			i2c_adapter_id(parent), mux_val);
> +	priv->adap.owner = THIS_MODULE;
> +	priv->adap.id = i2c_adapter_id(parent);

No, this is incorrect. I know the name i2c_adapter_id is very
confusing, but it's returning a bus number, not a bus ID. If you need
the parent's ID, you must copy it manually, there's no helper function
for that.

> +	priv->adap.algo = &priv->algo;
> +	priv->adap.algo_data = priv;
> +	priv->adap.timeout = VIRT_TIMEOUT;
> +	priv->adap.retries = VIRT_RETRIES;

I don't think these make sense. The actual transaction will happen at
the root level, that's where the relevant timeout and retries settings
are. The values above will never be used, so I'd rather not set them.

> +	priv->adap.dev.parent = &parent->dev;
> +	
> +	if (force_nr) {
> +		priv->adap.nr = force_nr;
> +		ret = i2c_add_numbered_adapter(&priv->adap);
> +	} else {
> +		ret = i2c_add_adapter(&priv->adap);
> +	}
> +	if (ret < 0) {

Might be worth an error message?

> +		kfree(priv);
> +		return NULL;
> +	}
> +
> +	dev_info(&parent->dev, "i2c-mux-%d: Virtual I2C bus - "
> +		"physical bus i2c-%d, client %02x, port %d\n",
> +		i2c_adapter_id(&priv->adap), i2c_adapter_id(parent), ((struct
> i2c_client*)priv->client)->addr, mux_val);

This is wrong and will crash for non-I2C mux chips. You just can't
assume that it is safe to cast priv->client to an i2c_client.

> +
> +	return &priv->adap;
> +}
> +EXPORT_SYMBOL_GPL(i2c_add_mux_adapter);
> +
> +int i2c_del_mux_adapter(struct i2c_adapter *adap)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	int ret;
> +
> +	ret = i2c_del_adapter(adap);
> +	if (ret < 0)
> +		return ret;
> +	kfree(priv);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_del_mux_adapter);
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>");
> +MODULE_DESCRIPTION("Virtual I2C driver for multiplexed I2C busses");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> new file mode 100755
> index 0000000..95b9bde
> --- /dev/null
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -0,0 +1,8 @@
> +#
> +# Multiplexer I2C chip drivers configuration
> +#
> +
> +menu "Multiplexer I2C Chip support"
> +	depends on I2C && I2C_MUX && EXPERIMENTAL

I2C_MUX itself already depends in I2C, so I guess it's enough to depend
on I2C_MUX.

> +
> +endmenu
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> new file mode 100755
> index 0000000..0416a52
> --- /dev/null
> +++ b/drivers/i2c/muxes/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer I2C chip drivers.
> +
> +ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)

CONFIG_I2C_DEBUG_CHIP is gone. I suggest you introduce a new option,
CONFIG_I2C_DEBUG_MUX.

> +EXTRA_CFLAGS += -DDEBUG -DI2C_DEBUG_CORE

-DDEBUG should be enough.

> +endif
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 7b40cda..24ff2e5 100755
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -580,6 +580,20 @@ union i2c_smbus_data {
>  #define I2C_SMBUS_BLOCK_PROC_CALL   7		/* SMBus 2.0 */
>  #define I2C_SMBUS_I2C_BLOCK_DATA    8
> 
> +/*
> + * Called to create a 'virtual' i2c bus which represents a multiplexed bus
> + * segment.  The client and mux_val are passed to the select and deselect
> + * callback functions to perform hardware-specific mux control.
> + */
> +struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
> +				void *client, u32 force_nr, u32 mux_val,
> +				int (*select_cb) (struct i2c_adapter *,
> +						void *, u32),
> +				int (*deselect_cb) (struct i2c_adapter *,
> +						void *, u32));
> +
> +int i2c_del_mux_adapter(struct i2c_adapter *adap);
> +

These functions are not available if mux support wasn't selected, so
their declarations shouldn't be there in that case. In fact, I would
suggest that you move these to a separate header, <linux/i2c-mux.h>,
which only multiplexer drivers (and i2c-mux.c) would include. This
keeps things nicely separated.

> 
>  #ifdef __KERNEL__
> 

I'll go on with reviewing the PCA954x mux driver.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Jean Delvare]

One thing I forgot:

On Thu, 15 Apr 2010 14:49:06 +0200, Jean Delvare wrote:
> On Mon, 25 Jan 2010 13:13:48 +0100, Michael Lawnick wrote:
> > +static int i2c_check_clients(struct i2c_adapter *adapter, int addr)
> > +{
> > +	int result = 0;
> 
> Useless initialization.
> 
> > +
> > +	result = device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
> > +
> > +	if (!result && (adapter->dev.parent->bus == &i2c_bus_type))
> 
> All devices don't have a parent, so you should first check that
> adapter->dev.parent isn't NULL. Try your code with i2c-stub for
> example, it would crash.
> 
> > +		result = i2c_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
> > +	
> > +	return result;
> > +}

As discussed some weeks ago, this isn't actually sufficient. You don't
only need to check the parent segments for address business, you also
need to check all child segments, recursively. If any child segment has
a device using the address in question, then you can't use it.

This may be more difficult to implement. In particular, you'll have to
pay attention to locking.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Jean Delvare said the following:
> Hi Michael,
> 
> Sorry for the long delay. Here finally comes a review of your code.
Thank you!

[Very much comments snipped]
 It will take a while to process that, so reply to certain points and V2
might last some day^wwee^w time.

-- 
KR
Michael

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Jean Delvare]

On Fri, 16 Apr 2010 14:44:42 +0200, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Michael,
> > 
> > Sorry for the long delay. Here finally comes a review of your code.
> Thank you!
> 
> [Very much comments snipped]
>  It will take a while to process that, so reply to certain points and V2
> might last some day^wwee^w time.

Given how long it took me to review the code, I can't blame you for
that ;)

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Jean Delvare said the following:
> One thing I forgot:
> 
>> > +		result = i2c_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
>> > +	
>> > +	return result;
>> > +}
> 
> As discussed some weeks ago, this isn't actually sufficient. You don't
> only need to check the parent segments for address business, you also
> need to check all child segments, recursively. If any child segment has
> a device using the address in question, then you can't use it.
> 
> This may be more difficult to implement. In particular, you'll have to
> pay attention to locking.
> 
:-) This can't happen if we keep the part you commented on in the other
mail about probing for client one level above. Then this situation can't
arise.

-- 
KR
Michael

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Jean Delvare]

On Fri, 16 Apr 2010 15:10:11 +0200, Michael Lawnick wrote:
> Jean Delvare said the following:
> > One thing I forgot:
> > 
> >> > +		result = i2c_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
> >> > +	
> >> > +	return result;
> >> > +}
> > 
> > As discussed some weeks ago, this isn't actually sufficient. You don't
> > only need to check the parent segments for address business, you also
> > need to check all child segments, recursively. If any child segment has
> > a device using the address in question, then you can't use it.
> > 
> > This may be more difficult to implement. In particular, you'll have to
> > pay attention to locking.
> > 
> :-) This can't happen if we keep the part you commented on in the other
> mail about probing for client one level above. Then this situation can't
> arise.

I don't understand. In your code, the probe is done at the parent
level, where address business had already been tested. What is needed
is child segments checking, so the other side of the tree. I just can't
see how your code would help with that.

Can you please explain why the probe is needed, and what it is doing
that the standard address business check didn't cover already?

-- 
Jean Delvare

Re: [PATCH 2/2] i2c: driver for PCA954x I2C multiplexer series.

[Jean Delvare]

Hi Michael,

On Mon, 25 Jan 2010 14:00:23 +0100, Michael Lawnick wrote:
> Sometimes you check and check and... and then sh*# happens.
> 
> Subject: [PATCH 2/2] i2c: driver for PCA954x I2C multiplexer series.
> 
> Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> Acked-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>

Review:

> ---
>  drivers/i2c/muxes/Kconfig   |    9 ++
>  drivers/i2c/muxes/Makefile  |    2 +
>  drivers/i2c/muxes/pca954x.c |  325
> +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/pca954x.h |   13 ++
>  4 files changed, 349 insertions(+), 0 deletions(-)
>  create mode 100755 drivers/i2c/muxes/pca954x.c
>  create mode 100755 include/linux/i2c/pca954x.h
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 95b9bde..72c7055 100755
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -5,4 +5,13 @@
>  menu "Multiplexer I2C Chip support"
>  	depends on I2C && I2C_MUX && EXPERIMENTAL
> 
> +config I2CMUX_PCA954x

I would prefer I2C_MUX instead of I2CMUX. 

> +	tristate "Philips PCA953x I2C Mux/switches"

Typo: PCA954x.

> +	help
> +	  If you say yes here you get support for the Philips PCA954x
> +	  I2C mux/switch devices.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called pca954x.
> +
>  endmenu
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 0416a52..2338d4c 100755
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -1,6 +1,8 @@
>  #
>  # Makefile for multiplexer I2C chip drivers.
> 
> +obj-$(CONFIG_I2CMUX_PCA954x)	+= pca954x.o
> +
>  ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)

CONFIG_I2C_DEBUG_CHIP is gone.

>  EXTRA_CFLAGS += -DDEBUG -DI2C_DEBUG_CORE
>  endif
> diff --git a/drivers/i2c/muxes/pca954x.c b/drivers/i2c/muxes/pca954x.c
> new file mode 100755
> index 0000000..0440a69
> --- /dev/null
> +++ b/drivers/i2c/muxes/pca954x.c
> @@ -0,0 +1,325 @@
> +/*
> + * I2C multiplexer
> + *
> + * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> + * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
> + *
> + * This module supports the PCA954x series of I2C multiplexer/switch chips
> + * made by Philips Semiconductors.
> + * This includes the:
> + *     PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547
> + *     and PCA9548.
> + *
> + * These chips are all controlled via the I2C bus itself, and all have a
> + * single 8-bit register (normally at 0x70).  The upstream "parent" bus fans

This is confusing. 0x70 is the device's I2C address, not a register
address. There is no register address for these chips, as they have a
single register. So I wouldn't mention the address at all.

> + * out to two, four, or eight downstream busses or channels; which of these
> + * are selected is determined by the chip type and register contents.  A
> + * mux can select only one sub-bus at a time; a switch can select any
> + * combination simultaneously.
> + *
> + * Based on:
> + *    pca954x.c from Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> + * Copyright (C) 2006
> + *
> + * Based on:
> + *    pca954x.c from Ken Harrenstien
> + * Copyright (C) 2004 Google, Inc. (Ken Harrenstien)
> + *
> + * Based on:
> + *    i2c-virtual_cb.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> + * and
> + *    pca9540.c from Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>, which was
> + *    based on pcf8574.c from the same project by Frodo Looijaard,
> + *    Philip Edelbrock, Dan Eaton and Aurelien Jarno.

Once again I think you go a little too deep in history. You can skip
the reference to the pcf8574.c drivers which is completely unrelated to
I2C bus multiplexing.

> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>

You don't need this one. OTOH you need <linux/slab.h> for kzalloc() and
<linux/device.h> for dev_info() etc.

> +
> +#include <linux/i2c/pca954x.h>
> +
> +#define PCA954X_MAX_NCHANS 8
> +
> +enum pca_type {
> +	pca_9540,
> +	pca_9542,
> +	pca_9543,
> +	pca_9544,
> +	pca_9545,
> +	pca_9546,
> +	pca_9547,
> +	pca_9548,
> +};
> +
> +struct pca954x {
> +	struct i2c_client *client;
> +	struct i2c_client dev;

You don't use either anywhere.

> +	unsigned int type;

Why not enum pca_type?

> +	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
> +
> +	u8 last_chan;
> +	unsigned int deselect_on_exit:1;
> +	int (*is_chan_connected)(struct i2c_adapter *adap,
> +			struct i2c_client *client, u32 chan);

What's this?

> +};
> +
> +struct chip_desc {
> +	u8 nchans;
> +	u8 enable;		/* used for muxes only */
> +	enum muxtype {
> +		pca954x_ismux = 0,
> +		pca954x_isswi
> +	} muxtype;

Why don't you simply go with a "is_switch" flag? Or you could even use
enable == 0 to differentiate between muxes and switches.

> +};
> +
> +/* Provide specs for the PCA954x types we know about */
> +static struct chip_desc chips[] = {

Could be const.

> +	[pca_9540] = {
> +		.nchans = 2,
> +		.enable = 0x4,
> +		.muxtype = pca954x_ismux,
> +	},
> +	[pca_9542] = {
> +		.nchans = 2,
> +		.enable = 0x4,
> +		.muxtype = pca954x_ismux,
> +	},
> +	[pca_9543] = {
> +		.nchans = 2,
> +		.enable = 0x0,

You don't have to explicitly mention 0 values when initializing static
structs.

> +		.muxtype = pca954x_isswi,
> +	},
> +	[pca_9544] = {
> +		.nchans = 4,
> +		.enable = 0x4,
> +		.muxtype = pca954x_ismux,
> +	},
> +	[pca_9545] = {
> +		.nchans = 4,
> +		.enable = 0x0,
> +		.muxtype = pca954x_isswi,
> +	},
> +	[pca_9546] = {
> +		.nchans = 4,
> +		.enable = 0x0,
> +		.muxtype = pca954x_isswi,
> +	},
> +	[pca_9547] = {
> +		.nchans = 8,
> +		.enable = 0x8,
> +		.muxtype = pca954x_ismux,
> +	},
> +	[pca_9548] = {
> +		.nchans = 8,
> +		.enable = 0x0,
> +		.muxtype = pca954x_isswi,
> +	},
> +};

As far as I can see, the 9540 and 9542 are handled the same, and so are
the 9545 and 9546. So you could remove 2 entries from the table above,
saving some space.

> +
> +static const struct i2c_device_id pca954x_id[] = {
> +	{ "pca9540", pca_9540 },
> +	{ "pca9542", pca_9542 },
> +	{ "pca9543", pca_9543 },
> +	{ "pca9544", pca_9544 },
> +	{ "pca9545", pca_9545 },
> +	{ "pca9546", pca_9546 },
> +	{ "pca9547", pca_9547 },
> +	{ "pca9548", pca_9548 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pca954x_id);
> +
> +static int pca954x_xfer(struct i2c_adapter *adap,
> +			struct i2c_client *client, int read_write, u8 *val)

val should be const u8 *, as you do not modify the value (but see
below).

> +{
> +	int ret = -ENODEV;
> +
> +	if (adap->algo->master_xfer) {
> +		struct i2c_msg msg;
> +		char buf[1];
> +
> +		msg.addr = client->addr;
> +		msg.flags = (read_write == I2C_SMBUS_READ ? I2C_M_RD : 0);
> +		msg.len = 1;
> +		buf[0] = *val;
> +		msg.buf = buf;

As you passed val by address, you might point msg.buf to it directly,
without an intermediate buffer. If you want to use a buffer, then you
have no reason to pass val by address.

> +		ret = adap->algo->master_xfer(adap, &msg, 1);
> +	} else if (adap->algo->smbus_xfer) {

The underlying adapter must implement either method, so a simple "else"
will do.

> +		union i2c_smbus_data data;
> +		ret = adap->algo->smbus_xfer(adap, client->addr,
> +						client->flags & I2C_M_TEN,

You don't actually need to care about this flag, as the PCA954x doesn't
use 10-bit addressing.

> +						read_write,
> +						*val, I2C_SMBUS_BYTE, &data);
> +	}

As you only use the above function for writing, you could simplify it
by hard-coding read_write to 0. This function is for your driver only,
so there's no point it making it more complex than it needs to be. Then
you might consider renaming it to pca954x_reg_write().

The above is basically a reimplementation of i2c_smbus_write_byte()
bypassing the bus locking. The idea that many mux drivers will do that
kind of thing doesn't really please me. It's digging deep into the guts
of the i2c subsystem. It is very easy to get it wrong, and any further
change to the i2c-core may have to be duplicated in all mux drivers.
There's only one for now, but I presume there will be a lot more
someday.

It might be better to implement "nolock" variants of i2c_transfer() and
i2c_smbus_xfer(), and maybe some helper functions, so that mux drivers
can just call them. I seem to recall that this is what an earlier candidate
patch for I2C multiplexing support did. We can go with your approach
for now, as I don't want to delay the inclusion of your patches. I'll
look into this in parallel. It won't be lightweight for sure, so I have
to think about it.

In the meantime, I would appreciate a comment at the top of this
function, explaining what it does and why it is needed.

> +
> +	return ret;
> +}
> +
> +static int pca954x_select_chan(struct i2c_adapter *adap,
> +				void *client, u32 chan)
> +{
> +	struct pca954x *data = i2c_get_clientdata((struct i2c_client *) client);

Cast from void * isn't needed.

> +	struct chip_desc *chip = &chips[data->type];
> +	u8 regval = 0;

Useless initialization.

> +	int ret = 0;
> +
> +	/* Callback to check for bus connection */
> +	if (data->is_chan_connected) {
> +		ret = data->is_chan_connected(adap, client, chan);
> +		if (!ret)
> +			return -EBUSY;
> +	}

I am confused. What are you doing here? Which ports are used should be
determined at initialization time.

> +
> +	/* we make switches look like muxes, not sure how to be smarter */
> +	if (chip->muxtype == pca954x_ismux)
> +		regval = chan | chip->enable;
> +	else
> +		regval = 1 << chan;

Looks good to me. If we want to be smarter, then we need to let the
user pass channel aggregation information through platform data. This
would certainly be useful in some cases, for example for some Tyan
boards. But this can be implemented later.

> +
> +	/* Only select the channel if its different from the last channel */
> +	if (data->last_chan != chan) {
> +		ret = pca954x_xfer(adap, client, I2C_SMBUS_WRITE, &regval);
> +		data->last_chan = chan;
> +	}
> +
> +	return ret;
> +}
> +
> +static int pca954x_deselect_mux(struct i2c_adapter *adap,
> +				void *client, u32 chan)
> +{
> +	struct pca954x *data = i2c_get_clientdata((struct i2c_client *) client);

Cast not needed.

> +	struct chip_desc *chip = &chips[data->type];
> +	u8 regval = 0;

Useless initialization.

> +
> +	/* Check if we have to stay on the last channel we selected */
> +	if (!data->deselect_on_exit && (chip->muxtype == pca954x_ismux))

Test looks suspicious. I can't see why muxes and switches would handle
deselect_on_exit differently.

BTW, I think it would be better to set deselect to NULL if calling
deselect isn't desired. There's little point in calling a function
which will never do anything.

> +		return 0;
> +
> +	/* Signal we are deselecting active channel */
> +	data->last_chan = -1;

last_chan is a u8, it can't be set to -1.

> +
> +	regval = chan | ~chip->enable;

This looks wrong, basically it is setting all bits except the one in
chip->enable to 1. I doubt this is what you want. Also, this looks
valid only for mux chips, what about switches?

> +	return pca954x_xfer(adap, client, I2C_SMBUS_WRITE, &regval);
> +}
> +
> +/*
> + * I2C init/probing/exit functions
> + */
> +static int pca954x_probe(struct i2c_client *client,
> +				const struct i2c_device_id *id)

Missing __devinit.

> +{
> +	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> +	struct pca954x_platform_data *pdata = client->dev.platform_data;
> +	int i, n, f;

Please come up with better variable names. One-letter variables are a
pain to read and search for.

> +	struct pca954x *data;
> +	int ret = -ENODEV;
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
> +		goto err;
> +
> +	data = kzalloc(sizeof(struct pca954x), GFP_KERNEL);
> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +
> +	/* Read the mux register at addr.  This does two things: it verifies
> +	 * that the mux is in fact present, and fetches its current
> +	 * contents for possible use with a future deselect algorithm.
> +	 */
> +	i = i2c_smbus_read_byte(client);
> +	if (i < 0) {
> +		dev_warn(&client->dev, "probe failed\n");
> +		goto exit_free;
> +	}
> +
> +	data->type = id->driver_data;
> +	data->last_chan = -1;			/* force the first selection */

As I wrote above, -1 isn't a possible u8 value.

> +
> +	/* Now create virtual busses and adapters for them */

"busses and adapters" is redundant.

> +	for (i = 0; i < chips[data->type].nchans; i++) {
> +		f = 0;				/* dynamic adap number */
> +		if (pdata && (i < pdata->num_modes))

Looks wrong. If there are fewer "modes" provided than the device has
channels, then no adapters should be instantiated for the the remaining
channels. The loop should iterate over min(chips[data->type].nchans,
pdata->num_modes).

> +			f = pdata->modes[i].adap_id; /* force static number */
> +
> +		data->virt_adaps[i] = i2c_add_mux_adapter(adap, client, f, i,
> +						&pca954x_select_chan,
> +						&pca954x_deselect_mux);
> +		if (data->virt_adaps[i] == NULL) {
> +			ret = -ENODEV;

This certainly deserves an error message in the logs.

> +			goto virt_reg_failed;
> +		}
> +
> +		data->deselect_on_exit = pdata ?
> +				pdata->modes[i].deselect_on_exit : 1;

What was your rationale for defaulting to 1? Limiting the total
capacitance of the overall bus? As long as we only select one channel
at a time, I don't think this is likely to be an issue.

I would default to 0, for performance reasons. This is also more
consistent. Otherwise, not specifying platform data at all results in
deselect_on_exit = 1 while specifying platform data but not specifying
deselect_on_exit results in deselect_on_exit = 0. That's confusing.

> +		data->is_chan_connected = pdata ?
> +				pdata->modes[i].is_chan_connected : NULL;

This is racy. You need to set all attributes _before_ you register the
adapter.

> +	}
> +
> +	dev_info(&client->dev, "registered %d virtual busses for I2C %s %s\n",
> +			i, chips[data->type].muxtype == pca954x_ismux ?
> +					"mux" : "switch", client->name);
> +
> +	return 0;
> +
> +virt_reg_failed:
> +	for (n = 0; n < i; n++)
> +		i2c_del_mux_adapter(data->virt_adaps[n]);

You can instead decrease i from its current value down to 0. This way
you don't need to introduce another loop variable.

> +exit_free:
> +	kfree(data);
> +err:
> +	return ret;
> +}
> +
> +static int __devexit pca954x_remove(struct i2c_client *client)
> +{
> +	struct pca954x *data = i2c_get_clientdata(client);
> +	struct chip_desc *chip = &chips[data->type];
> +	int i, err;
> +
> +	for (i = 0; i < chip->nchans; ++i)
> +		if (data->virt_adaps[i]) {
> +			err = i2c_del_mux_adapter(data->virt_adaps[i]);
> +			if (err)
> +				return err;
> +			data->virt_adaps[i] = NULL;
> +		}
> +
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct i2c_driver pca954x_driver = {
> +	.driver = {
> +		.name	= "pca954x",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe	= pca954x_probe,
> +	.remove	= __devexit_p(pca954x_remove),
> +	.id_table = pca954x_id,
> +};
> +
> +static int __init pca954x_init(void)
> +{
> +	return i2c_add_driver(&pca954x_driver);
> +}
> +
> +static void __exit pca954x_exit(void)
> +{
> +	i2c_del_driver(&pca954x_driver);
> +}
> +
> +module_init(pca954x_init);
> +module_exit(pca954x_exit);
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>");
> +MODULE_DESCRIPTION("PCA954x I2C mux/switch driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/i2c/pca954x.h b/include/linux/i2c/pca954x.h
> new file mode 100755
> index 0000000..18d5e79
> --- /dev/null
> +++ b/include/linux/i2c/pca954x.h
> @@ -0,0 +1,13 @@

Please add the standard copyright and license comment. Please also add
standard #ifdef/#endif to protect against multiple inclusions.

> +/* Platform data for the PCA954x I2C multiplexers */
> +
> +struct pca954x_platform_mode {
> +	int		adap_id;
> +	unsigned int	deselect_on_exit:1;
> +	int		(*is_chan_connected)(struct i2c_adapter *adap,
> +					struct i2c_client *client, u32 chan);
> +};
> +
> +struct pca954x_platform_data {
> +	struct pca954x_platform_mode *modes;
> +	int num_modes;
> +};

These structure needs full description, as it is a public interface.

As I don't have any hardware to test your code, I will try adding a
special mode to i2c-stub where it would instantiate a virtual PCA9540
chip, and present 3 adapters to the user instead of 1. That way I
should be able to preform some testing on my own.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c: driver for PCA954x I2C multiplexer series.

[Jean Delvare]

Hi Michael,

On Fri, 16 Apr 2010 17:38:07 +0200, Jean Delvare wrote:
> As I don't have any hardware to test your code, I will try adding a
> special mode to i2c-stub where it would instantiate a virtual PCA9540
> chip, and present 3 adapters to the user instead of 1. That way I
> should be able to preform some testing on my own.

I've done this, and the tests results so far look good. If you can
submit updated patches addressing my concerns, they should be ready to
go in kernel 2.6.35. Further improvements and fixes can happen later.

Thanks,
-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Jean Delvare said the following:
> On Fri, 16 Apr 2010 15:10:11 +0200, Michael Lawnick wrote:
>> Jean Delvare said the following:
>> > One thing I forgot:
>> > 
>> >> > +		result = i2c_check_clients(to_i2c_adapter(adapter->dev.parent), addr);
>> >> > +	
>> >> > +	return result;
>> >> > +}
>> > 
>> > As discussed some weeks ago, this isn't actually sufficient. You don't
>> > only need to check the parent segments for address business, you also
>> > need to check all child segments, recursively. If any child segment has
>> > a device using the address in question, then you can't use it.
>> > 
>> > This may be more difficult to implement. In particular, you'll have to
>> > pay attention to locking.
>> > 
>> :-) This can't happen if we keep the part you commented on in the other
>> mail about probing for client one level above. Then this situation can't
>> arise.
> 
> I don't understand. In your code, the probe is done at the parent
> level, where address business had already been tested. What is needed
> is child segments checking, so the other side of the tree. I just can't
> see how your code would help with that.
> 
> Can you please explain why the probe is needed, and what it is doing
> that the standard address business check didn't cover already?
> 
Well, these are my thoughts:

The generic situation is

--- MUXn-1 --- MUXn -+- MUXn+1 ---
                     |
                client/device

A device gets physically visible on all buses beyond the one it is
connected to.

Given the bus tree we can decide whether a device is really present on a
particular bus if we
- H/W probe it on selected level. If does not respond, the current bus
is wrong.
- H/W probe it one level higher. If it responds, the current bus is wrong.
The first probing is already done in standard initialization sequence. I
added the second probing. By recursively calling __i2c_check_addr() it
is possible to reduce H/W-probing to ambiguous cases.

The current implementation implies that mux'es are reset to 'neutral'
after every bus transaction. If this would not be the case, switching of
MUXn+1 to neutral needed to be added.

-- 
KR
Michael

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Jean Delvare]

Hi Michael,

On Mon, 19 Apr 2010 11:29:27 +0200, Michael Lawnick wrote:
> Jean Delvare said the following:
> > On Fri, 16 Apr 2010 15:10:11 +0200, Michael Lawnick wrote:
> >> Jean Delvare said the following:
> >> > As discussed some weeks ago, this isn't actually sufficient. You don't
> >> > only need to check the parent segments for address business, you also
> >> > need to check all child segments, recursively. If any child segment has
> >> > a device using the address in question, then you can't use it.
> >> > 
> >> > This may be more difficult to implement. In particular, you'll have to
> >> > pay attention to locking.
> >>
> >> :-) This can't happen if we keep the part you commented on in the other
> >> mail about probing for client one level above. Then this situation can't
> >> arise.
> > 
> > I don't understand. In your code, the probe is done at the parent
> > level, where address business had already been tested. What is needed
> > is child segments checking, so the other side of the tree. I just can't
> > see how your code would help with that.
> > 
> > Can you please explain why the probe is needed, and what it is doing
> > that the standard address business check didn't cover already?
>
> Well, these are my thoughts:
> 
> The generic situation is
> 
> --- MUXn-1 --- MUXn -+- MUXn+1 ---
>                      |
>                 client/device
> 
> A device gets physically visible on all buses beyond the one it is
> connected to.

True.

> Given the bus tree we can decide whether a device is really present on a
> particular bus if we

Please note that we don't have to do this. Other code, in the kernel or
in user-space, is responsible for telling us where the devices are, and
we should trust them. Auto-detected devices is a purely optional
extension, the usage of which is discouraged as much as possible. My
opinion is that I2C device auto-detection should be simply prohibited
on multiplexes I2C buses (that is, such i2c_adapters would have their
class set to 0). I even would like to enforce this, by either
preventing the instantiation of a mux device on an i2c_adapter with a
non-0 class, or by resetting the class to 0, with a big warning in the
kernel logs, as soon as a mux device is instantiated.

As a matter of fact, your own code sets the adapter class to 0 for all
"virtual adapters" (which I call branches.)

> - H/W probe it on selected level. If does not respond, the current bus
> is wrong.

Or the device simply doesn't respond to the message format you used.
There is no universal probe transaction for I2C devices.

> - H/W probe it one level higher. If it responds, the current bus is wrong.

And if it doesn't respond, you can't conclude.

> The first probing is already done in standard initialization sequence. I

I'm not sure I really understand what you mean there, so please let me
repeat here: by default, no probing is done at the hardware level when
an I2C device (i2c_client) is instantiated. i2c_register_board_info()
and i2c_new_device() do not issue any transaction on the bus. Only
i2c_new_probed() device and i2c_driver's .detect() callbacks do, and
using these isn't recommended. The former is fine-grained enough that
it should work in combination with muxes, but the latter isn't.

> added the second probing. By recursively calling __i2c_check_addr() it
> is possible to reduce H/W-probing to ambiguous cases.

We don't want to do hardware probing at all to determine address
business. Look at the i2c-core code as it exists today: address
business is a solely software issue as far as i2c-core is concerned;
I see no good reason to change that just because we add support for
multiplexing.

So, in order to determine whether an address is busy, we have to check
all parent bus segments (which you already do) and all child bus
segments (recursively.) Please change your patch to do this.

> The current implementation implies that mux'es are reset to 'neutral'
> after every bus transaction. If this would not be the case, switching of
> MUXn+1 to neutral needed to be added.

As I said in my review, I don't think we want to switch all muxes back
to neutral after every transaction, because it is very costly for
I2C-based muxes (and your pca954x driver even offers a way to bypass
the reset.) There is also the problem of the initial state of the mux
chip, before we load its driver: we have no reason to believe it is in
neutral state. On top of that, not all muxes _can_ be switched to
neutral. Think of simple muxes controlled by a single GPIO, these have
always exactly one child branch selected. 

Add to this that I don't know of any mux chip which can be
auto-detected. You will always have to explicitly instantiate the mux
device, so you might as well explicitly instantiate all other devices
on that bus. We might extend i2c_new_device() to create multiple
devices at once (like i2c_register_board_info does) for this purpose.

I hope this clarified my point.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Multiplexed I2C bus core support.

[Michael Lawnick]

Jean Delvare said the following:
> We don't want to do hardware probing at all to determine address
> business. Look at the i2c-core code as it exists today: address
> business is a solely software issue as far as i2c-core is concerned;
> I see no good reason to change that just because we add support for
> multiplexing.
> 
> I hope this clarified my point.
> 
I think you were clear, given the unhappiness I feel :-(

I'll be back...

-- 
Kind Regards,
Michael