[Patch v3] i2c: Multiplexed I2C bus core support

[Patch v3] i2c: Multiplexed I2C bus core support

[Michael Lawnick]

Add multiplexed bus core support. I2C multiplexer and switches
like pca954x get instantiated as new adapters per port.

Signed-off-by: Michael Lawnick <demx1175@wbit01lx.(none)>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
Based on kernel 2.6.35.rc2 +
[PATCH] i2c: Move adapter locking helpers to i2c-core
by Jean Delware <20100615140218.07090c90-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
<http://article.gmane.org/gmane.linux.drivers.i2c/6226/match=i2c+move+adapter+locking+helpers+i2c+core>

 drivers/i2c/Kconfig         |   11 +++
 drivers/i2c/Makefile        |    1 +
 drivers/i2c/i2c-core.c      |   80 +++++++++++++++++----
 drivers/i2c/i2c-dev.c       |   44 +++++++++++-
 drivers/i2c/i2c-mux.c       |  165
+++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-mux.h     |   46 ++++++++++++
 include/linux/i2c.h         |    8 ++
 7 files changed, 341 insertions(+), 14 deletions(-)
 create mode 100755 drivers/i2c/i2c-mux.c
 create mode 100755 include/linux/i2c-mux.h

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index d06083f..efb48ad
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -47,6 +47,17 @@ config I2C_CHARDEV
 	  This support is also available as a module.  If so, the module
 	  will be called i2c-dev.

+config I2C_MUX
+	tristate "I2C bus multiplexing support"
+	depends on EXPERIMENTAL
+	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.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i2c-mux.
+
 config I2C_HELPER_AUTO
 	bool "Autoselect pertinent helper modules"
 	default y
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index a7d9b4b..f363258
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
 obj-$(CONFIG_I2C)		+= i2c-core.o
 obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
+obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
 obj-y				+= algos/ busses/

 ifeq ($(CONFIG_I2C_DEBUG_CORE),y)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 05a147a..d46fb3e
--- 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>
@@ -417,10 +419,52 @@ static int __i2c_check_addr_busy(struct device
*dev, void *addrp)
 	return 0;
 }

+/* walk up mux tree */
+static int i2c_check_mux_parents(struct i2c_adapter *adapter, int addr)
+{
+	int	result;
+
+	result = device_for_each_child(&adapter->dev, &addr,
+					__i2c_check_addr_busy);
+
+	if (!result && i2c_parent_is_i2c_adapter(adapter))
+		result = i2c_check_mux_parents(
+				    to_i2c_adapter(adapter->dev.parent), addr);
+
+	return result;
+}
+
+/* recurse down mux tree */
+static int i2c_check_mux_children(struct device *dev, void *addrp)
+{
+	int result;
+
+	if (dev->type == &i2c_adapter_type)
+		result = device_for_each_child(dev, addrp,
+						i2c_check_mux_children);
+	else
+		result = __i2c_check_addr_busy(dev, addrp);
+
+	return result;
+}
+
 static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
 {
-	return device_for_each_child(&adapter->dev, &addr,
-				     __i2c_check_addr_busy);
+	int	result = 0;
+
+	i2c_lock_adapter(adapter);
+
+	if (i2c_parent_is_i2c_adapter(adapter))
+		result = i2c_check_mux_parents(
+				    to_i2c_adapter(adapter->dev.parent), addr);
+
+	if (!result)
+		result = device_for_each_child(&adapter->dev, &addr,
+						i2c_check_mux_children);
+
+	i2c_unlock_adapter(adapter);
+
+	return result;
 }

 /**
@@ -429,7 +473,10 @@ static int i2c_check_addr_busy(struct i2c_adapter
*adapter, int addr)
  */
 void i2c_lock_adapter(struct i2c_adapter *adapter)
 {
-	rt_mutex_lock(&adapter->bus_lock);
+	if (i2c_parent_is_i2c_adapter(adapter))
+		i2c_lock_adapter(to_i2c_adapter(adapter->dev.parent));
+	else
+		rt_mutex_lock(&adapter->bus_lock);
 }
 EXPORT_SYMBOL_GPL(i2c_lock_adapter);

@@ -439,7 +486,10 @@ EXPORT_SYMBOL_GPL(i2c_lock_adapter);
  */
 static int i2c_trylock_adapter(struct i2c_adapter *adapter)
 {
-	return rt_mutex_trylock(&adapter->bus_lock);
+	if (i2c_parent_is_i2c_adapter(adapter))
+		return i2c_trylock_adapter(to_i2c_adapter(adapter->dev.parent));
+	else
+		return rt_mutex_trylock(&adapter->bus_lock);
 }

 /**
@@ -448,7 +498,10 @@ static int i2c_trylock_adapter(struct i2c_adapter
*adapter)
  */
 void i2c_unlock_adapter(struct i2c_adapter *adapter)
 {
-	rt_mutex_unlock(&adapter->bus_lock);
+	if (i2c_parent_is_i2c_adapter(adapter))
+		i2c_unlock_adapter(to_i2c_adapter(adapter->dev.parent));
+	else
+		rt_mutex_unlock(&adapter->bus_lock);
 }
 EXPORT_SYMBOL_GPL(i2c_unlock_adapter);

@@ -656,9 +709,9 @@ i2c_sysfs_new_device(struct device *dev, struct
device_attribute *attr,
 		return -EINVAL;

 	/* Keep track of the added device */
-	i2c_lock_adapter(adap);
+	rt_mutex_lock(&adap->bus_lock);
 	list_add_tail(&client->detected, &adap->userspace_clients);
-	i2c_unlock_adapter(adap);
+	rt_mutex_unlock(&adap->bus_lock);
 	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
 		 info.type, info.addr);

@@ -697,7 +750,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
device_attribute *attr,

 	/* Make sure the device was added through sysfs */
 	res = -ENOENT;
-	i2c_lock_adapter(adap);
+	rt_mutex_lock(&adap->bus_lock);
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		if (client->addr == addr) {
@@ -710,7 +763,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
device_attribute *attr,
 			break;
 		}
 	}
