[PATCH 0/4] Support multiplexed main SMBus interface on SB800

[PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Christian Fetzer]

This is an attempt to upstream the patches created by Thomas Brandon and
Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
chipset. (https://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg06757.html)

I have mainly rebased the latest patch version and tested the driver on a
HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
from a w83795adg).

The patched driver is running stable on the machine, given that ic2_piix4 is
loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
sensors triggers some errors:
    ERROR: Can't get value of subfeature temp1_min_alarm: Can't read

While the kernel log shows:
    i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
    i2c i2c-1: Error: no response!
    i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
Unfortunately I don't know how to tackle this specific issue.

Please review and let me know required changes in order to get this upstream
finally.

Eddi, Thomas, it would be great if you could verify the changes on your
machines.

Regards,
Christian

Christian Fetzer (4):
  i2c-piix4: Optionally release smba in piix4_adap_remove
  i2c-piix4: Convert piix4_main_adapter to array
  i2c-piix4: Add support for multiplexed main adapter in SB800
  i2c-piix4: Add adapter port name support for SB800 chipset

 drivers/i2c/busses/i2c-piix4.c | 151 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 134 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH 1/4] i2c-piix4: Optionally release smba in piix4_adap_remove

[Christian Fetzer]

This is in preparation to support the multiplexed SMBus main controller
in the SB800 chipset where the controller address is shared among
the four multiplexed ports. As such the address region should be
only freed for the first multiplexed adapter to avoid double free
warnings.

Signed-off-by: Christian Fetzer <fetzer.ch-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 630bce6..97d2165 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -660,13 +660,14 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	return 0;
 }
 
-static void piix4_adap_remove(struct i2c_adapter *adap)
+static void piix4_adap_remove(struct i2c_adapter *adap, int free_smba)
 {
 	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		release_region(adapdata->smba, SMBIOSIZE);
+		if (free_smba)
+			release_region(adapdata->smba, SMBIOSIZE);
 		kfree(adapdata);
 		kfree(adap);
 	}
@@ -675,12 +676,12 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 static void piix4_remove(struct pci_dev *dev)
 {
 	if (piix4_main_adapter) {
-		piix4_adap_remove(piix4_main_adapter);
+		piix4_adap_remove(piix4_main_adapter, 1);
 		piix4_main_adapter = NULL;
 	}
 
 	if (piix4_aux_adapter) {
-		piix4_adap_remove(piix4_aux_adapter);
+		piix4_adap_remove(piix4_aux_adapter, 1);
 		piix4_aux_adapter = NULL;
 	}
 }
-- 
1.9.1

[PATCH 2/4] i2c-piix4: Convert piix4_main_adapter to array

[Christian Fetzer]

The SB800 chipset supports a multiplexed main SMBus controller with
four ports. Therefore the static variable piix4_main_adapter is
converted into a piix4_main_adapters array that can hold one
i2c_adapter for each multiplexed port.

The auxiliary adapter remains unchanged since it represents the second
(not multiplexed) SMBus controller on the SB800 chipset.

Signed-off-by: Christian Fetzer <fetzer.ch-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 97d2165..8934a32 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -75,6 +75,9 @@
 #define PIIX4_WORD_DATA		0x0C
 #define PIIX4_BLOCK_DATA	0x14
 
+/* Multi-port constants */
+#define PIIX4_MAX_ADAPTERS 4
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the
@@ -561,7 +564,7 @@ static const struct pci_device_id piix4_ids[] = {
 
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
-static struct i2c_adapter *piix4_main_adapter;
+static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
@@ -629,7 +632,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return retval;
 
 	/* Try to register main SMBus adapter, give up if we can't */
-	retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
+	retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
 	if (retval < 0)
 		return retval;
 
@@ -675,9 +678,14 @@ static void piix4_adap_remove(struct i2c_adapter *adap, int free_smba)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	if (piix4_main_adapter) {
-		piix4_adap_remove(piix4_main_adapter, 1);
-		piix4_main_adapter = NULL;
+	int port;
+
+	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+		if (piix4_main_adapters[port]) {
+			piix4_adap_remove(piix4_main_adapters[port],
+					  port == 0);
+			piix4_main_adapters[port] = NULL;
+		}
 	}
 
 	if (piix4_aux_adapter) {
-- 
1.9.1

[PATCH 3/4] i2c-piix4: Add support for multiplexed main adapter in SB800

[Christian Fetzer]

The SB800 chipset supports a multiplexed main SMBus controller with
four ports. The multiplexed ports share the same SMBus address and
register set. The port is selected by bits 2:1 of the smb_en register
(0x2C).

Only one port can be active at any point in time therefore a mutex is
needed in order to synchronize access.

Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
support to access sensor data from the w83795adg).

Cc: Thomas Brandon <tbrandonau-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Eddi De Pieri <eddi-soWH+0lSOSbR7s880joybQ@public.gmane.org>
Signed-off-by: Christian Fetzer <fetzer.ch-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c | 114 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 107 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 8934a32..e344abd 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -23,6 +23,9 @@
 
    Note: we assume there can only be one device, with one or more
    SMBus interfaces.
+   The device can register multiple i2c_adapters (up to PIIX4_MAX_ADAPTERS).
+   For devices supporting multiple ports the i2c_adapter should provide
+   an i2c_algorithm to access them.
 */
 
 #include <linux/module.h>
@@ -37,6 +40,7 @@
 #include <linux/dmi.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 
 
 /* PIIX4 SMBus address offsets */
@@ -125,8 +129,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 	{ },
 };
 
