More on SMBus multiplexing

More on SMBus multiplexing

[Jean Delvare]

Hi all,

The more I think of Mark Hoffman's proposal to handle SMBus
multiplexing, the more I think he pointed us to the right direction.
However, I now wonder if things aren't even more simple than we first
thought.

The main idea is to remove the physical adapter from the i2c adapters
list, and add virtual busses instead. We already have the possibility to
do that right now. The second thing Mark proposed was to lock the
physical SMBus. While it may sound safer, I don't think it is really
necessary. The sole fact of removing it from the main list will
disconnect all chip clients, and clients loaded later won't see the
physical bus anymore. The only way to attach a client to the SMBus would
be to do it manually. Certainly it could hurt if someone would do that
(because it would interfer with the virtual busses) but it happens that
nobody does this (the whole thing is based on the idea that the core
will automatically connect all clients from the client lists with all
adapters from the adapters list).

What are we trying to protect us from with this "exclusive client" idea
exactly? It will always be possible to run raw commands on the physical
bus anyway, just call adapter.algo->smbus_xfer. There is no way we can
protect us against that. So I think we are trying to protect us from an
event that will never happen and that the protection can be circumvented
by someone with enough motivation anyway. Much work for nothing, IMHO.

If we forget about that bus locking and exclusive client access, I don't
think that anything is missing from the i2c core. All we need is
i2c_{add,del}_adapter, and possibly a way to find an adapter from its id
(I didn't find something like i2c_find_adapter, did I miss it?). I'm not
even sure that this is needed though. A possible approach is to have the
main smbus driver export its i2c_adapter structure, so that the virtual
smbus driver can access it. This also automagically creates a dependancy
between both modules, and ensures that loading the vitual bus driver
will load the main bus driver first.

Note that this approach (as Mark's original one, but contrary to my
original one) would require a couple changes to sensors-detect. It
should really only be a matter of adding the correct entries in the pci
devices list though. However, this could be made unnecessary by
requesting the second module after loading the first one, using
request_module(). I tried, it works, I just don't know if we want to do
it or not. The drawback is that it makes a few board-specific changes to
the main module.

As a side note, the concept of "exclusive access" for a multiplexer
client also raises issues in the case one SMBus would host more that one
multiplexer chip directly at its "root" level. I see no reason why this
would be technically impossible, yet Mark's original model wouldn't
support it. I guess it would be possible to emulate it as if the second
multiplexer was located behind the first one, but this would certainly
make things more complex.

Of course, as with Mark's approach, we have the benefit of having two
separated module, no code duplication and little to very little change
to the original driver, depending on whether we implement the autoload
mechanism or not, for a maximum maintainabilty (so that's better than
my proposal for the amd756-s4882 driver in 2.4/CVS, even after taking
Mark's comment into account).

I was about to commit my i2c-amd756-s4882 module to the lm_sensors CVS
repository, but now I think that I'll try that new approach instead of
my brutal code inclusion, which works but isn't really clean, to say the
least.

As a kind of proof of concept, I did a fake i2c-i801-vaio module to
virtualize the SMBus on my laptop (although it doesn't have a mux chip).
It works just OK as far as I can tell. Of course the code is stupidly
useless (the virtual adapter doesn't do anything more than dumbly
redirect the calls to the physical bus), and lacks the mux client
registration part, since there is no such chip. I think that the idea is
clear though, and at least now we have code to comment on ;)

Thanks.

Index: kernel/busses/Module.mk
===================================================================
RCS file: /home/cvs/lm_sensors2/kernel/busses/Module.mk,v
retrieving revision 1.48
diff -u -r1.48 Module.mk
--- kernel/busses/Module.mk	16 Apr 2004 20:56:53 -0000	1.48
+++ kernel/busses/Module.mk	23 Oct 2004 18:24:17 -0000
@@ -48,6 +48,7 @@
 endif
 ifneq ($(shell if grep -q '^CONFIG_I2C_I801=y' $(LINUX)/.config; then echo 1; fi),1)
 KERNELBUSSESTARGETS += $(MODULE_DIR)/i2c-i801.o
+KERNELBUSSESTARGETS += $(MODULE_DIR)/i2c-i801-vaio.o
 endif
 ifneq ($(shell if grep -q '^CONFIG_I2C_I810=y' $(LINUX)/.config; then echo 1; fi),1)
 KERNELBUSSESTARGETS += $(MODULE_DIR)/i2c-i810.o
Index: kernel/busses/i2c-i801-vaio.c
===================================================================
RCS file: kernel/busses/i2c-i801-vaio.c
diff -N kernel/busses/i2c-i801-vaio.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ kernel/busses/i2c-i801-vaio.c	23 Oct 2004 18:24:17 -0000
@@ -0,0 +1,93 @@
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include "version.h"
+
+extern struct i2c_adapter i801_adapter;
+
+static s32 i801_vaio_access(struct i2c_adapter * adap, u16 addr,
+		       unsigned short flags, char read_write, u8 command,
+		       int size, union i2c_smbus_data * data)
+{
+	return i801_adapter.algo->smbus_xfer(adap, addr, flags,
+		read_write, command, size, data);
+}
+
+static struct i2c_algorithm smbus_algorithm = {
+	.name		= "Non-I2C SMBus adapter",
+	.id		= I2C_ALGO_SMBUS,
+	.smbus_xfer	= i801_vaio_access,
+};
+
+struct i2c_adapter i801_vaio_adapter = {
+	.owner		= THIS_MODULE,
+	.id		= I2C_ALGO_SMBUS | I2C_HW_SMBUS_I801,
+	.algo		= &smbus_algorithm,
+	.name		= "unset",
+};
+
+static int __init i2c_i801_vaio_init(void)
+{
+	int err;
+
+	printk(KERN_INFO "i2c-i801-vaio version %s (%s)\n", LM_VERSION,
+	       LM_DATE);
+
+	if (pci_find_subsys(PCI_VENDOR_ID_INTEL,
+			    PCI_DEVICE_ID_INTEL_82801CA_3,
+			    PCI_VENDOR_ID_SONY, 0x80e7, NULL) == NULL) {
+		return -ENODEV;
+	}
+
+	err = i2c_del_adapter(&i801_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to delete physical "
+		       "adapter\n");
+		return err;
+	}
+
+	/* Attach mux chip to main adapter here */
+
+	smbus_algorithm.functionality = i801_adapter.algo->functionality;
+	snprintf(i801_vaio_adapter.name, 32, "SMBus I801 virtual adapter");
+
+	err = i2c_add_adapter(&i801_vaio_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to add virtual "
+		       "adapter\n");
+		i2c_add_adapter(&i801_adapter);
+		return err;
+	}
+
+	return 0;
+}
+
+static void __exit i2c_i801_vaio_exit(void)
+{
+	int err;
+	
+	err = i2c_del_adapter(&i801_vaio_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to delete virtual "
+		       "adapter\n");
+		return;
+	}
+
+	/* Detach mux chip from main adapter here */
+
+	i2c_add_adapter(&i801_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to add physical "
+		       "adapter back\n");
+		return;
+	}
+}
+
+MODULE_AUTHOR("Jean Delvare <khali@linux-fr.org");
+MODULE_DESCRIPTION("I801 SMBus multiplexing");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_i801_vaio_init);
+module_exit(i2c_i801_vaio_exit);
Index: kernel/busses/i2c-i801.c
===================================================================
RCS file: /home/cvs/lm_sensors2/kernel/busses/i2c-i801.c,v
retrieving revision 1.36
diff -u -r1.36 i2c-i801.c
--- kernel/busses/i2c-i801.c	22 May 2004 04:02:19 -0000	1.36
+++ kernel/busses/i2c-i801.c	23 Oct 2004 18:24:18 -0000
@@ -49,6 +49,7 @@
 #include <linux/ioport.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/kmod.h>
 #include <asm/io.h>
 #include "version.h"
 #include "sensors_compat.h"
@@ -548,7 +549,7 @@
 	.functionality	= i801_func,
 };
 