-	i2c_unlock_adapter(adap);
+	rt_mutex_unlock(&adap->bus_lock);

 	if (res < 0)
 		dev_err(dev, "%s: Can't find device in list\n",
@@ -737,10 +790,11 @@ static const struct attribute_group
*i2c_adapter_attr_groups[] = {
 	NULL
 };

-static struct device_type i2c_adapter_type = {
+struct device_type i2c_adapter_type = {
 	.groups		= i2c_adapter_attr_groups,
 	.release	= i2c_adapter_dev_release,
 };
+EXPORT_SYMBOL_GPL(i2c_adapter_type);

 #ifdef CONFIG_I2C_COMPAT
 static struct class_compat *i2c_adapter_compat_class;
@@ -995,7 +1049,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
 		return res;

 	/* Remove devices instantiated from sysfs */
-	i2c_lock_adapter(adap);
+	rt_mutex_lock(&adap->bus_lock);
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,
@@ -1003,7 +1057,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
 		list_del(&client->detected);
 		i2c_unregister_device(client);
 	}
-	i2c_unlock_adapter(adap);
+	rt_mutex_unlock(&adap->bus_lock);

 	/* Detach any active clients. This can't fail, thus we do not
 	   checking the returned value. */
diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index e0694e4..6717bba
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -193,12 +193,54 @@ static int i2cdev_check(struct device *dev, void
*addrp)
 	return dev->driver ? -EBUSY : 0;
 }

+/* walk up mux tree */
+static int i2cdev_check_mux_parents(struct i2c_adapter *adapter, int addr)
+{
+	int	result;
+
+	result = device_for_each_child(&adapter->dev, &addr, i2cdev_check);
+
+	if (!result && i2c_parent_is_i2c_adapter(adapter))
+		result = i2cdev_check_mux_parents(
+				    to_i2c_adapter(adapter->dev.parent), addr);
+
+	return result;
+}
+
+/* recurse down mux tree */
+static int i2cdev_check_mux_children(struct device *dev, void *addrp)
+{
+	int result;
+
+	if (dev->type == &i2c_adapter_type)
+		result = device_for_each_child(dev, addrp,
+						i2cdev_check_mux_children);
+	else
+		result = i2cdev_check(dev, addrp);
+
+	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;
+
+	i2c_lock_adapter(adapter);
+
+	if (i2c_parent_is_i2c_adapter(adapter))
+		result = i2cdev_check_mux_parents(
+				    to_i2c_adapter(adapter->dev.parent), addr);
+
+	if (!result)
+		result = device_for_each_child(&adapter->dev, &addr,
+						i2cdev_check_mux_children);
+
+	i2c_unlock_adapter(adapter);
+
+	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..d32a484
--- /dev/null
+++ b/drivers/i2c/i2c-mux.c
@@ -0,0 +1,165 @@
+/*
+ * 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>
+ * Copyright (c) 2009-2010 NSN GmbH & Co KG <michael.lawnick.ext@nsn.com>
+ *
+ * Simplifies access to complex multiplexed I2C bus topologies, by
presenting
+ * each multiplexed bus segment as an additional 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 Ken Harrenstien, Copyright (c) 2004 Google, Inc.
+ *	i2c-virtual.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
+ *
+ * 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/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+
+/* multiplexer per channel data */
+struct i2c_mux_priv {
+	struct i2c_adapter adap;
+	struct i2c_algorithm algo;
+
+	struct i2c_adapter *parent;
+	void *mux_dev;	/* the mux chip/device */
+	u32  chan_id;	/* the channel id */
+
+	int (*select)(struct i2c_adapter *, void *mux_dev, u32 chan_id);
+	int (*deselect)(struct i2c_adapter *, void *mux_dev, u32 chan_id);
+};
+
+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;
+	int ret;
+
+	/* Switch to the right mux port and perform the transfer. */
+
+	ret = priv->select(parent, priv->mux_dev, priv->chan_id);
+	if (ret >= 0)
+		ret = parent->algo->master_xfer(parent, msgs, num);
+	if (priv->deselect)
+		priv->deselect(parent, priv->mux_dev, priv->chan_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;
+	int ret;
+
+	/* Select the right mux port and perform the transfer. */
+
+	ret = priv->select(parent, priv->mux_dev, priv->chan_id);
+	if (ret >= 0)
+		ret = parent->algo->smbus_xfer(parent, addr, flags,
+					read_write, command, size, data);
+	if (priv->deselect)
+		priv->deselect(parent, priv->mux_dev, priv->chan_id);
+
+	return ret;
+}
+
+/* Return the parent's functionality */
+static u32 i2c_mux_functionality(struct i2c_adapter *adap)
+{
+	struct i2c_mux_priv *priv = adap->algo_data;
+	struct i2c_adapter *parent = priv->parent;
+
+	return parent->algo->functionality(parent);
+}
+
+struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
+				void *mux_dev, u32 force_nr, u32 chan_id,
+				int (*select) (struct i2c_adapter *,
+					       void *, u32),
+				int (*deselect) (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 = parent;
+	priv->mux_dev = mux_dev;
+	priv->chan_id = chan_id;
+	priv->select = select;
+	priv->deselect = deselect;
+
+	/* 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 (chan_id %d)", i2c_adapter_id(parent), chan_id);
+	priv->adap.owner = THIS_MODULE;
+	priv->adap.id = parent->id;
+	priv->adap.algo = &priv->algo;
+	priv->adap.algo_data = priv;
+	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) {
+		dev_err(&parent->dev,
+			"failed to add mux-adapter (error=%d)\n",
+			ret);
+		kfree(priv);
+		return NULL;
+	}
+
+	dev_info(&parent->dev, "Added multiplexed i2c bus %d\n",
+		 i2c_adapter_id(&priv->adap));
+
+	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("I2C driver for multiplexed I2C busses");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
new file mode 100755
index 0000000..4c52ee1
--- /dev/null
+++ b/include/linux/i2c-mux.h
@@ -0,0 +1,46 @@
+/*
+ *
+ * i2c-mux.h - functions for the i2c-bus mux support
+ *
+ * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
+ * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
+ * Michael Lawnick <michael.lawnick.ext-OYasijW0DpE@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _LINUX_I2C_MUX_H
+#define _LINUX_I2C_MUX_H
+
+#ifdef __KERNEL__
+
+/*
+ * Called to create a i2c bus on a multiplexed bus segment.
+ * The mux_dev and chan_id parameters 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 *mux_dev, u32 force_nr, u32 chan_id,
+				int (*select) (struct i2c_adapter *adap,
+				void *mux_dev, u32 chan_id),
+				int (*deselect) (struct i2c_adapter *,
+				void *mux_dev, u32 chan_id));
+
+int i2c_del_mux_adapter(struct i2c_adapter *adap);
+
+#endif /* __KERNEL__ */
+
+#endif /* _LINUX_I2C_MUX_H */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f2d36bd..8f2b83a
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -37,6 +37,7 @@
 #include <linux/of.h>		/* for struct device_node */

 extern struct bus_type i2c_bus_type;
+extern struct device_type i2c_adapter_type;

 /* --- General options ------------------------------------------------	*/

@@ -374,6 +375,13 @@ static inline void i2c_set_adapdata(struct
i2c_adapter *dev, void *data)
 	dev_set_drvdata(&dev->dev, data);
 }

+static inline int i2c_parent_is_i2c_adapter(const struct i2c_adapter
*adapter)
+{
+	return adapter->dev.parent != NULL
+		&& adapter->dev.parent->bus == &i2c_bus_type
+		&& adapter->dev.parent->type == &i2c_adapter_type;
+}
+
 /* Adapter locking functions, exported for shared pin cases */
 void i2c_lock_adapter(struct i2c_adapter *);
 void i2c_unlock_adapter(struct i2c_adapter *);
-- 
Michael Lawnick

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Jean Delvare]