+/* SB800 globals */
+DEFINE_MUTEX(piix4_mutex_sb800);
+
 struct i2c_piix4_adapdata {
 	unsigned short smba;
+	unsigned short port;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -313,6 +321,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus\n");
 
+	mutex_init(&piix4_mutex_sb800);
+
 	dev_info(&PIIX4_dev->dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
@@ -527,6 +537,49 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 	return 0;
 }
 
+/* Handles access to multiple SMBus ports on the SB800.
+ * The port is selected by bits 2:1 of the smb_en register (0x2C).
+ * NOTE: The selected port must be returned to the initial selection to
+ * avoid problems on certain systems.
+ * Return negative errno on error.
+ */
+static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
+		 unsigned short flags, char read_write,
+		 u8 command, int size, union i2c_smbus_data *data)
+{
+	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+	unsigned short smba_idx = 0xcd6;
+	u8 smba_en_lo, smb_en = 0x2c;
+	u8 port;
+	int retval;
+
+	mutex_lock(&piix4_mutex_sb800);
+
+	if (!request_region(smba_idx, 2, "smba_idx")) {
+		dev_err(&adap->dev, "SMBus base address index region "
+				"0x%x already in use!\n", smba_idx);
+		retval = -EBUSY;
+		goto ERROR;
+	}
+	outb_p(smb_en, smba_idx);
+	smba_en_lo = inb_p(smba_idx + 1);
+
+	port = adapdata->port;
+	if ((smba_en_lo & 6) != (port << 1))
+		outb_p((smba_en_lo & ~6) | (port << 1), smba_idx + 1);
+
+	retval = piix4_access(adap, addr, flags, read_write,
+			      command, size, data);
+
+	outb_p(smba_en_lo, smba_idx + 1);
+	release_region(smba_idx, 2);
+
+ERROR:
+	mutex_unlock(&piix4_mutex_sb800);
+
+	return retval;
+}
+
 static u32 piix4_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
@@ -539,6 +592,11 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= piix4_func,
 };
 
+static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = {
+	.smbus_xfer	= piix4_access_sb800,
+	.functionality	= piix4_func,
+};
+
 static const struct pci_device_id piix4_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -614,6 +672,41 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	return 0;
 }
 
+static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
+{
+	unsigned short port;
+	int retval;
+	struct i2c_piix4_adapdata *adapdata;
+
+	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+		retval = piix4_add_adapter(dev, smba, &piix4_main_adapters[port]);
+		if (retval < 0)
+			goto ERROR;
+
+		piix4_main_adapters[port]->algo = &piix4_smbus_algorithm_sb800;
+
+		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
+		adapdata->port = port;
+	}
+
+	return retval;
+
+ERROR:
+	dev_err(&dev->dev, "Error setting up SB800 adapters. "
+		"Unregistering all adapters!\n");
+	for (port--; port >= 0; port--) {
+		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
+		if (adapdata->smba) {
+			i2c_del_adapter(piix4_main_adapters[port]);
+			kfree(adapdata);
+			kfree(piix4_main_adapters[port]);
+			piix4_main_adapters[port] = NULL;
+		}
+	}
+
+	return retval;
+}
+
 static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
@@ -621,18 +714,25 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
 	     dev->revision >= 0x40) ||