-static struct i2c_adapter i801_adapter = {
+struct i2c_adapter i801_adapter = {
 	.owner		= THIS_MODULE,
 	.id		= I2C_ALGO_SMBUS | I2C_HW_SMBUS_I801,
 	.algo		= &smbus_algorithm,
@@ -558,6 +559,12 @@
 static struct pci_device_id i801_ids[] __devinitdata = {
 	{
 		.vendor =	PCI_VENDOR_ID_INTEL,
+		.device =	PCI_DEVICE_ID_INTEL_82801CA_3,
+		.subvendor =	PCI_VENDOR_ID_SONY,
+		.subdevice =	0x80e7,
+	},
+	{
+		.vendor =	PCI_VENDOR_ID_INTEL,
 		.device =	PCI_DEVICE_ID_INTEL_82801AA_3,
 		.subvendor =	PCI_ANY_ID,
 		.subdevice =	PCI_ANY_ID,
@@ -609,6 +616,7 @@
 
 static int __devinit i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
+	int err;
 
 	if (i801_setup(dev)) {
 		dev_warn(dev,
@@ -618,7 +626,14 @@
 
 	snprintf(i801_adapter.name, 32,
 		"SMBus I801 adapter at %04x", i801_smba);
-	return i2c_add_adapter(&i801_adapter);
+	if ((err = i2c_add_adapter(&i801_adapter)))
+		return err;
+
+	if (dev->subsystem_vendor == PCI_VENDOR_ID_SONY
+	 && dev->subsystem_device == 0x80e7)
+	 	request_module("i2c-i801-vaio");
+
+	return 0;
 }
 
 static void __devexit i801_remove(struct pci_dev *dev)
@@ -651,5 +666,7 @@
 MODULE_DESCRIPTION("I801 SMBus driver");
 MODULE_LICENSE("GPL");
 
+EXPORT_SYMBOL(i801_adapter);
+
 module_init(i2c_i801_init);
 module_exit(i2c_i801_exit);



-- 
Jean Delvare
http://khali.linux-fr.org/

Re: More on SMBus multiplexing

[Jean Delvare]

Replying to myself:

> As a kind of proof of concept, I did a fake i2c-i801-vaio module to
> virtualize the SMBus on my laptop (although it doesn't have a mux
> chip). It works just OK as far as I can tell. Of course the code is
> stupidly useless (the virtual adapter doesn't do anything more than
> dumbly redirect the calls to the physical bus), and lacks the mux
> client registration part, since there is no such chip. I think that
> the idea is clear though, and at least now we have code to comment on
> ;)

I find that I am unable to actually register the mux client. Odd, since
it worked OK on a 2.4 kernel, and several tries led me nowhere on 2.6
kernels. If anyone has sample code to just occupy a given I2C address on
a given bus, please share it with me.

However, why do we even need this? Looks far easier to simply exclude
the multiplexer address from the virtual busses (which we need to do
anyway). Nobody is supposed to access the physical bus directly (it's
not in the main adapters list anyway). Again, I see no reason to protect
us from something that is just never going to happen. This makes the
whole thing even more simple, exactly as in my demo code.

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: More on SMBus multiplexing

[Jean Delvare]

Hi Greg,

Would you accept such a patch? This adds Tyan S4882-specific SMBus
multiplexing support, in the form of a separate driver, as per Mark
Hoffman's suggestion.

Changes to the original driver (i2c-amd756) are kept to a minimum:

1* The i2c_adapter structure is exported (some noise due to a struct
name change, I didn't want to export something as vague as
"amd756_adapter").

2* Autoload of the new driver (i2c-amd756-s4882) when needed. This can
go away if you don't like it, but I thought it'd be more convenient for
the end user. BTW, what will happen if the i2c-amd756-s4882 module
doesn't exist? Should I add some #ifdef magic in the i2c-amd756 driver
so that the autoload will not happen if the i2c-amd756-s4882 module was
not selected?

The i2c-amd756-s4882 driver follows Mark Hoffman's suggestion, but in a
simplified way. On load:

1* Remove the physical adapter from the main i2c adapters list. As a
side effect, any client gets kicked off.

2* Register 5 virtual busses using to wrappers pointing to the original
SMBus code. Clients can connect to the new adapters where it makes
sense.

On unload, same thing but the other way around.

This approach has a drawback in the case several client drivers are
already loaded (they will get attached, detached and attached again in a
raw), but I don't think this is really a problem:

1* In most cases bus drivers are loaded before clients anyway.

2* SMBus multiplexing is quite rare, and I don't care if people with
such specific boards experience drawbacks at init time in uncommon
cases.

Advantages of the approach, however, are great. Multiplexing code
belongs to a different driver, which improves maintainability as Mark
pointed out, and means no impact on module size nor performance for
regular users. The whole thing is also almost transparent for userspace
and client drivers, requiring very very little change, if any at all.

Greg, anyone, comments on the approach and code are welcome. If
everyone's OK, we'll go that way for both the 2.6 kernel and lm_sensors
(for 2.4).

Thanks.

diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Kconfig linux-2.6.9-rc4-bk1/drivers/i2c/busses/Kconfig
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Kconfig	2004-10-16 19:37:33.000000000 +0200
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/Kconfig	2004-10-24 18:46:53.000000000 +0200
@@ -51,6 +51,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-amd756.
 
+config I2C_AMD756_S4882
+	tristate "SMBus multiplexing on the Tyan S4882"
+	depends on I2C_AMD756 && EXPERIMENTAL
+	default y
+	help
+	  Enabling this option will add specific SMBus support for the Tyan
+	  S4882 motherboard.  On this 4-CPU board, the SMBus is multiplexed
+	  over 8 different channels, where the various memory module EEPROMs
+	  and temperature sensors live.  Saying yes here will give you access
+	  to these in addition to the trunk.
+
 config I2C_AMD8111
 	tristate "AMD 8111"
 	depends on I2C && PCI && EXPERIMENTAL
diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Makefile linux-2.6.9-rc4-bk1/drivers/i2c/busses/Makefile
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Makefile	2004-10-12 19:44:25.000000000 +0200
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/Makefile	2004-10-23 22:55:45.000000000 +0200
@@ -6,6 +6,7 @@
 obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
 obj-$(CONFIG_I2C_ALI15X3)	+= i2c-ali15x3.o
 obj-$(CONFIG_I2C_AMD756)	+= i2c-amd756.o
+obj-$(CONFIG_I2C_AMD756_S4882)	+= i2c-amd756-s4882.o
 obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756-s4882.c linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756-s4882.c
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756-s4882.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756-s4882.c	2004-10-24 18:42:12.000000000 +0200
@@ -0,0 +1,266 @@
+/*
+ * i2c-amd756-s4882.c - i2c-amd756 extras for the Tyan S4882 motherboard
+ *
+ * Copyright (C) 2004 Jean Delvare <khali@linux-fr.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.
+ */
+ 
+/*
+ * We select the channels by sending commands to the Philips
+ * PCA9556 chip at I2C address 0x18. The main adapter is used for
+ * the non-multiplexed part of the bus, and 4 virtual adapters
+ * are defined for the multiplexed addresses: 0x50-0x53 (memory
+ * module EEPROM) located on channels 1-4, and 0x4c (LM63)
+ * located on multiplexed channels 0 and 5-7. We define one
+ * virtual adapter per CPU, which corresponds to two multiplexed
+ * channels:
+ *   CPU0: virtual adapter 1, channels 1 and 0
+ *   CPU1: virtual adapter 2, channels 2 and 5
+ *   CPU2: virtual adapter 3, channels 3 and 6
+ *   CPU3: virtual adapter 4, channels 4 and 7
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+
+extern struct i2c_adapter amd756_smbus;
+
+static struct i2c_adapter *s4882_adapter;
+static struct i2c_algorithm *s4882_algo;
+
+/* Wrapper access functions for multiplexed SMBus */
+static struct semaphore amd756_lock;
+
+static s32 amd756_access_virt0(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	int error;
+
+	/* We exclude the multiplexed addresses */
+	if (addr == 0x4c || (addr & 0xfc) == 0x50 || (addr & 0xfc) == 0x30
+	 || addr == 0x18)
+		return -1;
+
+	down(&amd756_lock);
+
+	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
+					      command, size, data);
+
+	up(&amd756_lock);
+
+	return error;
+}
+
+/* We remember the last used channels combination so as to only switch
+   channels when it is really needed. This greatly reduces the SMBus
+   overhead, but also assumes that nobody will be writing to the PCA9556
+   in our back. */
+static u8 last_channels;
+
+static inline s32 amd756_access_channel(struct i2c_adapter * adap, u16 addr,
+					unsigned short flags, char read_write,
+					u8 command, int size,
+					union i2c_smbus_data * data,
+					u8 channels)
+{
+	int error;
+
+	/* We exclude the non-multiplexed addresses */
+	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
+		return -1;
+
+	down(&amd756_lock);
+
+	if (last_channels != channels) {
+		union i2c_smbus_data mplxdata;
+		mplxdata.byte = channels;
+
+		error = amd756_smbus.algo->smbus_xfer(adap, 0x18, 0,
+						      I2C_SMBUS_WRITE, 0x01,
+						      I2C_SMBUS_BYTE_DATA,
+						      &mplxdata);
+		if (error)
+			goto UNLOCK;
+		last_channels = channels;
+	}
+	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
+					      command, size, data);
+
+UNLOCK:
+	up(&amd756_lock);
+	return error;
+}
+
+static s32 amd756_access_virt1(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU0: channels 1 and 0 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x03);
+}
+
+static s32 amd756_access_virt2(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU1: channels 2 and 5 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x24);
+}
+
+static s32 amd756_access_virt3(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU2: channels 3 and 6 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x48);
+}
+
+static s32 amd756_access_virt4(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU3: channels 4 and 7 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x90);
+}
+
+static int __init amd756_s4882_init(void)
+{
+	int i, error;
+	union i2c_smbus_data ioconfig;
+
+	if (pci_find_subsys(PCI_VENDOR_ID_AMD, 0x746B, PCI_VENDOR_ID_AMD,
+			    0x36C0, NULL) == NULL)
+		return -ENODEV;
+
+	printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4882\n");
+
+	init_MUTEX(&amd756_lock);
+
+	/* Unregister physical bus */
+	error = i2c_del_adapter(&amd756_smbus);
+	if (error) {
+		dev_err(&amd756_smbus.dev, "Physical bus removal "
+			"failed\n");
+		goto ERROR0;
+	}
+
+	/* Define the 5 virtual adapters and algorithms structures */
+	if (!(s4882_adapter = kmalloc(5 * sizeof(struct i2c_adapter),
+				      GFP_KERNEL))) {
+		error = -ENOMEM;
+		goto ERROR1;
+	}
+	if (!(s4882_algo = kmalloc(5 * sizeof(struct i2c_algorithm),
+				   GFP_KERNEL))) {
+		error = -ENOMEM;
+		goto ERROR2;
+	}
+
+	/* Fill in the new structures */
+	s4882_algo[0] = *(amd756_smbus.algo);
+	s4882_algo[0].smbus_xfer = amd756_access_virt0;
+	s4882_adapter[0] = amd756_smbus;
+	s4882_adapter[0].algo = s4882_algo;
+	for (i = 1; i < 5; i++) {
+		s4882_algo[i] = *(amd756_smbus.algo);
+		s4882_adapter[i] = amd756_smbus;
+		sprintf(s4882_adapter[i].name,
+			"SMBus 8111 adapter (CPU%d)", i-1);
+		s4882_adapter[i].algo = s4882_algo+i;
+	}
+	s4882_algo[1].smbus_xfer = amd756_access_virt1;
+	s4882_algo[2].smbus_xfer = amd756_access_virt2;
+	s4882_algo[3].smbus_xfer = amd756_access_virt3;
+	s4882_algo[4].smbus_xfer = amd756_access_virt4;
+
+	/* Configure the PCA9556 multiplexer */
+	ioconfig.byte = 0x00; /* All I/O to output mode */
+	error = amd756_smbus.algo->smbus_xfer(&amd756_smbus, 0x18, 0,
+					      I2C_SMBUS_WRITE, 0x03,
+					      I2C_SMBUS_BYTE_DATA, &ioconfig);
+	if (error) {
+		dev_dbg(&amd756_smbus.dev, "PCA9556 configuration failed\n");
+		error = -EIO;
+		goto ERROR3;
+	}
+
+	/* Register virtual adapters */
+	for (i = 0; i < 5; i++) {
+		error = i2c_add_adapter(s4882_adapter+i);
+		if (error) {
+			dev_err(&amd756_smbus.dev,
+			       "Virtual adapter %d registration "
+			       "failed, module not inserted\n", i);
+			for (i--; i >= 0; i--)
+				i2c_del_adapter(s4882_adapter+i);
+			goto ERROR3;
+		}
+	}
+
+	return 0;
+
+ERROR3:
+	kfree(s4882_algo);
+	s4882_algo = NULL;
+ERROR2:
+	kfree(s4882_adapter);
+	s4882_adapter = NULL;
+ERROR1:
+	i2c_del_adapter(&amd756_smbus);
+ERROR0:
+	return error;
+}
+
+static void __exit amd756_s4882_exit(void)
+{
+	if (s4882_adapter) {
+		int i;
+
+		for (i = 0; i < 5; i++)
+			i2c_del_adapter(s4882_adapter+i);
+		kfree(s4882_adapter);
+		s4882_adapter = NULL;
+	}
+	if (s4882_algo) {
+		kfree(s4882_algo);
+		s4882_algo = NULL;
+	}
+
+	/* Restore physical bus */
+	if (i2c_add_adapter(&amd756_smbus))
+		dev_err(&amd756_smbus.dev, "Physical bus restoration "
+			"failed\n");
+}
+
+MODULE_AUTHOR("Jean Delvare <khali@linux-fr.org>");
+MODULE_DESCRIPTION("S4882 SMBus multiplexing");
+MODULE_LICENSE("GPL");
+
+module_init(amd756_s4882_init);
+module_exit(amd756_s4882_exit);
diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756.c linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756.c
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756.c	2004-10-16 21:13:45.000000000 +0200
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756.c	2004-10-24 18:51:52.000000000 +0200
@@ -47,6 +47,7 @@
 #include <linux/ioport.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/kmod.h>
 #include <asm/io.h>
 
 /* AMD756 SMBus address offsets */
@@ -302,23 +303,24 @@
 	.functionality	= amd756_func,
 };
 
