Skip to contentSkip to conversation

Improvements to the i2c-piix4 driver

PATCH Applied Jean Delvare 4 participants 18 messages Jan 29, 2016 – Feb 24, 2016

Subsystems (2)
I2C subsystem host drivers I2C/smbus controller drivers for pc

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

cover Improvements to the i2c-piix4 driver

Hi all,

Here are 3 proposed improvements to the i2c-piix4 driver, on top of the 3 patches I sent recently. Christian, I would appreciate if you can test them and confirm that they do not cause any regression for you.

[PATCH 1/3] i2c-piix4: Support alternative port selection register [PATCH 2/3] i2c-piix4: Always use the same type for port [PATCH 3/3] i2c-piix4: Pre-shift the port number

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#1 lore raw

PATCH 1/3 i2c-piix4: Support alternative port selection register

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +26 -4
@@ -85,8 +85,13 @@
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
 
-/* SB800 port is selected by bits 2:1 of the smb_en register (0x2c) */
+/*
+ * SB800 port is selected by bits 2:1 of the smb_en register (0x2c)
+ * or the smb_sel register (0x2e), depending on bit 0 of register 0x2f.
+ */
 #define SB800_PIIX4_PORT_IDX		0x2c
+#define SB800_PIIX4_PORT_IDX_ALT	0x2e
+#define SB800_PIIX4_PORT_IDX_SEL	0x2f
 #define SB800_PIIX4_PORT_IDX_MASK	0x06
 
 /* insmod parameters */

@@ -136,8 +141,13 @@ static const struct dmi_system_id piix4_
 	{ },
 };
 
-/* SB800 globals */
+/*
+ * SB800 globals
+ * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
+ * of I/O ports at SB800_PIIX4_SMB_IDX.
+ */
 static DEFINE_MUTEX(piix4_mutex_sb800);
+static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
 };

@@ -254,7 +264,7 @@ static int piix4_setup_sb800(struct pci_
 			     const struct pci_device_id *id, u8 aux)
 {
 	unsigned short piix4_smba;
-	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
+	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
 
 	/* SB800 and later SMBus does not support forcing address */

@@ -334,6 +344,18 @@ static int piix4_setup_sb800(struct pci_
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
 
+	/* Find which register is used for port selection */
+	mutex_lock(&piix4_mutex_sb800);
+	outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
+	port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	piix4_port_sel_sb800 = (port_sel & 0x01) ? SB800_PIIX4_PORT_IDX_ALT
+						 : SB800_PIIX4_PORT_IDX;
+	mutex_unlock(&piix4_mutex_sb800);
+
+	dev_info(&PIIX4_dev->dev,
+		 "Using register 0x%02x for SMBus port selection\n",
+		 (unsigned int)piix4_port_sel_sb800);
+
 	return piix4_smba;
 }
 

@@ -563,7 +585,7 @@ static s32 piix4_access_sb800(struct i2c
 
 	mutex_lock(&piix4_mutex_sb800);
 
-	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
+	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;
Signature · 2 lines
Jean Delvare
SUSE L3 Support
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Quoted 8 lines · Jean Delvare

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>

One small comment, see below.

Reviewed-by Mika Westerberg <[email protected]>
Quoted 52 lines
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +14 -3
@@ -85,8 +85,13 @@
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
 
-/* SB800 port is selected by bits 2:1 of the smb_en register (0x2c) */
+/*
+ * SB800 port is selected by bits 2:1 of the smb_en register (0x2c)
+ * or the smb_sel register (0x2e), depending on bit 0 of register 0x2f.
+ */
 #define SB800_PIIX4_PORT_IDX		0x2c
+#define SB800_PIIX4_PORT_IDX_ALT	0x2e
+#define SB800_PIIX4_PORT_IDX_SEL	0x2f
 #define SB800_PIIX4_PORT_IDX_MASK	0x06
 
 /* insmod parameters */

@@ -136,8 +141,13 @@ static const struct dmi_system_id piix4_
 	{ },
 };
 
-/* SB800 globals */
+/*
+ * SB800 globals
+ * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
+ * of I/O ports at SB800_PIIX4_SMB_IDX.
+ */
 static DEFINE_MUTEX(piix4_mutex_sb800);
+static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
 };