-	    dev->vendor == PCI_VENDOR_ID_AMD)
+	    dev->vendor == PCI_VENDOR_ID_AMD) {
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-	else
+		if (retval < 0)
+			return retval;
+
+		/* Try to register multiplexed main SMBus adapter,
+		 * give up if we can't */
+		retval = piix4_add_adapters_sb800(dev, retval);
+	} else {
 		retval = piix4_setup(dev, id);
+		if (retval < 0)
+			return retval;
 
-	/* If no main SMBus found, give up */
-	if (retval < 0)
-		return retval;
+		/* Try to register main SMBus adapter, give up if we can't */
+		retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
+	}
 
-	/* Try to register main SMBus adapter, give up if we can't */
-	retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
+	/* If no main SMBus found, give up */
 	if (retval < 0)
 		return retval;
 
-- 
1.9.1

[PATCH 4/4] i2c-piix4: Add adapter port name support for SB800 chipset

[Christian Fetzer]

This patch adds support for port names for the SB800 chipset.
Since the chipset supports a multiplexed main SMBus controller, adding
the channel name to the adapter name is necessary to differentiate the
ports better (for example in sensors output).

Signed-off-by: Christian Fetzer <fetzer.ch-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index e344abd..366878e 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -131,6 +131,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /* SB800 globals */
 DEFINE_MUTEX(piix4_mutex_sb800);
+static const char *piix4_main_port_names_sb800[4] = {
+	"SDA0", "SDA2", "SDA3", "SDA4"
+};
+static const char *piix4_aux_port_name_sb800 = "SDA1";
 
 struct i2c_piix4_adapdata {
 	unsigned short smba;
@@ -626,7 +630,7 @@ static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-			     struct i2c_adapter **padap)
+			     const char *name, struct i2c_adapter **padap)
 {
 	struct i2c_adapter *adap;
 	struct i2c_piix4_adapdata *adapdata;
@@ -655,7 +659,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	adap->dev.parent = &dev->dev;
 
 	snprintf(adap->name, sizeof(adap->name),
-		"SMBus PIIX4 adapter at %04x", smba);
+		"SMBus PIIX4 adapter %s at %04x", name, smba);
 
 	i2c_set_adapdata(adap, adapdata);
 
@@ -679,7 +683,9 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
 	struct i2c_piix4_adapdata *adapdata;
 
 	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
-		retval = piix4_add_adapter(dev, smba, &piix4_main_adapters[port]);
+		retval = piix4_add_adapter(dev, smba,
+					   piix4_main_port_names_sb800[port],
+					   &piix4_main_adapters[port]);
 		if (retval < 0)
 			goto ERROR;
 
@@ -729,7 +735,8 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			return retval;
 
 		/* Try to register main SMBus adapter, give up if we can't */
-		retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
+		retval = piix4_add_adapter(dev, retval, "",
+					   &piix4_main_adapters[0]);
 	}
 
 	/* If no main SMBus found, give up */
@@ -757,7 +764,8 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (retval > 0) {
 		/* Try to add the aux adapter if it exists,
 		 * piix4_add_adapter will clean up if this fails */
-		piix4_add_adapter(dev, retval, &piix4_aux_adapter);
+		piix4_add_adapter(dev, retval, piix4_aux_port_name_sb800,
+				  &piix4_aux_adapter);
 	}
 
 	return 0;
-- 
1.9.1

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On Tue, Aug 25, 2015 at 01:05:01PM +0200, Christian Fetzer wrote:
> This is an attempt to upstream the patches created by Thomas Brandon and
> Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
> chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)
> 
> I have mainly rebased the latest patch version and tested the driver on a
> HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
> from a w83795adg).
> 
> The patched driver is running stable on the machine, given that ic2_piix4 is
> loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
> sensors triggers some errors:
>     ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
> 
> While the kernel log shows:
>     i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
>     i2c i2c-1: Error: no response!
>     i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
> Unfortunately I don't know how to tackle this specific issue.
> 
> Please review and let me know required changes in order to get this upstream
> finally.
> 
> Eddi, Thomas, it would be great if you could verify the changes on your
> machines.
> 
> Regards,
> Christian
> 
> Christian Fetzer (4):
>   i2c-piix4: Optionally release smba in piix4_adap_remove
>   i2c-piix4: Convert piix4_main_adapter to array
>   i2c-piix4: Add support for multiplexed main adapter in SB800
>   i2c-piix4: Add adapter port name support for SB800 chipset
> 
>  drivers/i2c/busses/i2c-piix4.c | 151 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 134 insertions(+), 17 deletions(-)

Jean, is that on your list?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