-static struct i2c_adapter amd756_adapter = {
+struct i2c_adapter amd756_smbus = {
 	.owner		= THIS_MODULE,
 	.class          = I2C_CLASS_HWMON,
 	.algo		= &smbus_algorithm,
 	.name		= "unset",
 };
 
-enum chiptype { AMD756, AMD766, AMD768, NFORCE, AMD8111 };
+enum chiptype { AMD756, AMD766, AMD768, NFORCE, AMD8111, S4882 };
 static const char* chipname[] = {
 	"AMD756", "AMD766", "AMD768",
-	"nVidia nForce", "AMD8111",
+	"nVidia nForce", "AMD8111", "AMD8111",
 };
 
 static struct pci_device_id amd756_ids[] = {
 	{PCI_VENDOR_ID_AMD, 0x740B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD756 },
 	{PCI_VENDOR_ID_AMD, 0x7413, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD766 },
 	{PCI_VENDOR_ID_AMD, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD768 },
+	{PCI_VENDOR_ID_AMD, 0x746B, PCI_VENDOR_ID_AMD, 0x36C0, 0, 0, S4882 },
 	{PCI_VENDOR_ID_AMD, 0x746B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD8111 },
 	{PCI_VENDOR_ID_NVIDIA, 0x01B4, PCI_ANY_ID, PCI_ANY_ID, 0, 0, NFORCE },
 	{ 0, }
@@ -330,6 +332,7 @@
 				  const struct pci_device_id *id)
 {
 	int nforce = (id->driver_data == NFORCE);
+	int s4882 = (id->driver_data == S4882);
 	int error;
 	u8 temp;
 	
@@ -374,18 +377,21 @@
 	dev_dbg(&pdev->dev, "AMD756_smba = 0x%X\n", amd756_ioport);
 
 	/* set up the driverfs linkage to our parent device */
-	amd756_adapter.dev.parent = &pdev->dev;
+	amd756_smbus.dev.parent = &pdev->dev;
 
-	sprintf(amd756_adapter.name, "SMBus %s adapter at %04x",
+	sprintf(amd756_smbus.name, "SMBus %s adapter at %04x",
 		chipname[id->driver_data], amd756_ioport);
 
-	error = i2c_add_adapter(&amd756_adapter);
+	error = i2c_add_adapter(&amd756_smbus);
 	if (error) {
 		dev_err(&pdev->dev,
 			"Adapter registration failed, module not inserted\n");
 		goto out_err;
 	}
 
+	if (s4882)
+		request_module("i2c-amd756-s4882");
+
 	return 0;
 
  out_err:
@@ -395,7 +401,7 @@
 
 static void __devexit amd756_remove(struct pci_dev *dev)
 {
-	i2c_del_adapter(&amd756_adapter);
+	i2c_del_adapter(&amd756_smbus);
 	release_region(amd756_ioport, SMB_IOSIZE);
 }
 
@@ -420,5 +426,7 @@
 MODULE_DESCRIPTION("AMD756/766/768/8111 and nVidia nForce SMBus driver");
 MODULE_LICENSE("GPL");
 
+EXPORT_SYMBOL(amd756_smbus);
+
 module_init(amd756_init)
 module_exit(amd756_exit)


-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Jean Delvare]

Hi all,

The more I think of Mark Hoffman's proposal to handle SMBus
multiplexing, the more I think he pointed us to the right direction.
However, I now wonder if things aren't even more simple than we first
thought.

The main idea is to remove the physical adapter from the i2c adapters
list, and add virtual busses instead. We already have the possibility to
do that right now. The second thing Mark proposed was to lock the
physical SMBus. While it may sound safer, I don't think it is really
necessary. The sole fact of removing it from the main list will
disconnect all chip clients, and clients loaded later won't see the
physical bus anymore. The only way to attach a client to the SMBus would
be to do it manually. Certainly it could hurt if someone would do that
(because it would interfer with the virtual busses) but it happens that
nobody does this (the whole thing is based on the idea that the core
will automatically connect all clients from the client lists with all
adapters from the adapters list).

What are we trying to protect us from with this "exclusive client" idea
exactly? It will always be possible to run raw commands on the physical
bus anyway, just call adapter.algo->smbus_xfer. There is no way we can
protect us against that. So I think we are trying to protect us from an
event that will never happen and that the protection can be circumvented
by someone with enough motivation anyway. Much work for nothing, IMHO.

If we forget about that bus locking and exclusive client access, I don't
think that anything is missing from the i2c core. All we need is
i2c_{add,del}_adapter, and possibly a way to find an adapter from its id
(I didn't find something like i2c_find_adapter, did I miss it?). I'm not
even sure that this is needed though. A possible approach is to have the
main smbus driver export its i2c_adapter structure, so that the virtual
smbus driver can access it. This also automagically creates a dependancy
between both modules, and ensures that loading the vitual bus driver
will load the main bus driver first.

Note that this approach (as Mark's original one, but contrary to my
original one) would require a couple changes to sensors-detect. It
should really only be a matter of adding the correct entries in the pci
devices list though. However, this could be made unnecessary by
requesting the second module after loading the first one, using
request_module(). I tried, it works, I just don't know if we want to do
it or not. The drawback is that it makes a few board-specific changes to
the main module.

As a side note, the concept of "exclusive access" for a multiplexer
client also raises issues in the case one SMBus would host more that one
multiplexer chip directly at its "root" level. I see no reason why this
would be technically impossible, yet Mark's original model wouldn't
support it. I guess it would be possible to emulate it as if the second
multiplexer was located behind the first one, but this would certainly
make things more complex.

Of course, as with Mark's approach, we have the benefit of having two
separated module, no code duplication and little to very little change
to the original driver, depending on whether we implement the autoload
mechanism or not, for a maximum maintainabilty (so that's better than
my proposal for the amd756-s4882 driver in 2.4/CVS, even after taking
Mark's comment into account).

I was about to commit my i2c-amd756-s4882 module to the lm_sensors CVS
repository, but now I think that I'll try that new approach instead of
my brutal code inclusion, which works but isn't really clean, to say the
least.

As a kind of proof of concept, I did a fake i2c-i801-vaio module to
virtualize the SMBus on my laptop (although it doesn't have a mux chip).
It works just OK as far as I can tell. Of course the code is stupidly
useless (the virtual adapter doesn't do anything more than dumbly
redirect the calls to the physical bus), and lacks the mux client
registration part, since there is no such chip. I think that the idea is
clear though, and at least now we have code to comment on ;)

Thanks.

Index: kernel/busses/Module.mk
=================================RCS file: /home/cvs/lm_sensors2/kernel/busses/Module.mk,v
retrieving revision 1.48
diff -u -r1.48 Module.mk
--- kernel/busses/Module.mk	16 Apr 2004 20:56:53 -0000	1.48
+++ kernel/busses/Module.mk	23 Oct 2004 18:24:17 -0000
@@ -48,6 +48,7 @@
 endif
 ifneq ($(shell if grep -q '^CONFIG_I2C_I801=y' $(LINUX)/.config; then echo 1; fi),1)
 KERNELBUSSESTARGETS += $(MODULE_DIR)/i2c-i801.o
+KERNELBUSSESTARGETS += $(MODULE_DIR)/i2c-i801-vaio.o
 endif
 ifneq ($(shell if grep -q '^CONFIG_I2C_I810=y' $(LINUX)/.config; then echo 1; fi),1)
 KERNELBUSSESTARGETS += $(MODULE_DIR)/i2c-i810.o
Index: kernel/busses/i2c-i801-vaio.c
=================================RCS file: kernel/busses/i2c-i801-vaio.c
diff -N kernel/busses/i2c-i801-vaio.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ kernel/busses/i2c-i801-vaio.c	23 Oct 2004 18:24:17 -0000
@@ -0,0 +1,93 @@
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include "version.h"
+
+extern struct i2c_adapter i801_adapter;
+
+static s32 i801_vaio_access(struct i2c_adapter * adap, u16 addr,
+		       unsigned short flags, char read_write, u8 command,
+		       int size, union i2c_smbus_data * data)
+{
+	return i801_adapter.algo->smbus_xfer(adap, addr, flags,
+		read_write, command, size, data);
+}
+
+static struct i2c_algorithm smbus_algorithm = {
+	.name		= "Non-I2C SMBus adapter",
+	.id		= I2C_ALGO_SMBUS,
+	.smbus_xfer	= i801_vaio_access,
+};
+
+struct i2c_adapter i801_vaio_adapter = {
+	.owner		= THIS_MODULE,
+	.id		= I2C_ALGO_SMBUS | I2C_HW_SMBUS_I801,
+	.algo		= &smbus_algorithm,
+	.name		= "unset",
+};
+
+static int __init i2c_i801_vaio_init(void)
+{
+	int err;
+
+	printk(KERN_INFO "i2c-i801-vaio version %s (%s)\n", LM_VERSION,
+	       LM_DATE);
+
+	if (pci_find_subsys(PCI_VENDOR_ID_INTEL,
+			    PCI_DEVICE_ID_INTEL_82801CA_3,
+			    PCI_VENDOR_ID_SONY, 0x80e7, NULL) = NULL) {
+		return -ENODEV;
+	}
+
+	err = i2c_del_adapter(&i801_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to delete physical "
+		       "adapter\n");
+		return err;
+	}
+
+	/* Attach mux chip to main adapter here */
+
+	smbus_algorithm.functionality = i801_adapter.algo->functionality;
+	snprintf(i801_vaio_adapter.name, 32, "SMBus I801 virtual adapter");
+
+	err = i2c_add_adapter(&i801_vaio_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to add virtual "
+		       "adapter\n");
+		i2c_add_adapter(&i801_adapter);
+		return err;
+	}
+
+	return 0;
+}
+
+static void __exit i2c_i801_vaio_exit(void)
+{
+	int err;
+	
+	err = i2c_del_adapter(&i801_vaio_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to delete virtual "
+		       "adapter\n");
+		return;
+	}
+
+	/* Detach mux chip from main adapter here */
+
+	i2c_add_adapter(&i801_adapter);
+	if (err) {
+		printk(KERN_ERR "i2c-i801-vaio: Failed to add physical "
+		       "adapter back\n");
+		return;
+	}
+}
+
+MODULE_AUTHOR("Jean Delvare <khali@linux-fr.org");
+MODULE_DESCRIPTION("I801 SMBus multiplexing");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_i801_vaio_init);
+module_exit(i2c_i801_vaio_exit);
Index: kernel/busses/i2c-i801.c
=================================RCS file: /home/cvs/lm_sensors2/kernel/busses/i2c-i801.c,v
retrieving revision 1.36
diff -u -r1.36 i2c-i801.c
--- kernel/busses/i2c-i801.c	22 May 2004 04:02:19 -0000	1.36
+++ kernel/busses/i2c-i801.c	23 Oct 2004 18:24:18 -0000
@@ -49,6 +49,7 @@
 #include <linux/ioport.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/kmod.h>
 #include <asm/io.h>
 #include "version.h"
 #include "sensors_compat.h"
@@ -548,7 +549,7 @@
 	.functionality	= i801_func,
 };
 