Hi Michael,

> Add multiplexed bus core support. I2C multiplexer and switches
> like pca954x get instantiated as new adapters per port.
> 
> Signed-off-by: Michael Lawnick <demx1175@wbit01lx.(none)>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
> Based on kernel 2.6.35.rc2 +
> [PATCH] i2c: Move adapter locking helpers to i2c-core
> by Jean Delware <20100615140218.07090c90-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
> <http://article.gmane.org/gmane.linux.drivers.i2c/6226/match=i2c+move+adapter+locking+helpers+i2c+core>
> 
>  drivers/i2c/Kconfig         |   11 +++
>  drivers/i2c/Makefile        |    1 +
>  drivers/i2c/i2c-core.c      |   80 +++++++++++++++++----
>  drivers/i2c/i2c-dev.c       |   44 +++++++++++-
>  drivers/i2c/i2c-mux.c       |  165
> +++++++++++++++++++++++++++++++++++++++++++

As you can see, your e-mail client wrapped long lines again. I had to
manually fix the patch so that I could apply it...

>  include/linux/i2c-mux.h     |   46 ++++++++++++
>  include/linux/i2c.h         |    8 ++
>  7 files changed, 341 insertions(+), 14 deletions(-)
>  create mode 100755 drivers/i2c/i2c-mux.c
>  create mode 100755 include/linux/i2c-mux.h

A few comments below, just FYI. Nothing for you to do, the rest is up
to me I think. Thanks :)

> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index d06083f..efb48ad
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -47,6 +47,17 @@ config I2C_CHARDEV
>  	  This support is also available as a module.  If so, the module
>  	  will be called i2c-dev.
> 
> +config I2C_MUX
> +	tristate "I2C bus multiplexing support"
> +	depends on EXPERIMENTAL
> +	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.
> +
> +	  This support is also available as a module.  If so, the module
> +	  will be called i2c-mux.
> +
>  config I2C_HELPER_AUTO
>  	bool "Autoselect pertinent helper modules"
>  	default y
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index a7d9b4b..f363258
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
>  obj-$(CONFIG_I2C)		+= i2c-core.o
>  obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
>  obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
> +obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
>  obj-y				+= algos/ busses/
> 
>  ifeq ($(CONFIG_I2C_DEBUG_CORE),y)
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 05a147a..d46fb3e
> --- 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> */
> 
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> @@ -417,10 +419,52 @@ static int __i2c_check_addr_busy(struct device
> *dev, void *addrp)
>  	return 0;
>  }
> 
> +/* walk up mux tree */
> +static int i2c_check_mux_parents(struct i2c_adapter *adapter, int addr)
> +{
> +	int	result;
> +
> +	result = device_for_each_child(&adapter->dev, &addr,
> +					__i2c_check_addr_busy);
> +
> +	if (!result && i2c_parent_is_i2c_adapter(adapter))
> +		result = i2c_check_mux_parents(
> +				    to_i2c_adapter(adapter->dev.parent), addr);
> +
> +	return result;
> +}
> +
> +/* recurse down mux tree */
> +static int i2c_check_mux_children(struct device *dev, void *addrp)
> +{
> +	int result;
> +
> +	if (dev->type == &i2c_adapter_type)
> +		result = device_for_each_child(dev, addrp,
> +						i2c_check_mux_children);
> +	else
> +		result = __i2c_check_addr_busy(dev, addrp);
> +
> +	return result;
> +}
> +
>  static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
>  {
> -	return device_for_each_child(&adapter->dev, &addr,
> -				     __i2c_check_addr_busy);
> +	int	result = 0;
> +
> +	i2c_lock_adapter(adapter);
> +
> +	if (i2c_parent_is_i2c_adapter(adapter))
> +		result = i2c_check_mux_parents(
> +				    to_i2c_adapter(adapter->dev.parent), addr);
> +
> +	if (!result)
> +		result = device_for_each_child(&adapter->dev, &addr,
> +						i2c_check_mux_children);
> +
> +	i2c_unlock_adapter(adapter);

I'm not convinced by your locking model. Surrounding only this function
with i2c_lock/unlock_adapter() doesn't really help anything, does it?
As soon as the function returns, anyone can enter the check section
again, while the actual device has not yet been registered and will
thus not be found by device_for_each_child().

I believe that the lock should be taken, and released, in the calling
function, that is in i2c_new_device(). The lock should be taken right
before calling i2c_check_addr_busy(), and released only after
device_register() has been called.

For the other calling sites (i2c_detect_address and
i2c_new_probed_device), I'm not even sure we need to take the lock
at all. I am told that the low-level driver core code should be able
to cope with device_remove() being called in the middle of
device_for_each_child(). My reading of the low-level code couldn't
confirm this, but I am no expert of that part of the kernel.

I'm also unsure if the adapter's lock is the right lock to use. This
lock is supposed to be used to serialize I/O on the adapter. There's
no reason to stop I/O on the adapter just because a new client is
being registered. And thinking about it some more, it could even
deadlock, is the driver's probe() function does I/O (and it often
does.) The core i2c lock may be suitable, even though I admit it is
an abuse too, as the problem we are trying to solve is really local
to the specific adapter tree and not subsystem-wide (but OTOH, the
i2c core lock is used infrequently enough that it shouldn't be a
problem in practice.)

Anyway, I didn't expect you to solve this problem anyway. It was
there before you added mux support, so it's not your problem ;)