@@ -254,7 +264,7 @@ static int piix4_setup_sb800(struct pci_
 			     const struct pci_device_id *id, u8 aux)
 {
 	unsigned short piix4_smba;
-	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
+	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
 
 	/* SB800 and later SMBus does not support forcing address */

@@ -334,6 +344,18 @@ static int piix4_setup_sb800(struct pci_
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
 
+	/* Find which register is used for port selection */

drivers/i2c/busses/i2c-piix4.c static int piix4_setup_sb800(struct pci_ · line 357

Quoted 10 lines
drivers/i2c/busses/i2c-piix4.c +10 -0
+	mutex_lock(&piix4_mutex_sb800);
+	outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
+	port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	piix4_port_sel_sb800 = (port_sel & 0x01) ? SB800_PIIX4_PORT_IDX_ALT
+						 : SB800_PIIX4_PORT_IDX;
+	mutex_unlock(&piix4_mutex_sb800);
+
+	dev_info(&PIIX4_dev->dev,
+		 "Using register 0x%02x for SMBus port selection\n",
+		 (unsigned int)piix4_port_sel_sb800);

Would dev_dbg() be better here? Not sure how useful this information is for normal users.

Quoted 17 lines
drivers/i2c/busses/i2c-piix4.c +2 -1
+
 	return piix4_smba;
 }
 

@@ -563,7 +585,7 @@ static s32 piix4_access_sb800(struct i2c
 
 	mutex_lock(&piix4_mutex_sb800);
 
-	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
+	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;
Signature · 3 lines
-- 
Jean Delvare
SUSE L3 Support
author
#3 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Quoted 4 lines · Mika Westerberg
Quoted 4 lines
drivers/i2c/busses/i2c-piix4.c +0 -0
@@ -334,6 +344,18 @@ static int piix4_setup_sb800(struct pci_
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
Quoted 14 lines

drivers/i2c/busses/i2c-piix4.c static int piix4_setup_sb800(struct pci_ · line 357

Quoted 11 lines
drivers/i2c/busses/i2c-piix4.c +11 -0
+	/* Find which register is used for port selection */
+	mutex_lock(&piix4_mutex_sb800);
+	outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
+	port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	piix4_port_sel_sb800 = (port_sel & 0x01) ? SB800_PIIX4_PORT_IDX_ALT
+						 : SB800_PIIX4_PORT_IDX;
+	mutex_unlock(&piix4_mutex_sb800);
+
+	dev_info(&PIIX4_dev->dev,
+		 "Using register 0x%02x for SMBus port selection\n",
+		 (unsigned int)piix4_port_sel_sb800);

Would dev_dbg() be better here? Not sure how useful this information is for normal users.

Not sure. I agree that the register address isn't that helpful, but the fact that port selection is needed is good to know. Sure I could use dev_info() to mention that multiplexing is taking place, and then dev_dbg() to give the port address, but that seems needlessly complex. And it's not like the kernel log isn't full of I/O addresses already...

So I'd leave it as is, unless Wolfram objects.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#4 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

In reply to Jean Delvare
Quoted 5 lines · Jean Delvare

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Were you able to test both cases?

author
#5 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Hi Wolfram,

Quoted 8 lines · Wolfram Sang
Quoted 5 lines · Jean Delvare

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Were you able to test both cases?

I don't have the hardware myself so I was not able to test any case. I was hoping Christian would test. This is the reason why I am logging which register is used, so that we know if the alternative setting is ever used. I found the potential problem by looking at the datasheet, it's not something that has been reported (yet.)

Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH on AMD's web site, but Hudson2 in our driver) which suggest that the "alternative" setting is the only possible one on this chipset. The register used to figure out the setting is marked as reserved. If the register exists still and the relevant bit is set, then my patch should work. If not then a better patch will be needed.

I'll try to gain access to a system with a Bolton FCH and experiment with it.

Then there's the most recent device, codenamed "CZ", for which I have no information at all.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#6 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Hi Jean,

Quoted 25 lines · Jean Delvare

Hi Wolfram,

Quoted 8 lines · Wolfram Sang
Quoted 5 lines · Jean Delvare

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Were you able to test both cases?

I don't have the hardware myself so I was not able to test any case. I was hoping Christian would test. This is the reason why I am logging which register is used, so that we know if the alternative setting is ever used. I found the potential problem by looking at the datasheet, it's not something that has been reported (yet.)

Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH on AMD's web site, but Hudson2 in our driver) which suggest that the "alternative" setting is the only possible one on this chipset. The register used to figure out the setting is marked as reserved. If the register exists still and the relevant bit is set, then my patch should work. If not then a better patch will be needed.

Quoted 6 lines

I'll try to gain access to a system with a Bolton FCH and experiment with it.

Then there's the most recent device, codenamed "CZ", for which I have no information at all.

sorry for the late response. This time it slipped through on my side.

The sensors command still works fine without any noticable change. Here is the output:

[1894808.057493] piix4_smbus 0000:00:14.0: SMBus Host Controller at
0xb00, revision 0
[1894808.057504] piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus
port selection
[1894808.394960] i2c i2c-1: Found w83795adg rev. B at 0x2f
[1894808.666968] piix4_smbus 0000:00:14.0: Auxiliary SMBus Host

Controller at 0xb20

Thanks,
Christian

author
#7 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Quoted 40 lines · fetzerch

Hi Jean,

Quoted 25 lines · Jean Delvare

Hi Wolfram,

Quoted 8 lines · Wolfram Sang
Quoted 5 lines · Jean Delvare

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Were you able to test both cases?

I don't have the hardware myself so I was not able to test any case. I was hoping Christian would test. This is the reason why I am logging which register is used, so that we know if the alternative setting is ever used. I found the potential problem by looking at the datasheet, it's not something that has been reported (yet.)

Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH on AMD's web site, but Hudson2 in our driver) which suggest that the "alternative" setting is the only possible one on this chipset. The register used to figure out the setting is marked as reserved. If the register exists still and the relevant bit is set, then my patch should work. If not then a better patch will be needed.

Quoted 6 lines

I'll try to gain access to a system with a Bolton FCH and experiment with it.

Then there's the most recent device, codenamed "CZ", for which I have no information at all.

sorry for the late response. This time it slipped through on my side.

The sensors command still works fine without any noticable change. Here is the output:

Quoted 7 lines
[1894808.057493] piix4_smbus 0000:00:14.0: SMBus Host Controller at
0xb00, revision 0
[1894808.057504] piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus
port selection
[1894808.394960] i2c i2c-1: Found w83795adg rev. B at 0x2f
[1894808.666968] piix4_smbus 0000:00:14.0: Auxiliary SMBus Host

Controller at 0xb20

Thanks for testing, Christian, this is very helpful.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#8 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

In reply to Jean Delvare
Quoted 25 lines · Jean Delvare

Hi Wolfram,

Quoted 8 lines · Wolfram Sang
Quoted 5 lines · Jean Delvare

The SB800 register reference guide says that the SMBus port selection bits may not always be in register Smbus0En (0x2c) but could alternatively be found in register Smbus0Sel (0x2e) depending on the settings in register Smbus0SelEn (0x2f.) Add support for this configuration.

Were you able to test both cases?

I don't have the hardware myself so I was not able to test any case. I was hoping Christian would test. This is the reason why I am logging which register is used, so that we know if the alternative setting is ever used. I found the potential problem by looking at the datasheet, it's not something that has been reported (yet.)

Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH on AMD's web site, but Hudson2 in our driver) which suggest that the "alternative" setting is the only possible one on this chipset. The register used to figure out the setting is marked as reserved. If the register exists still and the relevant bit is set, then my patch should work. If not then a better patch will be needed.

Quoted 2 lines

I'll try to gain access to a system with a Bolton FCH and experiment with it.

I have access to a Bolton FCH system for one week. I tested the port selection and as the datasheet suggested, the "alternative" register is always used on this chipset. Register Smbus0SelEn (0x2f) doesn't exist so the code I submitted doesn't work for this chipset. After unconditionally using 0x2e for port selection, the driver seems to work fine, except that sensors-detect just hung the machine when probing SMBus port 2 address 0x2f (and it's 1200 km away from me and I can't reboot it, yay.) That could be a driver issue or (more likely) just another case of I2C/SMBus device that hates being probed and hangs the system for reprisal.

Quoted 2 lines

Then there's the most recent device, codenamed "CZ", for which I have no information at all.

Still looking into that as well, trying to get my hands on both the documentation and the hardware. I would bet that it works the same as the Hudson2/Bolton, but if I can verify that it would be safer.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#9 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Quoted 2 lines

so the code I submitted doesn't work for this chipset. After

So I wait for V2 in any case?

author
#10 lore raw

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

Quoted 4 lines · Wolfram Sang
Quoted 2 lines

so the code I submitted doesn't work for this chipset. After

So I wait for V2 in any case?

Yes please.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#11 lore raw

PATCH 2/3 i2c-piix4: Always use the same type for port

In reply to Jean Delvare

Sometimes u8 is used to store the port number, sometimes unsigned short is used. Consistently stick to a single type, for consistency and to avoid implicit casts.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-piix4.c +2 -2
@@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
 
 	/* SB800 */
 	bool sb800_main;
-	unsigned short port;
+	u8 port;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,

@@ -649,7 +649,7 @@ static struct i2c_adapter *piix4_main_ad
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-			     bool sb800_main, unsigned short port,
+			     bool sb800_main, u8 port,
 			     const char *name, struct i2c_adapter **padap)
 {
 	struct i2c_adapter *adap;
Signature · 2 lines
Jean Delvare
SUSE L3 Support
Applied
#13 lore raw

Re: [PATCH 2/3] i2c-piix4: Always use the same type for port

In reply to Jean Delvare
Quoted 8 lines · Jean Delvare

Sometimes u8 is used to store the port number, sometimes unsigned short is used. Consistently stick to a single type, for consistency and to avoid implicit casts.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>

Applied to for-next, thanks!

author
#14 lore raw

PATCH 3/3 i2c-piix4: Pre-shift the port number

In reply to Jean Delvare

Shift the port number at initialization time, so that it is ready to use at run time. That way we don't have to do it again for every SMBus transaction.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-piix4.c +5 -5
@@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
 
 	/* SB800 */
 	bool sb800_main;
-	u8 port;
+	u8 port;		/* Port number, shifted */
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,

@@ -589,8 +589,8 @@ static s32 piix4_access_sb800(struct i2c
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;
-	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != (port << 1))
-		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | (port << 1),
+	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
+		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
 		       SB800_PIIX4_SMB_IDX + 1);
 
 	retval = piix4_access(adap, addr, flags, read_write,

@@ -676,7 +676,7 @@ static int piix4_add_adapter(struct pci_
 
 	adapdata->smba = smba;
 	adapdata->sb800_main = sb800_main;
-	adapdata->port = port;
+	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;

@@ -812,7 +812,7 @@ static void piix4_adap_remove(struct i2c
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == 0) {
+		if (adapdata->port == (0 << 1)) {
 			release_region(adapdata->smba, SMBIOSIZE);
 			if (adapdata->sb800_main)
 				release_region(SB800_PIIX4_SMB_IDX, 2);
Signature · 2 lines
Jean Delvare
SUSE L3 Support
Reviewed-by on v1 3/3
#15 lore raw

Re: [PATCH 3/3] i2c-piix4: Pre-shift the port number

Quoted 44 lines · Jean Delvare

Shift the port number at initialization time, so that it is ready to use at run time. That way we don't have to do it again for every SMBus transaction.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-piix4.c +4 -4
@@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
 
 	/* SB800 */
 	bool sb800_main;
-	u8 port;
+	u8 port;		/* Port number, shifted */
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,

@@ -589,8 +589,8 @@ static s32 piix4_access_sb800(struct i2c
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;
-	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != (port << 1))
-		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | (port << 1),
+	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
+		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
 		       SB800_PIIX4_SMB_IDX + 1);
 
 	retval = piix4_access(adap, addr, flags, read_write,

@@ -676,7 +676,7 @@ static int piix4_add_adapter(struct pci_
 
 	adapdata->smba = smba;
 	adapdata->sb800_main = sb800_main;
-	adapdata->port = port;
+	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;

drivers/i2c/busses/i2c-piix4.c static void piix4_adap_remove(struct i2c · line 815

Quoted 5 lines
drivers/i2c/busses/i2c-piix4.c +1 -1
@@ -812,7 +812,7 @@ static void piix4_adap_remove(struct i2c
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == 0) {
+		if (adapdata->port == (0 << 1)) {

I suppose this is only for documentation purposes, right?

Reviewed-by Mika Westerberg <[email protected]>
Quoted 7 lines
drivers/i2c/busses/i2c-piix4.c +0 -0
 			release_region(adapdata->smba, SMBIOSIZE);
 			if (adapdata->sb800_main)
 				release_region(SB800_PIIX4_SMB_IDX, 2);
Signature · 3 lines
-- 
Jean Delvare
SUSE L3 Support
author
#16 lore raw

Re: [PATCH 3/3] i2c-piix4: Pre-shift the port number

Quoted 46 lines · Mika Westerberg
Quoted 45 lines · Jean Delvare

Shift the port number at initialization time, so that it is ready to use at run time. That way we don't have to do it again for every SMBus transaction.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-piix4.c +4 -4
@@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
 
 	/* SB800 */
 	bool sb800_main;
-	u8 port;
+	u8 port;		/* Port number, shifted */
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,

@@ -589,8 +589,8 @@ static s32 piix4_access_sb800(struct i2c
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;
-	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != (port << 1))
-		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | (port << 1),
+	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
+		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
 		       SB800_PIIX4_SMB_IDX + 1);
 
 	retval = piix4_access(adap, addr, flags, read_write,

@@ -676,7 +676,7 @@ static int piix4_add_adapter(struct pci_
 
 	adapdata->smba = smba;
 	adapdata->sb800_main = sb800_main;
-	adapdata->port = port;
+	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;

@@ -812,7 +812,7 @@ static void piix4_adap_remove(struct i2c
Quoted 6 lines

drivers/i2c/busses/i2c-piix4.c static void piix4_adap_remove(struct i2c · line 815

Quoted 4 lines
drivers/i2c/busses/i2c-piix4.c +1 -1
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == 0) {
+		if (adapdata->port == (0 << 1)) {

I suppose this is only for documentation purposes, right?

Yes. This change wasn't originally part of the patch, as I know it is a no-op, but I thought it was more correct to include it.

Quoted 6 lines
Reviewed-by Mika Westerberg <[email protected]>
Quoted 4 lines
drivers/i2c/busses/i2c-piix4.c +0 -0
 			release_region(adapdata->smba, SMBIOSIZE);
 			if (adapdata->sb800_main)
 				release_region(SB800_PIIX4_SMB_IDX, 2);

Thanks for the reviews!

Signature · 2 lines
Jean Delvare
SUSE L3 Support
Applied
#17 lore raw

Re: [PATCH 3/3] i2c-piix4: Pre-shift the port number

In reply to Jean Delvare
Quoted 8 lines · Jean Delvare

Shift the port number at initialization time, so that it is ready to use at run time. That way we don't have to do it again for every SMBus transaction.

Signed-off-by Jean Delvare <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Christian Fetzer <[email protected]>
Cc Wolfram Sang <[email protected]>

Applied to for-next, thanks!