-static struct i2c_adapter i801_adapter = {
+struct i2c_adapter i801_adapter = {
 	.owner		= THIS_MODULE,
 	.id		= I2C_ALGO_SMBUS | I2C_HW_SMBUS_I801,
 	.algo		= &smbus_algorithm,
@@ -558,6 +559,12 @@
 static struct pci_device_id i801_ids[] __devinitdata = {
 	{
 		.vendor =	PCI_VENDOR_ID_INTEL,
+		.device =	PCI_DEVICE_ID_INTEL_82801CA_3,
+		.subvendor =	PCI_VENDOR_ID_SONY,
+		.subdevice =	0x80e7,
+	},
+	{
+		.vendor =	PCI_VENDOR_ID_INTEL,
 		.device =	PCI_DEVICE_ID_INTEL_82801AA_3,
 		.subvendor =	PCI_ANY_ID,
 		.subdevice =	PCI_ANY_ID,
@@ -609,6 +616,7 @@
 
 static int __devinit i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
+	int err;
 
 	if (i801_setup(dev)) {
 		dev_warn(dev,
@@ -618,7 +626,14 @@
 
 	snprintf(i801_adapter.name, 32,
 		"SMBus I801 adapter at %04x", i801_smba);
-	return i2c_add_adapter(&i801_adapter);
+	if ((err = i2c_add_adapter(&i801_adapter)))
+		return err;
+
+	if (dev->subsystem_vendor = PCI_VENDOR_ID_SONY
+	 && dev->subsystem_device = 0x80e7)
+	 	request_module("i2c-i801-vaio");
+
+	return 0;
 }
 
 static void __devexit i801_remove(struct pci_dev *dev)
@@ -651,5 +666,7 @@
 MODULE_DESCRIPTION("I801 SMBus driver");
 MODULE_LICENSE("GPL");
 
+EXPORT_SYMBOL(i801_adapter);
+
 module_init(i2c_i801_init);
 module_exit(i2c_i801_exit);



-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Jean Delvare]

Replying to myself:

> As a kind of proof of concept, I did a fake i2c-i801-vaio module to
> virtualize the SMBus on my laptop (although it doesn't have a mux
> chip). It works just OK as far as I can tell. Of course the code is
> stupidly useless (the virtual adapter doesn't do anything more than
> dumbly redirect the calls to the physical bus), and lacks the mux
> client registration part, since there is no such chip. I think that
> the idea is clear though, and at least now we have code to comment on
> ;)

I find that I am unable to actually register the mux client. Odd, since
it worked OK on a 2.4 kernel, and several tries led me nowhere on 2.6
kernels. If anyone has sample code to just occupy a given I2C address on
a given bus, please share it with me.

However, why do we even need this? Looks far easier to simply exclude
the multiplexer address from the virtual busses (which we need to do
anyway). Nobody is supposed to access the physical bus directly (it's
not in the main adapters list anyway). Again, I see no reason to protect
us from something that is just never going to happen. This makes the
whole thing even more simple, exactly as in my demo code.

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Jean Delvare]

Hi Greg,

Would you accept such a patch? This adds Tyan S4882-specific SMBus
multiplexing support, in the form of a separate driver, as per Mark
Hoffman's suggestion.

Changes to the original driver (i2c-amd756) are kept to a minimum:

1* The i2c_adapter structure is exported (some noise due to a struct
name change, I didn't want to export something as vague as
"amd756_adapter").

2* Autoload of the new driver (i2c-amd756-s4882) when needed. This can
go away if you don't like it, but I thought it'd be more convenient for
the end user. BTW, what will happen if the i2c-amd756-s4882 module
doesn't exist? Should I add some #ifdef magic in the i2c-amd756 driver
so that the autoload will not happen if the i2c-amd756-s4882 module was
not selected?

The i2c-amd756-s4882 driver follows Mark Hoffman's suggestion, but in a
simplified way. On load:

1* Remove the physical adapter from the main i2c adapters list. As a
side effect, any client gets kicked off.

2* Register 5 virtual busses using to wrappers pointing to the original
SMBus code. Clients can connect to the new adapters where it makes
sense.

On unload, same thing but the other way around.

This approach has a drawback in the case several client drivers are
already loaded (they will get attached, detached and attached again in a
raw), but I don't think this is really a problem:

1* In most cases bus drivers are loaded before clients anyway.

2* SMBus multiplexing is quite rare, and I don't care if people with
such specific boards experience drawbacks at init time in uncommon
cases.

Advantages of the approach, however, are great. Multiplexing code
belongs to a different driver, which improves maintainability as Mark
pointed out, and means no impact on module size nor performance for
regular users. The whole thing is also almost transparent for userspace
and client drivers, requiring very very little change, if any at all.

Greg, anyone, comments on the approach and code are welcome. If
everyone's OK, we'll go that way for both the 2.6 kernel and lm_sensors
(for 2.4).

Thanks.

diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Kconfig linux-2.6.9-rc4-bk1/drivers/i2c/busses/Kconfig
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Kconfig	2004-10-16 19:37:33.000000000 +0200
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/Kconfig	2004-10-24 18:46:53.000000000 +0200
@@ -51,6 +51,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-amd756.
 
+config I2C_AMD756_S4882
+	tristate "SMBus multiplexing on the Tyan S4882"
+	depends on I2C_AMD756 && EXPERIMENTAL
+	default y
+	help
+	  Enabling this option will add specific SMBus support for the Tyan
+	  S4882 motherboard.  On this 4-CPU board, the SMBus is multiplexed
+	  over 8 different channels, where the various memory module EEPROMs
+	  and temperature sensors live.  Saying yes here will give you access
+	  to these in addition to the trunk.
+
 config I2C_AMD8111
 	tristate "AMD 8111"
 	depends on I2C && PCI && EXPERIMENTAL
diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Makefile linux-2.6.9-rc4-bk1/drivers/i2c/busses/Makefile
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Makefile	2004-10-12 19:44:25.000000000 +0200
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/Makefile	2004-10-23 22:55:45.000000000 +0200
@@ -6,6 +6,7 @@
 obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
 obj-$(CONFIG_I2C_ALI15X3)	+= i2c-ali15x3.o
 obj-$(CONFIG_I2C_AMD756)	+= i2c-amd756.o
+obj-$(CONFIG_I2C_AMD756_S4882)	+= i2c-amd756-s4882.o
 obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756-s4882.c linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756-s4882.c
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756-s4882.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756-s4882.c	2004-10-24 18:42:12.000000000 +0200
@@ -0,0 +1,266 @@
+/*
+ * i2c-amd756-s4882.c - i2c-amd756 extras for the Tyan S4882 motherboard
+ *
+ * Copyright (C) 2004 Jean Delvare <khali@linux-fr.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.
+ */
+ 
+/*
+ * We select the channels by sending commands to the Philips
+ * PCA9556 chip at I2C address 0x18. The main adapter is used for
+ * the non-multiplexed part of the bus, and 4 virtual adapters
+ * are defined for the multiplexed addresses: 0x50-0x53 (memory
+ * module EEPROM) located on channels 1-4, and 0x4c (LM63)
+ * located on multiplexed channels 0 and 5-7. We define one
+ * virtual adapter per CPU, which corresponds to two multiplexed
+ * channels:
+ *   CPU0: virtual adapter 1, channels 1 and 0
+ *   CPU1: virtual adapter 2, channels 2 and 5
+ *   CPU2: virtual adapter 3, channels 3 and 6
+ *   CPU3: virtual adapter 4, channels 4 and 7
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+
+extern struct i2c_adapter amd756_smbus;
+
+static struct i2c_adapter *s4882_adapter;
+static struct i2c_algorithm *s4882_algo;
+
+/* Wrapper access functions for multiplexed SMBus */
+static struct semaphore amd756_lock;
+
+static s32 amd756_access_virt0(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	int error;
+
+	/* We exclude the multiplexed addresses */
+	if (addr = 0x4c || (addr & 0xfc) = 0x50 || (addr & 0xfc) = 0x30
+	 || addr = 0x18)
+		return -1;
+
+	down(&amd756_lock);
+
+	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
+					      command, size, data);
+
+	up(&amd756_lock);
+
+	return error;
+}
+
+/* We remember the last used channels combination so as to only switch
+   channels when it is really needed. This greatly reduces the SMBus
+   overhead, but also assumes that nobody will be writing to the PCA9556
+   in our back. */
+static u8 last_channels;
+
+static inline s32 amd756_access_channel(struct i2c_adapter * adap, u16 addr,
+					unsigned short flags, char read_write,
+					u8 command, int size,
+					union i2c_smbus_data * data,
+					u8 channels)
+{
+	int error;
+
+	/* We exclude the non-multiplexed addresses */
+	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
+		return -1;
+
+	down(&amd756_lock);
+
+	if (last_channels != channels) {
+		union i2c_smbus_data mplxdata;
+		mplxdata.byte = channels;
+
+		error = amd756_smbus.algo->smbus_xfer(adap, 0x18, 0,
+						      I2C_SMBUS_WRITE, 0x01,
+						      I2C_SMBUS_BYTE_DATA,
+						      &mplxdata);
+		if (error)
+			goto UNLOCK;
+		last_channels = channels;
+	}
+	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
+					      command, size, data);
+
+UNLOCK:
+	up(&amd756_lock);
+	return error;
+}
+
+static s32 amd756_access_virt1(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU0: channels 1 and 0 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x03);
+}
+
+static s32 amd756_access_virt2(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU1: channels 2 and 5 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x24);
+}
+
+static s32 amd756_access_virt3(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU2: channels 3 and 6 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x48);
+}
+
+static s32 amd756_access_virt4(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU3: channels 4 and 7 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x90);
+}
+
+static int __init amd756_s4882_init(void)
+{
+	int i, error;
+	union i2c_smbus_data ioconfig;
+
+	if (pci_find_subsys(PCI_VENDOR_ID_AMD, 0x746B, PCI_VENDOR_ID_AMD,
+			    0x36C0, NULL) = NULL)
+		return -ENODEV;
+
+	printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4882\n");
+
+	init_MUTEX(&amd756_lock);
+
+	/* Unregister physical bus */
+	error = i2c_del_adapter(&amd756_smbus);
+	if (error) {
+		dev_err(&amd756_smbus.dev, "Physical bus removal "
+			"failed\n");
+		goto ERROR0;
+	}
+
+	/* Define the 5 virtual adapters and algorithms structures */
+	if (!(s4882_adapter = kmalloc(5 * sizeof(struct i2c_adapter),
+				      GFP_KERNEL))) {
+		error = -ENOMEM;
+		goto ERROR1;
+	}
+	if (!(s4882_algo = kmalloc(5 * sizeof(struct i2c_algorithm),
+				   GFP_KERNEL))) {
+		error = -ENOMEM;
+		goto ERROR2;
+	}
+
+	/* Fill in the new structures */
+	s4882_algo[0] = *(amd756_smbus.algo);
+	s4882_algo[0].smbus_xfer = amd756_access_virt0;
+	s4882_adapter[0] = amd756_smbus;
+	s4882_adapter[0].algo = s4882_algo;
+	for (i = 1; i < 5; i++) {
+		s4882_algo[i] = *(amd756_smbus.algo);
+		s4882_adapter[i] = amd756_smbus;
+		sprintf(s4882_adapter[i].name,
+			"SMBus 8111 adapter (CPU%d)", i-1);
+		s4882_adapter[i].algo = s4882_algo+i;
+	}
+	s4882_algo[1].smbus_xfer = amd756_access_virt1;
+	s4882_algo[2].smbus_xfer = amd756_access_virt2;
+	s4882_algo[3].smbus_xfer = amd756_access_virt3;
+	s4882_algo[4].smbus_xfer = amd756_access_virt4;
+
+	/* Configure the PCA9556 multiplexer */
+	ioconfig.byte = 0x00; /* All I/O to output mode */
+	error = amd756_smbus.algo->smbus_xfer(&amd756_smbus, 0x18, 0,
+					      I2C_SMBUS_WRITE, 0x03,
+					      I2C_SMBUS_BYTE_DATA, &ioconfig);
+	if (error) {
+		dev_dbg(&amd756_smbus.dev, "PCA9556 configuration failed\n");
+		error = -EIO;
+		goto ERROR3;
+	}
+
+	/* Register virtual adapters */
+	for (i = 0; i < 5; i++) {
+		error = i2c_add_adapter(s4882_adapter+i);
+		if (error) {
+			dev_err(&amd756_smbus.dev,
+			       "Virtual adapter %d registration "
+			       "failed, module not inserted\n", i);
+			for (i--; i >= 0; i--)
+				i2c_del_adapter(s4882_adapter+i);
+			goto ERROR3;
+		}
+	}
+
+	return 0;
+
+ERROR3:
+	kfree(s4882_algo);
+	s4882_algo = NULL;
+ERROR2:
+	kfree(s4882_adapter);
+	s4882_adapter = NULL;
+ERROR1:
+	i2c_del_adapter(&amd756_smbus);
+ERROR0:
+	return error;
+}
+
+static void __exit amd756_s4882_exit(void)
+{
+	if (s4882_adapter) {
+		int i;
+
+		for (i = 0; i < 5; i++)
+			i2c_del_adapter(s4882_adapter+i);
+		kfree(s4882_adapter);
+		s4882_adapter = NULL;
+	}
+	if (s4882_algo) {
+		kfree(s4882_algo);
+		s4882_algo = NULL;
+	}
+
+	/* Restore physical bus */
+	if (i2c_add_adapter(&amd756_smbus))
+		dev_err(&amd756_smbus.dev, "Physical bus restoration "
+			"failed\n");
+}
+
+MODULE_AUTHOR("Jean Delvare <khali@linux-fr.org>");
+MODULE_DESCRIPTION("S4882 SMBus multiplexing");
+MODULE_LICENSE("GPL");
+
+module_init(amd756_s4882_init);
+module_exit(amd756_s4882_exit);
diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756.c linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756.c
--- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756.c	2004-10-16 21:13:45.000000000 +0200
+++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756.c	2004-10-24 18:51:52.000000000 +0200
@@ -47,6 +47,7 @@
 #include <linux/ioport.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/kmod.h>
 #include <asm/io.h>
 
 /* AMD756 SMBus address offsets */
@@ -302,23 +303,24 @@
 	.functionality	= amd756_func,
 };
 