> +
> +	return result;
>  }
> 
>  /**
> @@ -429,7 +473,10 @@ static int i2c_check_addr_busy(struct i2c_adapter
> *adapter, int addr)
>   */
>  void i2c_lock_adapter(struct i2c_adapter *adapter)
>  {
> -	rt_mutex_lock(&adapter->bus_lock);
> +	if (i2c_parent_is_i2c_adapter(adapter))
> +		i2c_lock_adapter(to_i2c_adapter(adapter->dev.parent));
> +	else
> +		rt_mutex_lock(&adapter->bus_lock);
>  }
>  EXPORT_SYMBOL_GPL(i2c_lock_adapter);
> 
> @@ -439,7 +486,10 @@ EXPORT_SYMBOL_GPL(i2c_lock_adapter);
>   */
>  static int i2c_trylock_adapter(struct i2c_adapter *adapter)
>  {
> -	return rt_mutex_trylock(&adapter->bus_lock);
> +	if (i2c_parent_is_i2c_adapter(adapter))
> +		return i2c_trylock_adapter(to_i2c_adapter(adapter->dev.parent));
> +	else
> +		return rt_mutex_trylock(&adapter->bus_lock);
>  }
> 
>  /**
> @@ -448,7 +498,10 @@ static int i2c_trylock_adapter(struct i2c_adapter
> *adapter)
>   */
>  void i2c_unlock_adapter(struct i2c_adapter *adapter)
>  {
> -	rt_mutex_unlock(&adapter->bus_lock);
> +	if (i2c_parent_is_i2c_adapter(adapter))
> +		i2c_unlock_adapter(to_i2c_adapter(adapter->dev.parent));
> +	else
> +		rt_mutex_unlock(&adapter->bus_lock);
>  }
>  EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
> 
> @@ -656,9 +709,9 @@ i2c_sysfs_new_device(struct device *dev, struct
> device_attribute *attr,
>  		return -EINVAL;
> 
>  	/* Keep track of the added device */
> -	i2c_lock_adapter(adap);
> +	rt_mutex_lock(&adap->bus_lock);
>  	list_add_tail(&client->detected, &adap->userspace_clients);
> -	i2c_unlock_adapter(adap);
> +	rt_mutex_unlock(&adap->bus_lock);
>  	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
>  		 info.type, info.addr);
> 