On Tue, Aug 25, 2015 at 01:05:01PM +0200, Christian Fetzer wrote:
> This is an attempt to upstream the patches created by Thomas Brandon and
> Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
> chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)
> 
> I have mainly rebased the latest patch version and tested the driver on a
> HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
> from a w83795adg).
> 
> The patched driver is running stable on the machine, given that ic2_piix4 is
> loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
> sensors triggers some errors:
>     ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
> 
> While the kernel log shows:
>     i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
>     i2c i2c-1: Error: no response!
>     i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
> Unfortunately I don't know how to tackle this specific issue.
> 
> Please review and let me know required changes in order to get this upstream
> finally.
> 
> Eddi, Thomas, it would be great if you could verify the changes on your
> machines.

Yes, additional tests are always good for a patch series

Asking the Intel guys for help, I have not much expierence with x86
platforms... Mika, Jarkko, Andy any chance to help?

> 
> Regards,
> Christian
> 
> Christian Fetzer (4):
>   i2c-piix4: Optionally release smba in piix4_adap_remove
>   i2c-piix4: Convert piix4_main_adapter to array
>   i2c-piix4: Add support for multiplexed main adapter in SB800
>   i2c-piix4: Add adapter port name support for SB800 chipset
> 
>  drivers/i2c/busses/i2c-piix4.c | 151 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 134 insertions(+), 17 deletions(-)
> 
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Mika Westerberg]

On Tue, Oct 20, 2015 at 05:19:35PM +0200, Wolfram Sang wrote:
> On Tue, Aug 25, 2015 at 01:05:01PM +0200, Christian Fetzer wrote:
> > This is an attempt to upstream the patches created by Thomas Brandon and
> > Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
> > chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)
> > 
> > I have mainly rebased the latest patch version and tested the driver on a
> > HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
> > from a w83795adg).
> > 
> > The patched driver is running stable on the machine, given that ic2_piix4 is
> > loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
> > sensors triggers some errors:
> >     ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
> > 
> > While the kernel log shows:
> >     i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
> >     i2c i2c-1: Error: no response!
> >     i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
> > Unfortunately I don't know how to tackle this specific issue.
> > 
> > Please review and let me know required changes in order to get this upstream
> > finally.
> > 
> > Eddi, Thomas, it would be great if you could verify the changes on your
> > machines.
> 
> Yes, additional tests are always good for a patch series
> 
> Asking the Intel guys for help, I have not much expierence with x86
> platforms... Mika, Jarkko, Andy any chance to help?

Unfortunately I don't have hardware this old to test on :-/

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]


> > > Please review and let me know required changes in order to get this upstream
> > > finally.
> > > 
> > > Eddi, Thomas, it would be great if you could verify the changes on your
> > > machines.
> > 
> > Yes, additional tests are always good for a patch series
> > 
> > Asking the Intel guys for help, I have not much expierence with x86
> > platforms... Mika, Jarkko, Andy any chance to help?
> 
> Unfortunately I don't have hardware this old to test on :-/