-static struct i2c_adapter amd756_adapter = {
+struct i2c_adapter amd756_smbus = {
 	.owner		= THIS_MODULE,
 	.class          = I2C_CLASS_HWMON,
 	.algo		= &smbus_algorithm,
 	.name		= "unset",
 };
 
-enum chiptype { AMD756, AMD766, AMD768, NFORCE, AMD8111 };
+enum chiptype { AMD756, AMD766, AMD768, NFORCE, AMD8111, S4882 };
 static const char* chipname[] = {
 	"AMD756", "AMD766", "AMD768",
-	"nVidia nForce", "AMD8111",
+	"nVidia nForce", "AMD8111", "AMD8111",
 };
 
 static struct pci_device_id amd756_ids[] = {
 	{PCI_VENDOR_ID_AMD, 0x740B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD756 },
 	{PCI_VENDOR_ID_AMD, 0x7413, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD766 },
 	{PCI_VENDOR_ID_AMD, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD768 },
+	{PCI_VENDOR_ID_AMD, 0x746B, PCI_VENDOR_ID_AMD, 0x36C0, 0, 0, S4882 },
 	{PCI_VENDOR_ID_AMD, 0x746B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD8111 },
 	{PCI_VENDOR_ID_NVIDIA, 0x01B4, PCI_ANY_ID, PCI_ANY_ID, 0, 0, NFORCE },
 	{ 0, }
@@ -330,6 +332,7 @@
 				  const struct pci_device_id *id)
 {
 	int nforce = (id->driver_data = NFORCE);
+	int s4882 = (id->driver_data = S4882);
 	int error;
 	u8 temp;
 	
@@ -374,18 +377,21 @@
 	dev_dbg(&pdev->dev, "AMD756_smba = 0x%X\n", amd756_ioport);
 
 	/* set up the driverfs linkage to our parent device */
-	amd756_adapter.dev.parent = &pdev->dev;
+	amd756_smbus.dev.parent = &pdev->dev;
 
-	sprintf(amd756_adapter.name, "SMBus %s adapter at %04x",
+	sprintf(amd756_smbus.name, "SMBus %s adapter at %04x",
 		chipname[id->driver_data], amd756_ioport);
 
-	error = i2c_add_adapter(&amd756_adapter);
+	error = i2c_add_adapter(&amd756_smbus);
 	if (error) {
 		dev_err(&pdev->dev,
 			"Adapter registration failed, module not inserted\n");
 		goto out_err;
 	}
 
+	if (s4882)
+		request_module("i2c-amd756-s4882");
+
 	return 0;
 
  out_err:
@@ -395,7 +401,7 @@
 
 static void __devexit amd756_remove(struct pci_dev *dev)
 {
-	i2c_del_adapter(&amd756_adapter);
+	i2c_del_adapter(&amd756_smbus);
 	release_region(amd756_ioport, SMB_IOSIZE);
 }
 
@@ -420,5 +426,7 @@
 MODULE_DESCRIPTION("AMD756/766/768/8111 and nVidia nForce SMBus driver");
 MODULE_LICENSE("GPL");
 
+EXPORT_SYMBOL(amd756_smbus);
+
 module_init(amd756_init)
 module_exit(amd756_exit)


-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Mark Studebaker]

I disagree.
I think the original proposal, dating back several months, is still valid.
The proposal, briefly, elaborates on the 'i2c-virtual' work done previously
which recognizes the PCA955x chip and generates the additional i2c busses,
with locking. This requires an adapter/algorithm pair of drivers and changes
in i2c-core for the locking. I'm sure you already have the email threads
where we've discussed previously.
Your proposal looks less general, more board-specific.
Still catching up, been out of town for a while...
I'll continue to review your emails and code from the past week,
perhaps we can disuss on IRC this weekend.
mds

Jean Delvare wrote:
> Hi Greg,
> 
> Would you accept such a patch? This adds Tyan S4882-specific SMBus
> multiplexing support, in the form of a separate driver, as per Mark
> Hoffman's suggestion.
> 
> Changes to the original driver (i2c-amd756) are kept to a minimum:
> 
> 1* The i2c_adapter structure is exported (some noise due to a struct
> name change, I didn't want to export something as vague as
> "amd756_adapter").
> 
> 2* Autoload of the new driver (i2c-amd756-s4882) when needed. This can
> go away if you don't like it, but I thought it'd be more convenient for
> the end user. BTW, what will happen if the i2c-amd756-s4882 module
> doesn't exist? Should I add some #ifdef magic in the i2c-amd756 driver
> so that the autoload will not happen if the i2c-amd756-s4882 module was
> not selected?
> 
> The i2c-amd756-s4882 driver follows Mark Hoffman's suggestion, but in a
> simplified way. On load:
> 
> 1* Remove the physical adapter from the main i2c adapters list. As a
> side effect, any client gets kicked off.
> 
> 2* Register 5 virtual busses using to wrappers pointing to the original
> SMBus code. Clients can connect to the new adapters where it makes
> sense.
> 
> On unload, same thing but the other way around.
> 
> This approach has a drawback in the case several client drivers are
> already loaded (they will get attached, detached and attached again in a
> raw), but I don't think this is really a problem:
> 
> 1* In most cases bus drivers are loaded before clients anyway.
> 
> 2* SMBus multiplexing is quite rare, and I don't care if people with
> such specific boards experience drawbacks at init time in uncommon
> cases.
> 
> Advantages of the approach, however, are great. Multiplexing code
> belongs to a different driver, which improves maintainability as Mark
> pointed out, and means no impact on module size nor performance for
> regular users. The whole thing is also almost transparent for userspace
> and client drivers, requiring very very little change, if any at all.
> 
> Greg, anyone, comments on the approach and code are welcome. If
> everyone's OK, we'll go that way for both the 2.6 kernel and lm_sensors
> (for 2.4).
> 
> Thanks.
> 
> diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Kconfig linux-2.6.9-rc4-bk1/drivers/i2c/busses/Kconfig
> --- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Kconfig	2004-10-16 19:37:33.000000000 +0200
> +++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/Kconfig	2004-10-24 18:46:53.000000000 +0200
> @@ -51,6 +51,17 @@
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-amd756.
>  
> +config I2C_AMD756_S4882
> +	tristate "SMBus multiplexing on the Tyan S4882"
> +	depends on I2C_AMD756 && EXPERIMENTAL
> +	default y
> +	help
> +	  Enabling this option will add specific SMBus support for the Tyan
> +	  S4882 motherboard.  On this 4-CPU board, the SMBus is multiplexed
> +	  over 8 different channels, where the various memory module EEPROMs
> +	  and temperature sensors live.  Saying yes here will give you access
> +	  to these in addition to the trunk.
> +
>  config I2C_AMD8111
>  	tristate "AMD 8111"
>  	depends on I2C && PCI && EXPERIMENTAL
> diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Makefile linux-2.6.9-rc4-bk1/drivers/i2c/busses/Makefile
> --- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/Makefile	2004-10-12 19:44:25.000000000 +0200
> +++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/Makefile	2004-10-23 22:55:45.000000000 +0200
> @@ -6,6 +6,7 @@
>  obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
>  obj-$(CONFIG_I2C_ALI15X3)	+= i2c-ali15x3.o
>  obj-$(CONFIG_I2C_AMD756)	+= i2c-amd756.o
> +obj-$(CONFIG_I2C_AMD756_S4882)	+= i2c-amd756-s4882.o
>  obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
>  obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
> diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756-s4882.c linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756-s4882.c
> --- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756-s4882.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756-s4882.c	2004-10-24 18:42:12.000000000 +0200
> @@ -0,0 +1,266 @@
> +/*
> + * i2c-amd756-s4882.c - i2c-amd756 extras for the Tyan S4882 motherboard
> + *
> + * Copyright (C) 2004 Jean Delvare <khali@linux-fr.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.
> + */
> + 
> +/*
> + * We select the channels by sending commands to the Philips
> + * PCA9556 chip at I2C address 0x18. The main adapter is used for
> + * the non-multiplexed part of the bus, and 4 virtual adapters
> + * are defined for the multiplexed addresses: 0x50-0x53 (memory
> + * module EEPROM) located on channels 1-4, and 0x4c (LM63)
> + * located on multiplexed channels 0 and 5-7. We define one
> + * virtual adapter per CPU, which corresponds to two multiplexed
> + * channels:
> + *   CPU0: virtual adapter 1, channels 1 and 0
> + *   CPU1: virtual adapter 2, channels 2 and 5
> + *   CPU2: virtual adapter 3, channels 3 and 6
> + *   CPU3: virtual adapter 4, channels 4 and 7
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +
> +extern struct i2c_adapter amd756_smbus;
> +
> +static struct i2c_adapter *s4882_adapter;
> +static struct i2c_algorithm *s4882_algo;
> +
> +/* Wrapper access functions for multiplexed SMBus */
> +static struct semaphore amd756_lock;
> +
> +static s32 amd756_access_virt0(struct i2c_adapter * adap, u16 addr,
> +			       unsigned short flags, char read_write,
> +			       u8 command, int size,
> +			       union i2c_smbus_data * data)
> +{
> +	int error;
> +
> +	/* We exclude the multiplexed addresses */
> +	if (addr = 0x4c || (addr & 0xfc) = 0x50 || (addr & 0xfc) = 0x30
> +	 || addr = 0x18)
> +		return -1;
> +
> +	down(&amd756_lock);
> +
> +	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
> +					      command, size, data);
> +
> +	up(&amd756_lock);
> +
> +	return error;
> +}
> +
> +/* We remember the last used channels combination so as to only switch
> +   channels when it is really needed. This greatly reduces the SMBus
> +   overhead, but also assumes that nobody will be writing to the PCA9556
> +   in our back. */
> +static u8 last_channels;
> +
> +static inline s32 amd756_access_channel(struct i2c_adapter * adap, u16 addr,
> +					unsigned short flags, char read_write,
> +					u8 command, int size,
> +					union i2c_smbus_data * data,
> +					u8 channels)
> +{
> +	int error;
> +
> +	/* We exclude the non-multiplexed addresses */
> +	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
> +		return -1;
> +
> +	down(&amd756_lock);
> +
> +	if (last_channels != channels) {
> +		union i2c_smbus_data mplxdata;
> +		mplxdata.byte = channels;
> +
> +		error = amd756_smbus.algo->smbus_xfer(adap, 0x18, 0,
> +						      I2C_SMBUS_WRITE, 0x01,
> +						      I2C_SMBUS_BYTE_DATA,
> +						      &mplxdata);
> +		if (error)
> +			goto UNLOCK;
> +		last_channels = channels;
> +	}
> +	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
> +					      command, size, data);
> +
> +UNLOCK:
> +	up(&amd756_lock);
> +	return error;
> +}
> +
> +static s32 amd756_access_virt1(struct i2c_adapter * adap, u16 addr,
> +			       unsigned short flags, char read_write,
> +			       u8 command, int size,
> +			       union i2c_smbus_data * data)
> +{
> +	/* CPU0: channels 1 and 0 enabled */
> +	return amd756_access_channel(adap, addr, flags, read_write, command,
> +				     size, data, 0x03);
> +}
> +
> +static s32 amd756_access_virt2(struct i2c_adapter * adap, u16 addr,
> +			       unsigned short flags, char read_write,
> +			       u8 command, int size,
> +			       union i2c_smbus_data * data)
> +{
> +	/* CPU1: channels 2 and 5 enabled */
> +	return amd756_access_channel(adap, addr, flags, read_write, command,
> +				     size, data, 0x24);
> +}
> +
> +static s32 amd756_access_virt3(struct i2c_adapter * adap, u16 addr,
> +			       unsigned short flags, char read_write,
> +			       u8 command, int size,
> +			       union i2c_smbus_data * data)
> +{
> +	/* CPU2: channels 3 and 6 enabled */
> +	return amd756_access_channel(adap, addr, flags, read_write, command,
> +				     size, data, 0x48);
> +}
> +
> +static s32 amd756_access_virt4(struct i2c_adapter * adap, u16 addr,
> +			       unsigned short flags, char read_write,
> +			       u8 command, int size,
> +			       union i2c_smbus_data * data)
> +{
> +	/* CPU3: channels 4 and 7 enabled */
> +	return amd756_access_channel(adap, addr, flags, read_write, command,
> +				     size, data, 0x90);
> +}
> +
> +static int __init amd756_s4882_init(void)
> +{
> +	int i, error;
> +	union i2c_smbus_data ioconfig;
> +
> +	if (pci_find_subsys(PCI_VENDOR_ID_AMD, 0x746B, PCI_VENDOR_ID_AMD,
> +			    0x36C0, NULL) = NULL)
> +		return -ENODEV;
> +
> +	printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4882\n");
> +
> +	init_MUTEX(&amd756_lock);
> +
> +	/* Unregister physical bus */
> +	error = i2c_del_adapter(&amd756_smbus);
> +	if (error) {
> +		dev_err(&amd756_smbus.dev, "Physical bus removal "
> +			"failed\n");
> +		goto ERROR0;
> +	}
> +
> +	/* Define the 5 virtual adapters and algorithms structures */
> +	if (!(s4882_adapter = kmalloc(5 * sizeof(struct i2c_adapter),
> +				      GFP_KERNEL))) {
> +		error = -ENOMEM;
> +		goto ERROR1;
> +	}
> +	if (!(s4882_algo = kmalloc(5 * sizeof(struct i2c_algorithm),
> +				   GFP_KERNEL))) {
> +		error = -ENOMEM;
> +		goto ERROR2;
> +	}
> +
> +	/* Fill in the new structures */
> +	s4882_algo[0] = *(amd756_smbus.algo);
> +	s4882_algo[0].smbus_xfer = amd756_access_virt0;
> +	s4882_adapter[0] = amd756_smbus;
> +	s4882_adapter[0].algo = s4882_algo;
> +	for (i = 1; i < 5; i++) {
> +		s4882_algo[i] = *(amd756_smbus.algo);
> +		s4882_adapter[i] = amd756_smbus;
> +		sprintf(s4882_adapter[i].name,
> +			"SMBus 8111 adapter (CPU%d)", i-1);
> +		s4882_adapter[i].algo = s4882_algo+i;
> +	}
> +	s4882_algo[1].smbus_xfer = amd756_access_virt1;
> +	s4882_algo[2].smbus_xfer = amd756_access_virt2;
> +	s4882_algo[3].smbus_xfer = amd756_access_virt3;
> +	s4882_algo[4].smbus_xfer = amd756_access_virt4;
> +
> +	/* Configure the PCA9556 multiplexer */
> +	ioconfig.byte = 0x00; /* All I/O to output mode */
> +	error = amd756_smbus.algo->smbus_xfer(&amd756_smbus, 0x18, 0,
> +					      I2C_SMBUS_WRITE, 0x03,
> +					      I2C_SMBUS_BYTE_DATA, &ioconfig);
> +	if (error) {
> +		dev_dbg(&amd756_smbus.dev, "PCA9556 configuration failed\n");
> +		error = -EIO;
> +		goto ERROR3;
> +	}
> +
> +	/* Register virtual adapters */
> +	for (i = 0; i < 5; i++) {
> +		error = i2c_add_adapter(s4882_adapter+i);
> +		if (error) {
> +			dev_err(&amd756_smbus.dev,
> +			       "Virtual adapter %d registration "
> +			       "failed, module not inserted\n", i);
> +			for (i--; i >= 0; i--)
> +				i2c_del_adapter(s4882_adapter+i);
> +			goto ERROR3;
> +		}
> +	}
> +
> +	return 0;
> +
> +ERROR3:
> +	kfree(s4882_algo);
> +	s4882_algo = NULL;
> +ERROR2:
> +	kfree(s4882_adapter);
> +	s4882_adapter = NULL;
> +ERROR1:
> +	i2c_del_adapter(&amd756_smbus);
> +ERROR0:
> +	return error;
> +}
> +
> +static void __exit amd756_s4882_exit(void)
> +{
> +	if (s4882_adapter) {
> +		int i;
> +
> +		for (i = 0; i < 5; i++)
> +			i2c_del_adapter(s4882_adapter+i);
> +		kfree(s4882_adapter);
> +		s4882_adapter = NULL;
> +	}
> +	if (s4882_algo) {
> +		kfree(s4882_algo);
> +		s4882_algo = NULL;
> +	}
> +
> +	/* Restore physical bus */
> +	if (i2c_add_adapter(&amd756_smbus))
> +		dev_err(&amd756_smbus.dev, "Physical bus restoration "
> +			"failed\n");
> +}
> +
> +MODULE_AUTHOR("Jean Delvare <khali@linux-fr.org>");
> +MODULE_DESCRIPTION("S4882 SMBus multiplexing");
> +MODULE_LICENSE("GPL");
> +
> +module_init(amd756_s4882_init);
> +module_exit(amd756_s4882_exit);
> diff -ruN linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756.c linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756.c
> --- linux-2.6.9-rc4-bk1/drivers/i2c/busses.orig/i2c-amd756.c	2004-10-16 21:13:45.000000000 +0200
> +++ linux-2.6.9-rc4-bk1/drivers/i2c/busses/i2c-amd756.c	2004-10-24 18:51:52.000000000 +0200
> @@ -47,6 +47,7 @@
>  #include <linux/ioport.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/kmod.h>
>  #include <asm/io.h>
>  
>  /* AMD756 SMBus address offsets */
> @@ -302,23 +303,24 @@
>  	.functionality	= amd756_func,
>  };
>  
> -static struct i2c_adapter amd756_adapter = {
> +struct i2c_adapter amd756_smbus = {
>  	.owner		= THIS_MODULE,
>  	.class          = I2C_CLASS_HWMON,
>  	.algo		= &smbus_algorithm,
>  	.name		= "unset",
>  };
>  
> -enum chiptype { AMD756, AMD766, AMD768, NFORCE, AMD8111 };
> +enum chiptype { AMD756, AMD766, AMD768, NFORCE, AMD8111, S4882 };
>  static const char* chipname[] = {
>  	"AMD756", "AMD766", "AMD768",
> -	"nVidia nForce", "AMD8111",
> +	"nVidia nForce", "AMD8111", "AMD8111",
>  };
>  
>  static struct pci_device_id amd756_ids[] = {
>  	{PCI_VENDOR_ID_AMD, 0x740B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD756 },
>  	{PCI_VENDOR_ID_AMD, 0x7413, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD766 },
>  	{PCI_VENDOR_ID_AMD, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD768 },
> +	{PCI_VENDOR_ID_AMD, 0x746B, PCI_VENDOR_ID_AMD, 0x36C0, 0, 0, S4882 },
>  	{PCI_VENDOR_ID_AMD, 0x746B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, AMD8111 },
>  	{PCI_VENDOR_ID_NVIDIA, 0x01B4, PCI_ANY_ID, PCI_ANY_ID, 0, 0, NFORCE },
>  	{ 0, }
> @@ -330,6 +332,7 @@
>  				  const struct pci_device_id *id)
>  {
>  	int nforce = (id->driver_data = NFORCE);
> +	int s4882 = (id->driver_data = S4882);
>  	int error;
>  	u8 temp;
>  	
> @@ -374,18 +377,21 @@
>  	dev_dbg(&pdev->dev, "AMD756_smba = 0x%X\n", amd756_ioport);
>  
>  	/* set up the driverfs linkage to our parent device */
> -	amd756_adapter.dev.parent = &pdev->dev;
> +	amd756_smbus.dev.parent = &pdev->dev;
>  
> -	sprintf(amd756_adapter.name, "SMBus %s adapter at %04x",
> +	sprintf(amd756_smbus.name, "SMBus %s adapter at %04x",
>  		chipname[id->driver_data], amd756_ioport);
>  
> -	error = i2c_add_adapter(&amd756_adapter);
> +	error = i2c_add_adapter(&amd756_smbus);
>  	if (error) {
>  		dev_err(&pdev->dev,
>  			"Adapter registration failed, module not inserted\n");
>  		goto out_err;
>  	}
>  
> +	if (s4882)
> +		request_module("i2c-amd756-s4882");
> +
>  	return 0;
>  
>   out_err:
> @@ -395,7 +401,7 @@
>  
>  static void __devexit amd756_remove(struct pci_dev *dev)
>  {
> -	i2c_del_adapter(&amd756_adapter);
> +	i2c_del_adapter(&amd756_smbus);
>  	release_region(amd756_ioport, SMB_IOSIZE);
>  }
>  
> @@ -420,5 +426,7 @@
>  MODULE_DESCRIPTION("AMD756/766/768/8111 and nVidia nForce SMBus driver");
>  MODULE_LICENSE("GPL");
>  
> +EXPORT_SYMBOL(amd756_smbus);
> +
>  module_init(amd756_init)
>  module_exit(amd756_exit)
> 
> 

More on SMBus multiplexing

[Jean Delvare]

Hi Mark,

> I disagree.
> I think the original proposal, dating back several months, is still valid.
> The proposal, briefly, elaborates on the 'i2c-virtual' work done previously
> which recognizes the PCA955x chip and generates the additional i2c busses,
> with locking. This requires an adapter/algorithm pair of drivers and
> changes in i2c-core for the locking. I'm sure you already have the email
> threads where we've discussed previously.
> Your proposal looks less general, more board-specific.

Yes, I did read the i2c-virtual proposal. It looked interesting because
it adds an architecture for the whole concept of SMBus multiplexing.
However, it has several drawbacks:

1* It affects the core. It is certainly acceptable for Linux 2.6
(providing it actually proves to be useful) but I don't think such
changes belong to 2.4/CVS where compatibility and stability are (well,
should be) our main goal.

2* SMBus multiplexing *is* board-specific, no matter how you look at it.
For I2C multiplexers which are controllable with I2C commands (such as
the PCA9540), autodetection may be used (but see below). However, on the
S4882, multiplexing is done using 3 chips. The multiplexing control is
done by a PCA9556, which is an 8-pin GPIO chip. These pins (4 and 4) are
connected to two PCA9516 chips which do the real SMBus multiplexing job
according to the value of their input pins. You have to realize that the
PCA9516 chips could be controlled in a completely different way
(Super-I/O chips have GPIO pins which could be used for this for
example). And the PCA9556 chip could be controlling anything, not
necessarily SMBus multiplexers. This setup is simply not detectable.
Additionally, there are possible optimizations that cannot be done
automatically. For example, for the S4882, I merged virtual busses by
pairs, so as to have one virtual bus per CPU, which is both more
convenient for the user and less resource consuming.

3* Multiplexer controllers are usually hard, if not impossible, to
detect. The PCA9540 has a single register which is accessed with
smbus_{read,write}_byte commands, NOT smbus_{read,write}_byte_data
commands. The only thing that saves us for this one is its relatively
rare address (0x70). PCA9556 chips have four registers, no
identification register and eight possible addresses. I think that my
heuristics to detect it in sensors-detect are acceptable, but this
highlights the fact that we could face multiplexer chips that are simply
not detectable. This adds up to the fact that multiplexers controller
are not necessarily chips on the SMBus.

4* The previous proposal is around for quite some times now, but was
never merged in. If I remember correctly you had implementation
objections, which were never addressed.

5* There are only a couple boards with multiplexing out there, and I can
only hope that there won't be too many more in the future. Multiplexing
makes things slower and more complex, as everyone can easily understand.
I'm not sure it's worth to add things at the core level for just a
handful of boards.

So, I understand that you like defining clean interfaces for new things,
I do to. However, I don't see any added value in this specific case.
This won't allow for fully automatic detection. This won't allow for
code refactoring either, since each chip can be used in different ways.
And there isn't that much code to refactor either. In the case of the
S4882, the multiplexing control is just one smbus transfer for initial
configuration, and one smbus transfer for bus switching. If we really
want to refactor code, we could add an helper function in i2c-core (or
whatever) that creates N virtual busses from one physical bus.

I am personally easier with refactoring code afterwards, when we know
what can be moved in the common part,

My question was about whether my code was acceptable in the current
context, not whether there were prefered (but more complex) ways to
achieve the same goal. If anyone comes with a more generic
implementation at a later which everyone enjoys, I will of course update
my code to make use of it.

> I'll continue to review your emails and code from the past week,
> perhaps we can disuss on IRC this weekend.

Thanks. Note that I will not be able to spend too much time on IRC this
weekend though, since I will not be at home all the time. I'll take my
laptop with me and connect as time and technical conditions permit.

Thanks again,
Jean

More on SMBus multiplexing

[Mark Studebaker]

thanks for the detailed response. comments below

Jean Delvare wrote:
> Hi Mark,
> 
> 
>>I disagree.
>>I think the original proposal, dating back several months, is still valid.
>>The proposal, briefly, elaborates on the 'i2c-virtual' work done previously
>>which recognizes the PCA955x chip and generates the additional i2c busses,
>>with locking. This requires an adapter/algorithm pair of drivers and
>>changes in i2c-core for the locking. I'm sure you already have the email
>>threads where we've discussed previously.
>>Your proposal looks less general, more board-specific.
> 
> 
> Yes, I did read the i2c-virtual proposal. It looked interesting because
> it adds an architecture for the whole concept of SMBus multiplexing.
> However, it has several drawbacks:
> 
> 1* It affects the core. It is certainly acceptable for Linux 2.6
> (providing it actually proves to be useful) but I don't think such
> changes belong to 2.4/CVS where compatibility and stability are (well,
> should be) our main goal.
> 

If there's a way to do locking without affecting the core I'm all for it.
If there's a way to do it without locking at all, even better.
I'll look further into what you did to better understand your approach.
At the moment I don't understand how you avoided i2c-core changes :)

> 2* SMBus multiplexing *is* board-specific, no matter how you look at it.
> For I2C multiplexers which are controllable with I2C commands (such as
> the PCA9540), autodetection may be used (but see below). However, on the
> S4882, multiplexing is done using 3 chips. The multiplexing control is
> done by a PCA9556, which is an 8-pin GPIO chip. These pins (4 and 4) are
> connected to two PCA9516 chips which do the real SMBus multiplexing job
> according to the value of their input pins. You have to realize that the
> PCA9516 chips could be controlled in a completely different way
> (Super-I/O chips have GPIO pins which could be used for this for
> example). And the PCA9556 chip could be controlling anything, not
> necessarily SMBus multiplexers. This setup is simply not detectable.
> Additionally, there are possible optimizations that cannot be done
> automatically. For example, for the S4882, I merged virtual busses by
> pairs, so as to have one virtual bus per CPU, which is both more
> convenient for the user and less resource consuming.
> 
agreed.
The power of the i2c-virtual approach is that it's separated into
two pieces - algorithm and adapter.
The algorithm is the generic mux code (and locking?).
Perhaps a better name than i2c-virtual for that module is i2c-mux.
The adapter is the board-specific part. 
By separating the two, the board-specific (adapter) part is simple
and easy to clone into multiple versions for different boards.

> 3* Multiplexer controllers are usually hard, if not impossible, to
> detect. The PCA9540 has a single register which is accessed with
> smbus_{read,write}_byte commands, NOT smbus_{read,write}_byte_data
> commands. The only thing that saves us for this one is its relatively
> rare address (0x70). PCA9556 chips have four registers, no
> identification register and eight possible addresses. I think that my
> heuristics to detect it in sensors-detect are acceptable, but this
> highlights the fact that we could face multiplexer chips that are simply
> not detectable. This adds up to the fact that multiplexers controller
> are not necessarily chips on the SMBus.
> 
agreed.
some adapter modules may contain rudamentary detection
(recognition of address 0x70, for example), others
may have none, and activate on module loading
perhaps with a module parameter to identify "root" bus number?)

> 4* The previous proposal is around for quite some times now, but was
> never merged in. If I remember correctly you had implementation
> objections, which were never addressed.
> 
Because these boards are rare (and I don't have access to one).
What I call "i2c-virtual" I really mean
"the original i2c-virtual implementation plus my
suggestions" which you are right, were never addressed.

> 5* There are only a couple boards with multiplexing out there, and I can
> only hope that there won't be too many more in the future. Multiplexing
> makes things slower and more complex, as everyone can easily understand.
> I'm not sure it's worth to add things at the core level for just a
> handful of boards.
> 
agreed. the less core mods the better.

> So, I understand that you like defining clean interfaces for new things,
> I do to. However, I don't see any added value in this specific case.
> This won't allow for fully automatic detection. This won't allow for
> code refactoring either, since each chip can be used in different ways.
> And there isn't that much code to refactor either. In the case of the
> S4882, the multiplexing control is just one smbus transfer for initial
> configuration, and one smbus transfer for bus switching. If we really
> want to refactor code, we could add an helper function in i2c-core (or
> whatever) that creates N virtual busses from one physical bus.
> 
> I am personally easier with refactoring code afterwards, when we know
> what can be moved in the common part,
> 
> My question was about whether my code was acceptable in the current
> context, not whether there were prefered (but more complex) ways to
> achieve the same goal. If anyone comes with a more generic
> implementation at a later which everyone enjoys, I will of course update
> my code to make use of it.
> 
> 
>>I'll continue to review your emails and code from the past week,
>>perhaps we can disuss on IRC this weekend.
> 
> 
> Thanks. Note that I will not be able to spend too much time on IRC this
> weekend though, since I will not be at home all the time. I'll take my
> laptop with me and connect as time and technical conditions permit.
> 
thanks for letting me comment vaguely and then elaborate later
without getting cranky, I appreciate it...
don't lug a laptop around Paris just for me :)

> Thanks again,
> Jean
> 

More on SMBus multiplexing

[Mark Studebaker]

Khali and I discussed this at length this weekend on IRC.
We agree on a general direction toward a generic implementation,
modeled in part on i2c-virtual.
In the meantime, we agree that his i2c-amd756-4882 module is straightforward
is headed in the right direction, and is acceptable for 2.6;
so I no longer object to his patch.
In the near future I'll be working on cleaning up the original i2c-virtual implmementation.

mds



Jean Delvare wrote:
> Hi Mark,
> 
> 
>>I disagree.
>>I think the original proposal, dating back several months, is still valid.
>>The proposal, briefly, elaborates on the 'i2c-virtual' work done previously
>>which recognizes the PCA955x chip and generates the additional i2c busses,
>>with locking. This requires an adapter/algorithm pair of drivers and
>>changes in i2c-core for the locking. I'm sure you already have the email
>>threads where we've discussed previously.
>>Your proposal looks less general, more board-specific.
> 
> 
> Yes, I did read the i2c-virtual proposal. It looked interesting because
> it adds an architecture for the whole concept of SMBus multiplexing.
> However, it has several drawbacks:
> 
> 1* It affects the core. It is certainly acceptable for Linux 2.6
> (providing it actually proves to be useful) but I don't think such
> changes belong to 2.4/CVS where compatibility and stability are (well,
> should be) our main goal.
> 
> 2* SMBus multiplexing *is* board-specific, no matter how you look at it.
> For I2C multiplexers which are controllable with I2C commands (such as
> the PCA9540), autodetection may be used (but see below). However, on the
> S4882, multiplexing is done using 3 chips. The multiplexing control is
> done by a PCA9556, which is an 8-pin GPIO chip. These pins (4 and 4) are
> connected to two PCA9516 chips which do the real SMBus multiplexing job
> according to the value of their input pins. You have to realize that the
> PCA9516 chips could be controlled in a completely different way
> (Super-I/O chips have GPIO pins which could be used for this for
> example). And the PCA9556 chip could be controlling anything, not
> necessarily SMBus multiplexers. This setup is simply not detectable.
> Additionally, there are possible optimizations that cannot be done
> automatically. For example, for the S4882, I merged virtual busses by
> pairs, so as to have one virtual bus per CPU, which is both more
> convenient for the user and less resource consuming.
> 
> 3* Multiplexer controllers are usually hard, if not impossible, to
> detect. The PCA9540 has a single register which is accessed with
> smbus_{read,write}_byte commands, NOT smbus_{read,write}_byte_data
> commands. The only thing that saves us for this one is its relatively
> rare address (0x70). PCA9556 chips have four registers, no
> identification register and eight possible addresses. I think that my
> heuristics to detect it in sensors-detect are acceptable, but this
> highlights the fact that we could face multiplexer chips that are simply
> not detectable. This adds up to the fact that multiplexers controller
> are not necessarily chips on the SMBus.
> 
> 4* The previous proposal is around for quite some times now, but was
> never merged in. If I remember correctly you had implementation
> objections, which were never addressed.
> 
> 5* There are only a couple boards with multiplexing out there, and I can
> only hope that there won't be too many more in the future. Multiplexing
> makes things slower and more complex, as everyone can easily understand.
> I'm not sure it's worth to add things at the core level for just a
> handful of boards.
> 
> So, I understand that you like defining clean interfaces for new things,
> I do to. However, I don't see any added value in this specific case.
> This won't allow for fully automatic detection. This won't allow for
> code refactoring either, since each chip can be used in different ways.
> And there isn't that much code to refactor either. In the case of the
> S4882, the multiplexing control is just one smbus transfer for initial
> configuration, and one smbus transfer for bus switching. If we really
> want to refactor code, we could add an helper function in i2c-core (or
> whatever) that creates N virtual busses from one physical bus.
> 
> I am personally easier with refactoring code afterwards, when we know
> what can be moved in the common part,
> 
> My question was about whether my code was acceptable in the current
> context, not whether there were prefered (but more complex) ways to
> achieve the same goal. If anyone comes with a more generic
> implementation at a later which everyone enjoys, I will of course update
> my code to make use of it.
> 
> 
>>I'll continue to review your emails and code from the past week,
>>perhaps we can disuss on IRC this weekend.
> 
> 
> Thanks. Note that I will not be able to spend too much time on IRC this
> weekend though, since I will not be at home all the time. I'll take my
> laptop with me and connect as time and technical conditions permit.
> 
> Thanks again,
> Jean
> 

More on SMBus multiplexing

[Jean Delvare]

Quoting Mark D. Studebaker:

> Khali and I discussed this at length this weekend on IRC.
> We agree on a general direction toward a generic implementation,
> modeled in part on i2c-virtual.

True :)

> In the meantime, we agree that his i2c-amd756-4882 module is
> straightforward is headed in the right direction, and is acceptable
> for 2.6; so I no longer object to his patch.

Greg, please apply the patch then, unless you have personal objections.
(I may resend it with a Signed-off-by line if you want, or just add it
yourself.)

> In the near future I'll be working on cleaning up the original
> i2c-virtual implmementation.

Sounds good. Maybe you can work on this with Mark M. Hoffman since he
seems to be interested as well.

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Jean Delvare]

Quoting myself:

> Greg, please apply the patch then, unless you have personal
> objections.(I may resend it with a Signed-off-by line if you want, or
> just add it yourself.)

On second thought, don't. As I said on LKML this morning, my autoload
code doesn't work and we will have to stick to manual loading for now.
I'll provide a simplified patch that does this tomorrow.

Thanks and sorry for the trouble.

-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Greg KH]