I expected us to just use i2c_lock/unlock_adapter() everywhere for
simplicity. Now I have to agree that using the segment's mutex works
too, as the operation is both local to the mutex and unrelated to
the other use cases of i2c_lock/unlock_adapter(). But it becomes a
little tricky, so it should be all documented clearly (which I will
do, don't worry.)

> @@ -697,7 +750,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
> device_attribute *attr,
> 
>  	/* Make sure the device was added through sysfs */
>  	res = -ENOENT;
> -	i2c_lock_adapter(adap);
> +	rt_mutex_lock(&adap->bus_lock);
>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>  				 detected) {
>  		if (client->addr == addr) {
> @@ -710,7 +763,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
> device_attribute *attr,
>  			break;
>  		}
>  	}
> -	i2c_unlock_adapter(adap);
> +	rt_mutex_unlock(&adap->bus_lock);
> 
>  	if (res < 0)
>  		dev_err(dev, "%s: Can't find device in list\n",
> @@ -737,10 +790,11 @@ static const struct attribute_group
> *i2c_adapter_attr_groups[] = {
>  	NULL
>  };
> 
> -static struct device_type i2c_adapter_type = {
> +struct device_type i2c_adapter_type = {
>  	.groups		= i2c_adapter_attr_groups,
>  	.release	= i2c_adapter_dev_release,
>  };
> +EXPORT_SYMBOL_GPL(i2c_adapter_type);
> 
>  #ifdef CONFIG_I2C_COMPAT
>  static struct class_compat *i2c_adapter_compat_class;
> @@ -995,7 +1049,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
>  		return res;
> 
>  	/* Remove devices instantiated from sysfs */
> -	i2c_lock_adapter(adap);
> +	rt_mutex_lock(&adap->bus_lock);
>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>  				 detected) {
>  		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,
> @@ -1003,7 +1057,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
>  		list_del(&client->detected);
>  		i2c_unregister_device(client);
>  	}
> -	i2c_unlock_adapter(adap);
> +	rt_mutex_unlock(&adap->bus_lock);
> 
>  	/* Detach any active clients. This can't fail, thus we do not
>  	   checking the returned value. */
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index e0694e4..6717bba
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -193,12 +193,54 @@ static int i2cdev_check(struct device *dev, void
> *addrp)
>  	return dev->driver ? -EBUSY : 0;
>  }
> 
> +/* walk up mux tree */
> +static int i2cdev_check_mux_parents(struct i2c_adapter *adapter, int addr)
> +{
> +	int	result;
> +
> +	result = device_for_each_child(&adapter->dev, &addr, i2cdev_check);
> +
> +	if (!result && i2c_parent_is_i2c_adapter(adapter))
> +		result = i2cdev_check_mux_parents(
> +				    to_i2c_adapter(adapter->dev.parent), addr);
> +
> +	return result;
> +}
> +
> +/* recurse down mux tree */
> +static int i2cdev_check_mux_children(struct device *dev, void *addrp)
> +{
> +	int result;
> +
> +	if (dev->type == &i2c_adapter_type)
> +		result = device_for_each_child(dev, addrp,
> +						i2cdev_check_mux_children);
> +	else
> +		result = i2cdev_check(dev, addrp);
> +
> +	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;
> +
> +	i2c_lock_adapter(adapter);
> +
> +	if (i2c_parent_is_i2c_adapter(adapter))
> +		result = i2cdev_check_mux_parents(
> +				    to_i2c_adapter(adapter->dev.parent), addr);
> +
> +	if (!result)
> +		result = device_for_each_child(&adapter->dev, &addr,
> +						i2cdev_check_mux_children);
> +
> +	i2c_unlock_adapter(adapter);
> +
> +	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..d32a484
> --- /dev/null
> +++ b/drivers/i2c/i2c-mux.c
> @@ -0,0 +1,165 @@
> +/*
> + * 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>
> + * Copyright (c) 2009-2010 NSN GmbH & Co KG <michael.lawnick.ext@nsn.com>
> + *
> + * Simplifies access to complex multiplexed I2C bus topologies, by
> presenting
> + * each multiplexed bus segment as an additional 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 Ken Harrenstien, Copyright (c) 2004 Google, Inc.
> + *	i2c-virtual.c from Brian Kuschak <bkuschak-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> + *
> + * 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/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +
> +/* multiplexer per channel data */
> +struct i2c_mux_priv {
> +	struct i2c_adapter adap;
> +	struct i2c_algorithm algo;
> +
> +	struct i2c_adapter *parent;
> +	void *mux_dev;	/* the mux chip/device */
> +	u32  chan_id;	/* the channel id */
> +
> +	int (*select)(struct i2c_adapter *, void *mux_dev, u32 chan_id);
> +	int (*deselect)(struct i2c_adapter *, void *mux_dev, u32 chan_id);
> +};
> +
> +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;
> +	int ret;
> +
> +	/* Switch to the right mux port and perform the transfer. */
> +
> +	ret = priv->select(parent, priv->mux_dev, priv->chan_id);
> +	if (ret >= 0)
> +		ret = parent->algo->master_xfer(parent, msgs, num);
> +	if (priv->deselect)
> +		priv->deselect(parent, priv->mux_dev, priv->chan_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;
> +	int ret;
> +
> +	/* Select the right mux port and perform the transfer. */
> +
> +	ret = priv->select(parent, priv->mux_dev, priv->chan_id);
> +	if (ret >= 0)
> +		ret = parent->algo->smbus_xfer(parent, addr, flags,
> +					read_write, command, size, data);
> +	if (priv->deselect)
> +		priv->deselect(parent, priv->mux_dev, priv->chan_id);
> +
> +	return ret;
> +}
> +
> +/* Return the parent's functionality */
> +static u32 i2c_mux_functionality(struct i2c_adapter *adap)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	struct i2c_adapter *parent = priv->parent;
> +
> +	return parent->algo->functionality(parent);
> +}
> +
> +struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
> +				void *mux_dev, u32 force_nr, u32 chan_id,
> +				int (*select) (struct i2c_adapter *,
> +					       void *, u32),
> +				int (*deselect) (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 = parent;
> +	priv->mux_dev = mux_dev;
> +	priv->chan_id = chan_id;
> +	priv->select = select;
> +	priv->deselect = deselect;
> +
> +	/* 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 (chan_id %d)", i2c_adapter_id(parent), chan_id);
> +	priv->adap.owner = THIS_MODULE;
> +	priv->adap.id = parent->id;
> +	priv->adap.algo = &priv->algo;
> +	priv->adap.algo_data = priv;
> +	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) {
> +		dev_err(&parent->dev,
> +			"failed to add mux-adapter (error=%d)\n",
> +			ret);
> +		kfree(priv);
> +		return NULL;
> +	}
> +
> +	dev_info(&parent->dev, "Added multiplexed i2c bus %d\n",
> +		 i2c_adapter_id(&priv->adap));
> +
> +	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("I2C driver for multiplexed I2C busses");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
> new file mode 100755
> index 0000000..4c52ee1
> --- /dev/null
> +++ b/include/linux/i2c-mux.h
> @@ -0,0 +1,46 @@
> +/*
> + *
> + * i2c-mux.h - functions for the i2c-bus mux support
> + *
> + * Copyright (c) 2008-2009 Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> + * Copyright (c) 2008-2009 Eurotech S.p.A. <info-ymFC7zkAqBI1GQ1Ptb7lUw@public.gmane.org>
> + * Michael Lawnick <michael.lawnick.ext-OYasijW0DpE@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#ifndef _LINUX_I2C_MUX_H
> +#define _LINUX_I2C_MUX_H
> +
> +#ifdef __KERNEL__
> +
> +/*
> + * Called to create a i2c bus on a multiplexed bus segment.
> + * The mux_dev and chan_id parameters 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 *mux_dev, u32 force_nr, u32 chan_id,
> +				int (*select) (struct i2c_adapter *adap,
> +				void *mux_dev, u32 chan_id),
> +				int (*deselect) (struct i2c_adapter *,
> +				void *mux_dev, u32 chan_id));
> +
> +int i2c_del_mux_adapter(struct i2c_adapter *adap);
> +
> +#endif /* __KERNEL__ */
> +
> +#endif /* _LINUX_I2C_MUX_H */
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index f2d36bd..8f2b83a
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -37,6 +37,7 @@
>  #include <linux/of.h>		/* for struct device_node */
> 
>  extern struct bus_type i2c_bus_type;
> +extern struct device_type i2c_adapter_type;
> 
>  /* --- General options ------------------------------------------------	*/
> 
> @@ -374,6 +375,13 @@ static inline void i2c_set_adapdata(struct
> i2c_adapter *dev, void *data)
>  	dev_set_drvdata(&dev->dev, data);
>  }
> 
> +static inline int i2c_parent_is_i2c_adapter(const struct i2c_adapter
> *adapter)
> +{
> +	return adapter->dev.parent != NULL
> +		&& adapter->dev.parent->bus == &i2c_bus_type
> +		&& adapter->dev.parent->type == &i2c_adapter_type;
> +}
> +
>  /* Adapter locking functions, exported for shared pin cases */
>  void i2c_lock_adapter(struct i2c_adapter *);
>  void i2c_unlock_adapter(struct i2c_adapter *);

-- 
Jean Delvare

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Michael Lawnick]