And visual review? (That's what I need to do mostly, too)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Mika Westerberg]

On Thu, Oct 22, 2015 at 01:03:25PM +0200, Wolfram Sang wrote:
> 
> > > > Please review and let me know required changes in order to get this upstream
> > > > finally.
> > > > 
> > > > Eddi, Thomas, it would be great if you could verify the changes on your
> > > > machines.
> > > 
> > > Yes, additional tests are always good for a patch series
> > > 
> > > Asking the Intel guys for help, I have not much expierence with x86
> > > platforms... Mika, Jarkko, Andy any chance to help?
> > 
> > Unfortunately I don't have hardware this old to test on :-/
> 
> And visual review? (That's what I need to do mostly, too)

Sure.

I don't have a copy of these patches but I went ahead and looked them up
from archives. Christian can you Cc me on next iteration?

Mostly they look good to me. Few comments though.

Patch 2/4: should we remove adapter in reverse order?

Patch 3/4: some stylistic issues, like:
	- ERROR label should not be in capital letters actually it is
	  not needed at all if you do unlock and return -EBUSY if
	  request_region() fails.
	- Block comment style

In addition I'm not sure if requesting io region for each transfer is
good idea. Can't we just request it once for this driver and handle the
necessary serialization using the mutex?

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[fetzerch]

Hi Mika,

On 22.10.2015 13:43, Mika Westerberg wrote:
> On Thu, Oct 22, 2015 at 01:03:25PM +0200, Wolfram Sang wrote:
>>>>> Please review and let me know required changes in order to get this upstream
>>>>> finally.
>>>>>
>>>>> Eddi, Thomas, it would be great if you could verify the changes on your
>>>>> machines.
>>>> Yes, additional tests are always good for a patch series
>>>>
>>>> Asking the Intel guys for help, I have not much expierence with x86
>>>> platforms... Mika, Jarkko, Andy any chance to help?
>>> Unfortunately I don't have hardware this old to test on :-/
>> And visual review? (That's what I need to do mostly, too)
> Sure.
>
> I don't have a copy of these patches but I went ahead and looked them up
> from archives. Christian can you Cc me on next iteration?
>
> Mostly they look good to me. Few comments though.
>
> Patch 2/4: should we remove adapter in reverse order?
>
> Patch 3/4: some stylistic issues, like:
> 	- ERROR label should not be in capital letters actually it is
> 	  not needed at all if you do unlock and return -EBUSY if
> 	  request_region() fails.
> 	- Block comment style
>
> In addition I'm not sure if requesting io region for each transfer is
> good idea. Can't we just request it once for this driver and handle the
> necessary serialization using the mutex?
Thanks for the review. I've just sent patchset v2 where I tried to 
incorporate the requested changes.
I'm not sure though what you mean with 'block comment style'.
I've tried to apply the same style that is used throughout the file.

Thanks,
Christian

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Jean Delvare]

Hi Wolfram, Christian,

On Sat, 10 Oct 2015 08:45:49 +0100, Wolfram Sang wrote:
> On Tue, Aug 25, 2015 at 01:05:01PM +0200, Christian Fetzer wrote:
> > This is an attempt to upstream the patches created by Thomas Brandon and
> > Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
> > chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)
> > 
> > I have mainly rebased the latest patch version and tested the driver on a
> > HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
> > from a w83795adg).
> > 
> > The patched driver is running stable on the machine, given that ic2_piix4 is
> > loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
> > sensors triggers some errors:
> >     ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
> > 
> > While the kernel log shows:
> >     i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
> >     i2c i2c-1: Error: no response!
> >     i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
> > Unfortunately I don't know how to tackle this specific issue.

Christian, is the problem still present with the latest version of the patches?

> > Please review and let me know required changes in order to get this upstream
> > finally.
> > 
> > Eddi, Thomas, it would be great if you could verify the changes on your
> > machines.
> > 
> > Regards,
> > Christian
> > 
> > Christian Fetzer (4):
> >   i2c-piix4: Optionally release smba in piix4_adap_remove
> >   i2c-piix4: Convert piix4_main_adapter to array
> >   i2c-piix4: Add support for multiplexed main adapter in SB800
> >   i2c-piix4: Add adapter port name support for SB800 chipset
> > 
> >  drivers/i2c/busses/i2c-piix4.c | 151 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 134 insertions(+), 17 deletions(-)
> 
> Jean, is that on your list?

Very sorry, I intended to reply to thins but let it slip through :(

Now I see the patches have been reviewed multiple times and merged for
kernel v4.5. I looked at the code and suspect there are issues which
need to be fixed. But I would like to read the previous reviews to make
sure I'm not missing something...

Wolfram, would you be kind enough to put all the previous versions of
the patch set and their reviews into a single mbox file and send that
to me? So I can understand the reasons behind the current code.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Jean Delvare]

Hi Wolfram,

On Tue, 20 Oct 2015 17:19:35 +0200, Wolfram Sang wrote:
> On Tue, Aug 25, 2015 at 01:05:01PM +0200, Christian Fetzer wrote:
> > This is an attempt to upstream the patches created by Thomas Brandon and
> > Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
> > chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)
> > 
> > I have mainly rebased the latest patch version and tested the driver on a
> > HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
> > from a w83795adg).
> > 
> > The patched driver is running stable on the machine, given that ic2_piix4 is
> > loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
> > sensors triggers some errors:
> >     ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
> > 
> > While the kernel log shows:
> >     i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
> >     i2c i2c-1: Error: no response!
> >     i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
> > Unfortunately I don't know how to tackle this specific issue.
> > 
> > Please review and let me know required changes in order to get this upstream
> > finally.
> > 
> > Eddi, Thomas, it would be great if you could verify the changes on your
> > machines.
> 
> Yes, additional tests are always good for a patch series
> 
> Asking the Intel guys for help, I have not much expierence with x86
> platforms... Mika, Jarkko, Andy any chance to help?

JFYI... The i2c-piix4 driver was originally for Intel hardware, however
for many many years now Intel has moved to i2c-i801 and the only recent
hardware supported by the i2c-piix4 driver is from AMD. So if you want
to bother a hardware vendor with this driver, that should be AMD, not
Intel.

-- 
Jean Delvare
SUSE L3 Support