On Wed, Nov 03, 2004 at 11:07:47PM +0100, Jean Delvare wrote:
> Quoting myself:
> 
> > Greg, please apply the patch then, unless you have personal
> > objections.(I may resend it with a Signed-off-by line if you want, or
> > just add it yourself.)
> 
> On second thought, don't. As I said on LKML this morning, my autoload
> code doesn't work and we will have to stick to manual loading for now.
> I'll provide a simplified patch that does this tomorrow.
> 
> Thanks and sorry for the trouble.

Heh, glad I've been putting i2c patches off for a while, it payed off
here :)

thanks,

greg k-h

More on SMBus multiplexing

[Jean Delvare]

Hi Greg,

> > On second thought, don't. As I said on LKML this morning, my autoload
> > code doesn't work and we will have to stick to manual loading for now.
> > I'll provide a simplified patch that does this tomorrow.
> >
> > Thanks and sorry for the trouble.
>
> Heh, glad I've been putting i2c patches off for a while, it payed off
> here :)

Hehe, true enough. That said, I think you have been curiously silent
about the SMBus multiplexing issue. Care to tell me if the last approach
I am proposing (minus the autoloading) is fine with you? If you don't
want to comment on the i2c part of it, please at least tell me if it is
acceptable kernel-wise.

Thanks,
Jean