Jean Delvare said the following:
> Hi Michael,
> 
>>  drivers/i2c/i2c-mux.c       |  165
>> > +++++++++++++++++++++++++++++++++++++++++++
> As you can see, your e-mail client wrapped long lines again. I had to
> manually fix the patch so that I could apply it...
> 
I'm really sorry about that :-(
I've tested it by sending to an alternative address and it worked.
No idea where this wrap comes from.
...
>> @@ -656,9 +709,9 @@ i2c_sysfs_new_device(struct device *dev, struct
>> device_attribute *attr,
>>  		return -EINVAL;
>> 
>>  	/* Keep track of the added device */
>> -	i2c_lock_adapter(adap);
>> +	rt_mutex_lock(&adap->bus_lock);
>>  	list_add_tail(&client->detected, &adap->userspace_clients);
>> -	i2c_unlock_adapter(adap);
>> +	rt_mutex_unlock(&adap->bus_lock);
>>  	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
>>  		 info.type, info.addr);
>> 
> 
> I expected us to just use i2c_lock/unlock_adapter() everywhere for
> simplicity. Now I have to agree that using the segment's mutex works
> too, as the operation is both local to the mutex and unrelated to
> the other use cases of i2c_lock/unlock_adapter(). But it becomes a
> little tricky, so it should be all documented clearly (which I will
> do, don't worry.)
> 
>> @@ -697,7 +750,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
>> device_attribute *attr,
>> 
>>  	/* Make sure the device was added through sysfs */
>>  	res = -ENOENT;
>> -	i2c_lock_adapter(adap);
>> +	rt_mutex_lock(&adap->bus_lock);
>>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>>  				 detected) {
>>  		if (client->addr == addr) {
>> @@ -710,7 +763,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
>> device_attribute *attr,
>>  			break;
>>  		}
>>  	}
>> -	i2c_unlock_adapter(adap);
>> +	rt_mutex_unlock(&adap->bus_lock);
>> 
>>  	if (res < 0)
>>  		dev_err(dev, "%s: Can't find device in list\n",

In i2c_sysfs_delete_device you need a local lock, otherwise you'll get
a deadlock on removing sub-clients/tree. This in turn brings the local
lock to i2c_sysfs_new_device().

Thank you for your review.
ToDo after release: Inventing mux-adapter-name definition in mux-client
code instead of mux-code. You were right in your last review, this is
terribly necessary.

-- 
KR
Michael Lawnick

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Jean Delvare]

On Tue, 20 Jul 2010 08:44:23 +0200, Michael Lawnick wrote:
> Jean Delvare said the following:
> >> @@ -656,9 +709,9 @@ i2c_sysfs_new_device(struct device *dev, struct
> >> device_attribute *attr,
> >>  		return -EINVAL;
> >> 
> >>  	/* Keep track of the added device */
> >> -	i2c_lock_adapter(adap);
> >> +	rt_mutex_lock(&adap->bus_lock);
> >>  	list_add_tail(&client->detected, &adap->userspace_clients);
> >> -	i2c_unlock_adapter(adap);
> >> +	rt_mutex_unlock(&adap->bus_lock);
> >>  	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
> >>  		 info.type, info.addr);
> >> 
> > 
> > I expected us to just use i2c_lock/unlock_adapter() everywhere for
> > simplicity. Now I have to agree that using the segment's mutex works
> > too, as the operation is both local to the mutex and unrelated to
> > the other use cases of i2c_lock/unlock_adapter(). But it becomes a
> > little tricky, so it should be all documented clearly (which I will
> > do, don't worry.)
> > 
> >> @@ -697,7 +750,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
> >> device_attribute *attr,
> >> 
> >>  	/* Make sure the device was added through sysfs */
> >>  	res = -ENOENT;
> >> -	i2c_lock_adapter(adap);
> >> +	rt_mutex_lock(&adap->bus_lock);
> >>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
> >>  				 detected) {
> >>  		if (client->addr == addr) {
> >> @@ -710,7 +763,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
> >> device_attribute *attr,
> >>  			break;
> >>  		}
> >>  	}
> >> -	i2c_unlock_adapter(adap);
> >> +	rt_mutex_unlock(&adap->bus_lock);
> >> 
> >>  	if (res < 0)
> >>  		dev_err(dev, "%s: Can't find device in list\n",
> 
> In i2c_sysfs_delete_device you need a local lock, otherwise you'll get
> a deadlock on removing sub-clients/tree. This in turn brings the local
> lock to i2c_sysfs_new_device().

This is only relevant if the device instantiated / removed from
user-space is an I2C mux chip, right?

Please remember that i2c_lock_adapter() and rt_mutex_lock() might do
exactly the same, if applied to the root segment of an I2C tree. So if
i2c_lock_adapter() would deadlock, I fear that a simple rt_mutex_lock()
might deadlock too. So in the end we might have to introduce another
mutex dedicated to protecting the adapter->userspace_clients list.
Maybe we should have done this from the beginning...

> Thank you for your review.
> ToDo after release: Inventing mux-adapter-name definition in mux-client
> code instead of mux-code. You were right in your last review, this is
> terribly necessary.

OK.

-- 
Jean Delvare

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Michael Lawnick]

Jean Delvare said the following:
> On Tue, 20 Jul 2010 08:44:23 +0200, Michael Lawnick wrote:
>> Jean Delvare said the following:
>> >> @@ -697,7 +750,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
>> >> device_attribute *attr,
>> >>
>> >>  	/* Make sure the device was added through sysfs */
>> >>  	res = -ENOENT;
>> >> -	i2c_lock_adapter(adap);
>> >> +	rt_mutex_lock(&adap->bus_lock);
>> >>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>> >>  				 detected) {
>> >>  		if (client->addr == addr) {
>> >> @@ -710,7 +763,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
>> >> device_attribute *attr,
>> >>  			break;
>> >>  		}
>> >>  	}
>> >> -	i2c_unlock_adapter(adap);
>> >> +	rt_mutex_unlock(&adap->bus_lock);
>> >>
>> >>  	if (res < 0)
>> >>  		dev_err(dev, "%s: Can't find device in list\n",
>>
>> In i2c_sysfs_delete_device you need a local lock, otherwise you'll get
>> a deadlock on removing sub-clients/tree. This in turn brings the local
>> lock to i2c_sysfs_new_device().
>
> This is only relevant if the device instantiated / removed from
> user-space is an I2C mux chip, right?
>
> Please remember that i2c_lock_adapter() and rt_mutex_lock() might do
> exactly the same, if applied to the root segment of an I2C tree. So if
> i2c_lock_adapter() would deadlock, I fear that a simple rt_mutex_lock()
> might deadlock too. So in the end we might have to introduce another
> mutex dedicated to protecting the adapter->userspace_clients list.
> Maybe we should have done this from the beginning...
>

Nearly missed this. Have not yet looked into your ftp link, so don't
know whether it is still relevant, but anyway:
The difference above is that rt_mutex_lock locks the (mux-)adapter only
while i2c_lock_adapter locks the root-adapter. So if a parent mux is
unloaded there is no conflict with children.

-- 
Michael Lawnick

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Jean Delvare]

Hi Michael,

On Fri, 09 Jul 2010 11:07:46 +0200, Michael Lawnick wrote:
> Add multiplexed bus core support. I2C multiplexer and switches
> like pca954x get instantiated as new adapters per port.
> 
> Signed-off-by: Michael Lawnick <demx1175@wbit01lx.(none)>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
> Based on kernel 2.6.35.rc2 +
> [PATCH] i2c: Move adapter locking helpers to i2c-core
> by Jean Delware <20100615140218.07090c90-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
> <http://article.gmane.org/gmane.linux.drivers.i2c/6226/match=i2c+move+adapter+locking+helpers+i2c+core>
> 
>  drivers/i2c/Kconfig         |   11 +++
>  drivers/i2c/Makefile        |    1 +
>  drivers/i2c/i2c-core.c      |   80 +++++++++++++++++----
>  drivers/i2c/i2c-dev.c       |   44 +++++++++++-
>  drivers/i2c/i2c-mux.c       |  165 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-mux.h     |   46 ++++++++++++
>  include/linux/i2c.h         |    8 ++
>  7 files changed, 341 insertions(+), 14 deletions(-)
>  create mode 100755 drivers/i2c/i2c-mux.c
>  create mode 100755 include/linux/i2c-mux.h

Patch applied, with two changes:
* Reverted changes to i2c_sysfs_new_device(), i2c_sysfs_delete_device()
and i2c_del_adapter() - no longer needed after I added a dedicated
mutex for userspace client lists.
* Removed locking in i2c_check_addr_busy() and i2cdev_check_addr(). It
was insufficient anyway, I'll think of something better and add it
later.

Rodoflo, I've added your Acked-by, I hope this is fine.

Resulting patch can be seen here:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-multiplexing-core-support.patch

Thanks again for your contribution and patience :)

-- 
Jean Delvare

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Rodolfo Giometti]

On Tue, Jul 20, 2010 at 09:27:29PM +0200, Jean Delvare wrote:
> Hi Michael,
> 
> Patch applied, with two changes:
> * Reverted changes to i2c_sysfs_new_device(), i2c_sysfs_delete_device()
> and i2c_del_adapter() - no longer needed after I added a dedicated
> mutex for userspace client lists.
> * Removed locking in i2c_check_addr_busy() and i2cdev_check_addr(). It
> was insufficient anyway, I'll think of something better and add it
> later.

Great! :)

> Rodoflo, I've added your Acked-by, I hope this is fine.

It's ok for me.

> Resulting patch can be seen here:
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-multiplexing-core-support.patch
> 
> Thanks again for your contribution and patience :)

Michael, maybe you wish updating the linux-i2c wiki page regarding
this issue?

   https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Jean Delvare]

On Thu, 22 Jul 2010 08:42:43 +0200, Rodolfo Giometti wrote:
> Michael, maybe you wish updating the linux-i2c wiki page regarding
> this issue?
> 
>    https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing

Or maybe you could do it, Rodolfo? Michael is apparently busy and so am
I.

Thanks,
-- 
Jean Delvare

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Rodolfo Giometti]

On Tue, Aug 10, 2010 at 02:33:35PM +0200, Jean Delvare wrote:
> On Thu, 22 Jul 2010 08:42:43 +0200, Rodolfo Giometti wrote:
> > Michael, maybe you wish updating the linux-i2c wiki page regarding
> > this issue?
> > 
> >    https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing
> 
> Or maybe you could do it, Rodolfo? Michael is apparently busy and so am
> I.

It's ok for me. I just suggested Michael since he did the last (hard)
work but if you agree I have no problem in doing it. :)

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Michael Lawnick]

Rodolfo Giometti said the following:
> On Tue, Aug 10, 2010 at 02:33:35PM +0200, Jean Delvare wrote:
>> On Thu, 22 Jul 2010 08:42:43 +0200, Rodolfo Giometti wrote:
>> > Michael, maybe you wish updating the linux-i2c wiki page regarding
>> > this issue?
>> > 
>> >    https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing
>> 
>> Or maybe you could do it, Rodolfo? Michael is apparently busy and so am
>> I.
> 
> It's ok for me. I just suggested Michael since he did the last (hard)
> work but if you agree I have no problem in doing it. :)
> 
Just came out of vacation.
Yes, I'm busy in my project, so I would be glad if you could do it. If
there are any questions you might PM me.

-- 
KR
Michael

i2c: Multiplexed I2C bus core support

[Gerlando Falauto]

Hi everyone,

On 07/09/2010 11:07 AM, Michael Lawnick wrote:
> Add multiplexed bus core support. I2C multiplexer and switches
> like pca954x get instantiated as new adapters per port.
>
> Signed-off-by: Michael Lawnick<demx1175@wbit01lx.(none)>
> Cc: Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>

I noticed that when you have multiple muxes connected to the same 
branch, implementation hides some information about the complete i2c 
topology.

So for instance if I have something like (notice there are three 
PCA9544's "in parallel", each with its own address, which I assume is a 
bit unusual):

#  CPU (i2c-0)
#  '--I2C Mux0 (PCA9547) @ 0x70
#      +--Port0 (i2c-1)
#      +--Port1 (i2c-2)
#      +--Port2 (i2c-3)
#      +--Port3 (i2c-4)
#      +--Port4 (i2c-5)
#      |   '--- I2C Mux1 (PCA9544) @ 0x74
#      |   |     +--Port0 (i2c-9)
#      |   |     +--Port1 (i2c-10)
#      |   |     +--Port2 (i2c-11)
#      |   |     '--Port3 (i2c-12)
#      |   +--- I2C Mux2 (PCA9544) @ 0x71
#      |   |     +--Port0 (i2c-13)
#      |   |     +--Port1 (i2c-14)
#      |   |     +--Port2 (i2c-15)
#      |   |     '--Port3 (i2c-16)
#      |   '--- I2C Mux3 (PCA9544) @ 0x72
#      |         +--Port0 (i2c-17)
#      |         +--Port1 (i2c-18)
#      |         +--Port2 (i2c-19)
#      |         '--Port3 (i2c-20)
#      +--Port5 (i2c-6)
#      +--Port6 (i2c-7)
#      '--Port7 (i2c-8)


under /sys/bus/i2c/devices/ I get:

0-0010 -> ../../../devices/platform/i2c-gpio/i2c-0/0-0010
0-0011 -> ../../../devices/platform/i2c-gpio/i2c-0/0-0011
0-0070 -> ../../../devices/platform/i2c-gpio/i2c-0/0-0070
5-0071 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/5-0071
5-0072 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/5-0072
5-0074 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/5-0074
|2c-0 -> ../../../devices/platform/i2c-gpio/i2c-0
i2c-1 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-1
i2c-2 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-2
i2c-3 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-3
i2c-4 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-4
i2c-5 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5
i2c-6 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-6
i2c-7 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-7
i2c-8 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-8
i2c-9 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-9
i2c-10 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-10
i2c-11 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-11
i2c-12 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-12
i2c-13 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-13
i2c-14 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-14
i2c-15 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-15
i2c-16 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-16
i2c-17 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-17
i2c-18 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-18
i2c-19 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-19
i2c-20 -> ../../../devices/platform/i2c-gpio/i2c-0/i2c-5/i2c-20