More on SMBus multiplexing

[Greg KH]

On Thu, Nov 04, 2004 at 09:53:17AM +0100, Jean Delvare wrote:
> 
> Hi Greg,
> 
> > > On second thought, don't. As I said on LKML this morning, my autoload
> > > code doesn't work and we will have to stick to manual loading for now.
> > > I'll provide a simplified patch that does this tomorrow.
> > >
> > > Thanks and sorry for the trouble.
> >
> > Heh, glad I've been putting i2c patches off for a while, it payed off
> > here :)
> 
> Hehe, true enough. That said, I think you have been curiously silent
> about the SMBus multiplexing issue. Care to tell me if the last approach
> I am proposing (minus the autoloading) is fine with you? If you don't
> want to comment on the i2c part of it, please at least tell me if it is
> acceptable kernel-wise.

It looks fine kernel-wise from what I can tell.  Sorry about being
quiet, been busy in other parts of the kernel lately that happen to have
bigger fires to put out (stupid usb suspend issues...)

thanks,

greg k-h

More on SMBus multiplexing

[Jean Delvare]

Quoting myself:

> On second thought, don't. As I said on LKML this morning, my autoload
> code doesn't work and we will have to stick to manual loading for now.
> I'll provide a simplified patch that does this tomorrow.

We are not exactly tomorrow but here is the modified patch. Please note
that this is only safe tp apply *after* my other patch that adds extra
checks to i2c_del_adapter.

Please apply,
Thanks.

Signed-off-by: Jean Delvare <khali@linux-fr.org>

diff -ruN drivers/i2c/busses.orig/Kconfig drivers/i2c/busses/Kconfig
--- drivers/i2c/busses.orig/Kconfig	2004-10-26 20:51:51.000000000 +0200
+++ drivers/i2c/busses/Kconfig	2004-11-04 20:47:38.000000000 +0100
@@ -51,6 +51,19 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-amd756.
 