So I have no way of telling that:

  i2c-{9,10,11,12} are connected to mux1 at address 0x74 of i2c-5
  i2c-{13,14,15,16} are connected to mux2 at address 0x71 of i2c-5
  i2c-{17,18,19,20} are connected to mux3 at address 0x72 of i2c-5

because I get:

# cat /sys/bus/i2c/devices/i2c-9/name
i2c-5-mux (chan_id 0)
# cat /sys/bus/i2c/devices/i2c-13/name
i2c-5-mux (chan_id 0)

which only reveals i2c-9 is the bus segment connected to channel 0 of a 
mux behind i2c-5, but since there's two of them, I have no way to tell 
which one...

So I guess the easiest thing to do would be to add this information 
within the .name attribute, though I fear this may somehow turn out not 
as trivial at it looks.

What I'd like most though would be to get some hierarchy as follows:

devices/platform/i2c-gpio/i2c-0/0-0070/i2c-5/5-0071/i2c-9
devices/platform/i2c-gpio/i2c-0/0-0070/i2c-5/5-0071/i2c-10
devices/platform/i2c-gpio/i2c-0/0-0070/i2c-5/5-0071/i2c-11
devices/platform/i2c-gpio/i2c-0/0-0070/i2c-5/5-0071/i2c-12

or even something like:

devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-0(=>...i2c-9)
devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-1(=>...i2c-10)
devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-2(=>...i2c-11)
devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-3(=>...i2c-12)

Would the above seem reasonable?

Thanks!
Gerlando

Re: i2c: Multiplexed I2C bus core support

[Michael Lawnick]

Am 13.02.2013 16:36, schrieb Gerlando Falauto:
> Hi everyone,
> 
> On 07/09/2010 11:07 AM, Michael Lawnick wrote:
>> Add multiplexed bus core support. I2C multiplexer and switches
>> like pca954x get instantiated as new adapters per port.
>>
>> Signed-off-by: Michael Lawnick<demx1175@wbit01lx.(none)>
>> Cc: Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> 
> I noticed that when you have multiple muxes connected to the same 
> branch, implementation hides some information about the complete i2c 
> topology.
> 
> So for instance if I have something like (notice there are three 
> PCA9544's "in parallel", each with its own address, which I assume is a 
> bit unusual):
> 
> #  CPU (i2c-0)
> #  '--I2C Mux0 (PCA9547) @ 0x70
> #      +--Port0 (i2c-1)
> #      +--Port1 (i2c-2)
> #      +--Port2 (i2c-3)
> #      +--Port3 (i2c-4)
> #      +--Port4 (i2c-5)
> #      |   '--- I2C Mux1 (PCA9544) @ 0x74
> #      |   |     +--Port0 (i2c-9)
> #      |   |     +--Port1 (i2c-10)
> #      |   |     +--Port2 (i2c-11)
> #      |   |     '--Port3 (i2c-12)
> #      |   +--- I2C Mux2 (PCA9544) @ 0x71
> #      |   |     +--Port0 (i2c-13)
> #      |   |     +--Port1 (i2c-14)
> #      |   |     +--Port2 (i2c-15)
> #      |   |     '--Port3 (i2c-16)
...
> which only reveals i2c-9 is the bus segment connected to channel 0 of a 
> mux behind i2c-5, but since there's two of them, I have no way to tell 
> which one...
> 
> So I guess the easiest thing to do would be to add this information 
> within the .name attribute, though I fear this may somehow turn out not 
> as trivial at it looks.

Jean already detected this problem when mux code was brought into
kernel, but solution was postponed until someone gets enough pain ;-)
On my project I do no automatic i2c detection, but all is controlled via
user space. So I always know where and which devices and buses get
connected.

> 
> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-0(=>...i2c-9)
> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-1(=>...i2c-10)
> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-2(=>...i2c-11)
> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-3(=>...i2c-12)

Introducing links to sub-buses, named with respective channel id sounds
good for me. Beside that your link example does not reflect your ASCII
graphics of course ;-)

-- 
KR
Michael

Re: i2c: Multiplexed I2C bus core support

[Gerlando Falauto]

Hi Michael,

first of all thanks for your answer :-)

On 02/18/2013 11:19 AM, Michael Lawnick wrote:

> On my project I do no automatic i2c detection, but all is controlled via
> user space. So I always know where and which devices and buses get
> connected.

You mean you add devices through sysfs? That's another approach I was 
thinking I'd follow, except it still doesn't solve my problem... meaning 
I have no reliable way of telling the numbers assigned to the busses added.

>> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-0(=>...i2c-9)
>> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-1(=>...i2c-10)
>> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-2(=>...i2c-11)
>> devices/platform/i2c-gpio/i2c-0/0-0070/chan-4(=>...i2c-5)/5-0071/chan-3(=>...i2c-12)
>
> Introducing links to sub-buses, named with respective channel id sounds
> good for me. Beside that your link example does not reflect your ASCII
> graphics of course ;-)

*COUGH...COUGH* Oh, that was on purpose... *COUGH...COUGH* so to see 
whether anyone was paying attention. Good catch! :-)

Thanks!
Gerlando

Re: i2c: Multiplexed I2C bus core support

[Michael Lawnick]

Am 18.02.2013 11:38, schrieb Gerlando Falauto:
> Hi Michael,
> 
> first of all thanks for your answer :-)
> 
> On 02/18/2013 11:19 AM, Michael Lawnick wrote:
> 
>> On my project I do no automatic i2c detection, but all is controlled via
>> user space. So I always know where and which devices and buses get
>> connected.
> 
> You mean you add devices through sysfs? That's another approach I was 
> thinking I'd follow, except it still doesn't solve my problem... meaning 
> I have no reliable way of telling the numbers assigned to the busses added.

Under the assumption that bus numbers increase (which might not be true
in some special cases that do not hit me), you simply compare existing
buses before and after add.
This will be no more necessary if your patch works as intended. Due to
other work load I will not be able to test in the next weeks :-(
-- 
KR
Michael