+config I2C_AMD756_S4882
+	tristate "SMBus multiplexing on the Tyan S4882"
+	depends on I2C_AMD756 && EXPERIMENTAL
+	help
+	  Enabling this option will add specific SMBus support for the Tyan
+	  S4882 motherboard.  On this 4-CPU board, the SMBus is multiplexed
+	  over 8 different channels, where the various memory module EEPROMs
+	  and temperature sensors live.  Saying yes here will give you access
+	  to these in addition to the trunk.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-amd756-s4882.
+
 config I2C_AMD8111
 	tristate "AMD 8111"
 	depends on I2C && PCI && EXPERIMENTAL
diff -ruN drivers/i2c/busses.orig/Makefile drivers/i2c/busses/Makefile
--- drivers/i2c/busses.orig/Makefile	2004-10-26 20:48:19.000000000 +0200
+++ drivers/i2c/busses/Makefile	2004-11-04 19:57:31.000000000 +0100
@@ -6,6 +6,7 @@
 obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
 obj-$(CONFIG_I2C_ALI15X3)	+= i2c-ali15x3.o
 obj-$(CONFIG_I2C_AMD756)	+= i2c-amd756.o
+obj-$(CONFIG_I2C_AMD756_S4882)	+= i2c-amd756-s4882.o
 obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
diff -ruN drivers/i2c/busses.orig/i2c-amd756-s4882.c drivers/i2c/busses/i2c-amd756-s4882.c
--- drivers/i2c/busses.orig/i2c-amd756-s4882.c	1970-01-01 01:00:00.000000000 +0100
+++ drivers/i2c/busses/i2c-amd756-s4882.c	2004-11-06 20:42:55.000000000 +0100
@@ -0,0 +1,261 @@
+/*
+ * i2c-amd756-s4882.c - i2c-amd756 extras for the Tyan S4882 motherboard
+ *
+ * Copyright (C) 2004 Jean Delvare <khali@linux-fr.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.
+ */
+ 
+/*
+ * We select the channels by sending commands to the Philips
+ * PCA9556 chip at I2C address 0x18. The main adapter is used for
+ * the non-multiplexed part of the bus, and 4 virtual adapters
+ * are defined for the multiplexed addresses: 0x50-0x53 (memory
+ * module EEPROM) located on channels 1-4, and 0x4c (LM63)
+ * located on multiplexed channels 0 and 5-7. We define one
+ * virtual adapter per CPU, which corresponds to two multiplexed
+ * channels:
+ *   CPU0: virtual adapter 1, channels 1 and 0
+ *   CPU1: virtual adapter 2, channels 2 and 5
+ *   CPU2: virtual adapter 3, channels 3 and 6
+ *   CPU3: virtual adapter 4, channels 4 and 7
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+
+extern struct i2c_adapter amd756_smbus;
+
+static struct i2c_adapter *s4882_adapter;
+static struct i2c_algorithm *s4882_algo;
+
+/* Wrapper access functions for multiplexed SMBus */
+static struct semaphore amd756_lock;
+
+static s32 amd756_access_virt0(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	int error;
+
+	/* We exclude the multiplexed addresses */
+	if (addr = 0x4c || (addr & 0xfc) = 0x50 || (addr & 0xfc) = 0x30
+	 || addr = 0x18)
+		return -1;
+
+	down(&amd756_lock);
+
+	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
+					      command, size, data);
+
+	up(&amd756_lock);
+
+	return error;
+}
+
+/* We remember the last used channels combination so as to only switch
+   channels when it is really needed. This greatly reduces the SMBus
+   overhead, but also assumes that nobody will be writing to the PCA9556
+   in our back. */
+static u8 last_channels;
+
+static inline s32 amd756_access_channel(struct i2c_adapter * adap, u16 addr,
+					unsigned short flags, char read_write,
+					u8 command, int size,
+					union i2c_smbus_data * data,
+					u8 channels)
+{
+	int error;
+
+	/* We exclude the non-multiplexed addresses */
+	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
+		return -1;
+
+	down(&amd756_lock);
+
+	if (last_channels != channels) {
+		union i2c_smbus_data mplxdata;
+		mplxdata.byte = channels;
+
+		error = amd756_smbus.algo->smbus_xfer(adap, 0x18, 0,
+						      I2C_SMBUS_WRITE, 0x01,
+						      I2C_SMBUS_BYTE_DATA,
+						      &mplxdata);
+		if (error)
+			goto UNLOCK;
+		last_channels = channels;
+	}
+	error = amd756_smbus.algo->smbus_xfer(adap, addr, flags, read_write,
+					      command, size, data);
+
+UNLOCK:
+	up(&amd756_lock);
+	return error;
+}
+
+static s32 amd756_access_virt1(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU0: channels 1 and 0 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x03);
+}
+
+static s32 amd756_access_virt2(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU1: channels 2 and 5 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x24);
+}
+
+static s32 amd756_access_virt3(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU2: channels 3 and 6 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x48);
+}
+
+static s32 amd756_access_virt4(struct i2c_adapter * adap, u16 addr,
+			       unsigned short flags, char read_write,
+			       u8 command, int size,
+			       union i2c_smbus_data * data)
+{
+	/* CPU3: channels 4 and 7 enabled */
+	return amd756_access_channel(adap, addr, flags, read_write, command,
+				     size, data, 0x90);
+}
+
+static int __init amd756_s4882_init(void)
+{
+	int i, error;
+	union i2c_smbus_data ioconfig;
+
+	/* Unregister physical bus */
+	error = i2c_del_adapter(&amd756_smbus);
+	if (error) {
+		if (error != -EINVAL)
+			dev_err(&amd756_smbus.dev, "Physical bus removal "
+				"failed\n");
+		goto ERROR0;
+	}
+
+	printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4882\n");
+	init_MUTEX(&amd756_lock);
+
+	/* Define the 5 virtual adapters and algorithms structures */
+	if (!(s4882_adapter = kmalloc(5 * sizeof(struct i2c_adapter),
+				      GFP_KERNEL))) {
+		error = -ENOMEM;
+		goto ERROR1;
+	}
+	if (!(s4882_algo = kmalloc(5 * sizeof(struct i2c_algorithm),
+				   GFP_KERNEL))) {
+		error = -ENOMEM;
+		goto ERROR2;
+	}
+
+	/* Fill in the new structures */
+	s4882_algo[0] = *(amd756_smbus.algo);
+	s4882_algo[0].smbus_xfer = amd756_access_virt0;
+	s4882_adapter[0] = amd756_smbus;
+	s4882_adapter[0].algo = s4882_algo;
+	for (i = 1; i < 5; i++) {
+		s4882_algo[i] = *(amd756_smbus.algo);
+		s4882_adapter[i] = amd756_smbus;
+		sprintf(s4882_adapter[i].name,
+			"SMBus 8111 adapter (CPU%d)", i-1);
+		s4882_adapter[i].algo = s4882_algo+i;
+	}
+	s4882_algo[1].smbus_xfer = amd756_access_virt1;
+	s4882_algo[2].smbus_xfer = amd756_access_virt2;
+	s4882_algo[3].smbus_xfer = amd756_access_virt3;
+	s4882_algo[4].smbus_xfer = amd756_access_virt4;
+
+	/* Configure the PCA9556 multiplexer */
+	ioconfig.byte = 0x00; /* All I/O to output mode */
+	error = amd756_smbus.algo->smbus_xfer(&amd756_smbus, 0x18, 0,
+					      I2C_SMBUS_WRITE, 0x03,
+					      I2C_SMBUS_BYTE_DATA, &ioconfig);
+	if (error) {
+		dev_dbg(&amd756_smbus.dev, "PCA9556 configuration failed\n");
+		error = -EIO;
+		goto ERROR3;
+	}
+
+	/* Register virtual adapters */
+	for (i = 0; i < 5; i++) {
+		error = i2c_add_adapter(s4882_adapter+i);
+		if (error) {
+			dev_err(&amd756_smbus.dev,
+			       "Virtual adapter %d registration "
+			       "failed, module not inserted\n", i);
+			for (i--; i >= 0; i--)
+				i2c_del_adapter(s4882_adapter+i);
+			goto ERROR3;
+		}
+	}
+
+	return 0;
+
+ERROR3:
+	kfree(s4882_algo);
+	s4882_algo = NULL;
+ERROR2:
+	kfree(s4882_adapter);
+	s4882_adapter = NULL;
+ERROR1:
+	i2c_del_adapter(&amd756_smbus);
+ERROR0:
+	return error;
+}
+
+static void __exit amd756_s4882_exit(void)
+{
+	if (s4882_adapter) {
+		int i;
+
+		for (i = 0; i < 5; i++)
+			i2c_del_adapter(s4882_adapter+i);
+		kfree(s4882_adapter);
+		s4882_adapter = NULL;
+	}
+	if (s4882_algo) {
+		kfree(s4882_algo);
+		s4882_algo = NULL;
+	}
+
+	/* Restore physical bus */
+	if (i2c_add_adapter(&amd756_smbus))
+		dev_err(&amd756_smbus.dev, "Physical bus restoration "
+			"failed\n");
+}
+
+MODULE_AUTHOR("Jean Delvare <khali@linux-fr.org>");
+MODULE_DESCRIPTION("S4882 SMBus multiplexing");
+MODULE_LICENSE("GPL");
+
+module_init(amd756_s4882_init);
+module_exit(amd756_s4882_exit);
diff -ruN drivers/i2c/busses.orig/i2c-amd756.c drivers/i2c/busses/i2c-amd756.c
--- drivers/i2c/busses.orig/i2c-amd756.c	2004-10-26 20:48:19.000000000 +0200
+++ drivers/i2c/busses/i2c-amd756.c	2004-11-04 19:58:25.000000000 +0100
@@ -302,7 +302,7 @@
 	.functionality	= amd756_func,
 };
 
-static struct i2c_adapter amd756_adapter = {
+struct i2c_adapter amd756_smbus = {
 	.owner		= THIS_MODULE,
 	.class          = I2C_CLASS_HWMON,
 	.algo		= &smbus_algorithm,
@@ -374,12 +374,12 @@
 	dev_dbg(&pdev->dev, "AMD756_smba = 0x%X\n", amd756_ioport);
 
 	/* set up the driverfs linkage to our parent device */
-	amd756_adapter.dev.parent = &pdev->dev;
+	amd756_smbus.dev.parent = &pdev->dev;
 
-	sprintf(amd756_adapter.name, "SMBus %s adapter at %04x",
+	sprintf(amd756_smbus.name, "SMBus %s adapter at %04x",
 		chipname[id->driver_data], amd756_ioport);
 
-	error = i2c_add_adapter(&amd756_adapter);
+	error = i2c_add_adapter(&amd756_smbus);
 	if (error) {
 		dev_err(&pdev->dev,
 			"Adapter registration failed, module not inserted\n");
@@ -395,7 +395,7 @@
 
 static void __devexit amd756_remove(struct pci_dev *dev)
 {
-	i2c_del_adapter(&amd756_adapter);
+	i2c_del_adapter(&amd756_smbus);
 	release_region(amd756_ioport, SMB_IOSIZE);
 }
 
@@ -420,5 +420,7 @@
 MODULE_DESCRIPTION("AMD756/766/768/8111 and nVidia nForce SMBus driver");
 MODULE_LICENSE("GPL");
 
+EXPORT_SYMBOL(amd756_smbus);
+
 module_init(amd756_init)
 module_exit(amd756_exit)


-- 
Jean Delvare
http://khali.linux-fr.org/

More on SMBus multiplexing

[Greg KH]

On Mon, Nov 08, 2004 at 10:50:05PM +0100, Jean Delvare wrote:
> Quoting myself:
> 
> > On second thought, don't. As I said on LKML this morning, my autoload
> > code doesn't work and we will have to stick to manual loading for now.
> > I'll provide a simplified patch that does this tomorrow.
> 
> We are not exactly tomorrow but here is the modified patch. Please note
> that this is only safe tp apply *after* my other patch that adds extra
> checks to i2c_del_adapter.
> 
> Please apply,

You sent this patch with the wrong -p level :(
I fixed it up...

Applied, thanks.

greg k-h

More on SMBus multiplexing

[Jean Delvare]

Hi Greg,

>You sent this patch with the wrong -p level :(
>I fixed it up...

Oops. I'm really sorry about this. I generated the patch as a test to
make sure it was correct first, and completely forgot that it wasn't
meant to be sent to you. Thanks for fixing my silly mistakes. I'll try
[1] to be more careful next times.

Thanks,
Jean

[1] There is no try. I *will* be more careful next times ;)