Support multiplexed main SMBus interface on SB80
v2 cover Support multiplexed main SMBus interface on SB80
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/[email protected]/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=ffUnfortunately 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
v2:
- Incorporated changes requested by Mika
- remove adapter in reverse order
- ERROR label
- request base address index region only once
Christian Fetzer (5):
i2c-piix4: Optionally release smba in piix4_adap_remove
i2c-piix4: Convert piix4_main_adapter to array
i2c-piix4: Request base address index region once for SB800
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 | 172 ++++++++++++++++++++++++++++++++++------- 1 file changed, 144 insertions(+), 28 deletions(-)
Signature · 1 line
1.9.1
PATCH v2 1/5 i2c-piix4: Optionally release smba in piix4_adap_remove
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.
--- drivers/i2c/busses/i2c-piix4.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +5 -4
@@ -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 ; } }
Signature · 1 line
1.9.1
Re: [PATCH v2 1/5] i2c-piix4: Optionally release smba in piix4_adap_remove
drivers/i2c/busses/i2c-piix4.c static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 663
Quoted 2 lines · Christian Fetzer
drivers/i2c/busses/i2c-piix4.c +1
-1-static void piix4_adap_remove (struct i2c_adapter *adap) +static void piix4_adap_remove (struct i2c_adapter *adap,int free_smba)
How about bool instead?
PATCH v2 2/5 i2c-piix4: Convert piix4_main_adapter to array
In reply to Christian FetzerThe 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.
--- drivers/i2c/busses/i2c-piix4.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-piix4.c +13 -5
@@ -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 =PIIX4_MAX_ADAPTERS -1 ; port >=0 ; port--) { +if (piix4_main_adapters[port]) { +piix4_adap_remove (piix4_main_adapters[port], + port ==0 ); + piix4_main_adapters[port] =NULL ; + } }if (piix4_aux_adapter) {
Signature · 1 line
1.9.1
Re: [PATCH v2 2/5] i2c-piix4: Convert piix4_main_adapter to array
Quoted 9 lines · Christian Fetzer
The SB800 chipset supports a multiplexed main SMBus controller with four ports. Therefore the static variable
piix4_main_adapteris converted into apiix4_main_adaptersarray that can hold onei2c_adapterfor 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 <[email protected]>
Looks good to me,
Re: [PATCH v2 2/5] i2c-piix4: Convert piix4_main_adapter to array
In reply to Christian FetzerQuoted 26 lines · Christian Fetzer
The SB800 chipset supports a multiplexed main SMBus controller with four ports. Therefore the static variable
piix4_main_adapteris converted into apiix4_main_adaptersarray that can hold onei2c_adapterfor each multiplexed port.The auxiliary adapter remains unchanged since it represents the second
(not multiplexed) SMBus controller on the SB800 chipset.(quoted diff) +0
-0@@ -675,9 +678,14 @@ static void piix4_adap_remove(struct i2c_adapter *adap, int free_smba)
static voidpiix4_remove(struct pci_dev *dev)
{drivers/i2c/busses/i2c-piix4.c +8
-3-if (piix4_main_adapter) { -piix4_adap_remove (piix4_main_adapter,1 ); - piix4_main_adapter =NULL ; +int port; + +for (port =PIIX4_MAX_ADAPTERS -1 ; port >=0 ; port--) { +if (piix4_main_adapters[port]) { +piix4_adap_remove (piix4_main_adapters[port], + port == 0); + piix4_main_adapters[port] =NULL ; + }
Quoted 1 line
}
Would it be
int port = PIIX4_MAX_ADAPTERS;
while (--port) {
if (…) {…}
}
?
--
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH v2 3/5 i2c-piix4: Request base address index region once for SB800
In reply to Christian FetzerRequest the SMBus base address index region once in piix4_probe. This is particularly useful when using the multiplexed adapter in SB800 as it avoids requesting and releasing the region on every transfer.
--- drivers/i2c/busses/i2c-piix4.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)
drivers/i2c/busses/i2c-piix4.c +22 -13
@@ -125,6 +125,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = { { }, }; +/* SB800 globals */ +static unsigned short piix4_smb_idx_sb800; +struct i2c_piix4_adapdata {unsigned short smba; };
@@ -232,7 +235,6 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev, const struct pci_device_id *id, u8 aux) {unsigned short piix4_smba; -unsigned short smba_idx =0xcd6 ;u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;u8 i2ccfg, i2ccfg_offset =0x10 ;
@@ -254,16 +256,10 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,else smb_en = (aux) ?0x28 :0x2c ; -if (!request_region (smba_idx,2 ,"smba_idx" )) { -dev_err (&PIIX4_dev->dev,"SMBus base address index region " -"0x%x already in use!\n" , smba_idx); -return -EBUSY ; - } -outb_p (smb_en, smba_idx); - smba_en_lo =inb_p (smba_idx +1 ); -outb_p (smb_en +1 , smba_idx); - smba_en_hi =inb_p (smba_idx +1 ); -release_region (smba_idx,2 ); +outb_p (smb_en, piix4_smb_idx_sb800); + smba_en_lo =inb_p (piix4_smb_idx_sb800 +1 ); +outb_p (smb_en +1 , piix4_smb_idx_sb800); + smba_en_hi =inb_p (piix4_smb_idx_sb800 +1 );if (!smb_en) { smb_en_status = smba_en_lo &0x10 ;
@@ -616,16 +612,26 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,static int piix4_probe (struct pci_dev *dev,const struct pci_device_id *id) { +unsigned short smba_idx =0xcd6 ;int retval;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 ) { + +if (!request_region (smba_idx,2 ,"smba_idx" )) { +dev_err (&dev->dev,"SMBus base address index region " +"0x%x already in use!\n" , smba_idx); +return -EBUSY ; + } + piix4_smb_idx_sb800 = smba_idx; +/* base address location etc changed in SB800 */ retval =piix4_setup_sb800 (dev, id,0 ); -else + }else { retval =piix4_setup (dev, id); + }/* If no main SMBus found, give up */ if (retval <0 )
@@ -692,6 +698,9 @@ static void piix4_remove(struct pci_dev *dev)piix4_adap_remove (piix4_aux_adapter,1 ); piix4_aux_adapter =NULL ; } + +if (piix4_smb_idx_sb800) +release_region (piix4_smb_idx_sb800,2 ); }static struct pci_driver piix4_driver = {
Signature · 1 line
1.9.1
Re: [PATCH v2 3/5] i2c-piix4: Request base address index region once for SB800
Quoted 61 lines · Christian Fetzer
Request the SMBus base address index region once in
piix4_probe. This is particularly useful when using the multiplexed adapter in SB800 as it avoids requesting and releasing the region on every transfer.Signed-off-by Christian Fetzer <[email protected]>--- drivers/i2c/busses/i2c-piix4.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)drivers/i2c/busses/i2c-piix4.c +8
-11@@ -125,6 +125,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = { { }, }; +/* SB800 globals */ +static unsigned short piix4_smb_idx_sb800; +struct i2c_piix4_adapdata {unsigned short smba; };@@ -232,7 +235,6 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev, const struct pci_device_id *id, u8 aux) {unsigned short piix4_smba; -unsigned short smba_idx =0xcd6 ;u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;u8 i2ccfg, i2ccfg_offset =0x10 ;@@ -254,16 +256,10 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,else smb_en = (aux) ?0x28 :0x2c ; -if (!request_region (smba_idx,2 ,"smba_idx" )) { -dev_err (&PIIX4_dev->dev,"SMBus base address index region " -"0x%x already in use!\n" , smba_idx); -return -EBUSY ; - } -outb_p (smb_en, smba_idx); - smba_en_lo =inb_p (smba_idx +1 ); -outb_p (smb_en +1 , smba_idx); - smba_en_hi =inb_p (smba_idx +1 ); -release_region (smba_idx,2 ); +outb_p (smb_en, piix4_smb_idx_sb800); + smba_en_lo =inb_p (piix4_smb_idx_sb800 +1 ); +outb_p (smb_en +1 , piix4_smb_idx_sb800); + smba_en_hi =inb_p (piix4_smb_idx_sb800 +1 );if (!smb_en) { smb_en_status = smba_en_lo &0x10 ;@@ -616,16 +612,26 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,static int piix4_probe (struct pci_dev *dev,const struct pci_device_id *id) { +unsigned short smba_idx =0xcd6 ;int retval; if ((dev->vendor == PCI_VENDOR_ID_ATI && dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 628
Quoted 10 lines
drivers/i2c/busses/i2c-piix4.c +8
-1dev->revision >= 0x40) || - dev->vendor == PCI_VENDOR_ID_AMD) + dev->vendor == PCI_VENDOR_ID_AMD) { + +if (!request_region (smba_idx,2 ,"smba_idx" )) { +dev_err (&dev->dev,"SMBus base address index region " +"0x%x already in use!\n" , smba_idx); +return -EBUSY ; + } + piix4_smb_idx_sb800 = smba_idx;
If I read this right, piix4_smb_idx_sb800 will always be 0xcd6. Why do you need the global variable at all then? Why not have constant like below?
#define SB800_PIIX4_SMB_IDX 0xcd6
Re: [PATCH v2 3/5] i2c-piix4: Request base address index region once for SB800
In reply to Christian FetzerQuoted 17 lines · Christian Fetzer
Request the SMBus base address index region once in
piix4_probe. This is particularly useful when using the multiplexed adapter in SB800 as it avoids requesting and releasing the region on every transfer.(quoted diff) +0
-0@@ -616,16 +612,26 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
static intpiix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
{drivers/i2c/busses/i2c-piix4.c +1
-0+unsigned short smba_idx =0xcd6 ;int retval;
if ((dev->vendor==PCI_VENDOR_ID_ATI&&dev->device==PCI_DEVICE_ID_ATI_SBX00_SMBUS&&dev->revision>=0x40) ||
drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 622
Quoted 3 lines
drivers/i2c/busses/i2c-piix4.c +2
-1- dev->vendor == PCI_VENDOR_ID_AMD) + dev->vendor == PCI_VENDOR_ID_AMD) { +
Redundant empty line.
drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 625
Quoted 4 lines
(quoted diff) +2
-0+ if (!request_region(smba_idx, 2, "smba_idx")) { + dev_err(&dev->dev, "SMBus base address indexregion "
drivers/i2c/busses/i2c-piix4.c +1
-0+ "0x%x already in use!\n", smba_idx);
Do not split string literals, something like following will be okay.
dev_err(&dev->dev,
"SMBus base address index region 0x%x already in use!\n", smba_idx);
Quoted 10 lines
drivers/i2c/busses/i2c-piix4.c +4
-0+return -EBUSY ; + } + piix4_smb_idx_sb800 = smba_idx; +/* base address location etc changed in SB800 */ retval =
piix4_setup_sb800(dev, id, 0);drivers/i2c/busses/i2c-piix4.c +1
-1-else + }else {retval =
piix4_setup(dev, id);(quoted diff) +1
-0+ }
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH v2 4/5 i2c-piix4: Add support for multiplexed main adapter in SB800
In reply to Christian FetzerThe 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).
--- drivers/i2c/busses/i2c-piix4.c | 102 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 97 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-piix4.c +97 -5
@@ -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 */
@@ -126,10 +130,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = { };/* SB800 globals */ +DEFINE_MUTEX (piix4_mutex_sb800);static unsigned short piix4_smb_idx_sb800;struct i2c_piix4_adapdata {unsigned short smba; +unsigned short port; }; static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -309,6 +315,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 );
@@ -523,6 +531,42 @@ 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). +* Returns negative errno on error. +* +* Note: The selected port must be returned to the initial selection to avoid +* problems on certain systems. +*/ +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); +u8 smba_en_lo, smb_en =0x2c ; +u8 port; +int retval; + +mutex_lock (&piix4_mutex_sb800); + +outb_p (smb_en, piix4_smb_idx_sb800); + smba_en_lo =inb_p (piix4_smb_idx_sb800 +1 ); + + port = adapdata->port; +if ((smba_en_lo &6 ) != (port <<1 )) +outb_p ((smba_en_lo & ~6 ) | (port <<1 ), + piix4_smb_idx_sb800 +1 ); + + retval =piix4_access (adap, addr, flags, read_write, + command, size, data); + +outb_p (smba_en_lo, piix4_smb_idx_sb800 +1 ); + +mutex_unlock (&piix4_mutex_sb800); + +return retval; +} +static u32 piix4_func (struct i2c_adapter *adapter) { return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
@@ -535,6 +579,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 ) },
@@ -610,6 +659,42 @@ 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) {unsigned short smba_idx =0xcd6 ;
@@ -629,19 +714,26 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)/* base address location etc changed in SB800 */ retval =piix4_setup_sb800 (dev, id,0 ); +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; + +/* 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; -/* Try to register main SMBus adapter, give up if we can't */ - retval =piix4_add_adapter (dev, retval, &piix4_main_adapters[0 ]); -if (retval <0 ) -return retval; -/* Check for auxiliary SMBus on some AMD chipsets */ retval = -ENODEV ;
Signature · 1 line
1.9.1
Re: [PATCH v2 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800
Quoted 8 lines · 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_enregister (0x2C).Only one port can be active at any point in time therefore a mutex is needed in order to synchronize access.
Quoted 6 lines
Tested on HP ProLiant MicroServer G7 N54L (where this patch adds support to access sensor data from the w83795adg).
Cc Thomas Brandon <[email protected]>Cc Eddi De Pieri <[email protected]>Signed-off-by Christian Fetzer <[email protected]>
Few minor comments, see below.
Regardless of them,
Quoted 49 lines
--- drivers/i2c/busses/i2c-piix4.c | 102 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 97 insertions(+), 5 deletions(-)drivers/i2c/busses/i2c-piix4.c +8
-0@@ -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 */ @@ -126,10 +130,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = { };/* SB800 globals */ +DEFINE_MUTEX (piix4_mutex_sb800);static unsigned short piix4_smb_idx_sb800;struct i2c_piix4_adapdata {unsigned short smba; +unsigned short port; }; static int piix4_setup(struct pci_dev *PIIX4_dev,@@ -309,6 +315,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 );
drivers/i2c/busses/i2c-piix4.c static s32 piix4_access(struct i2c_adapter * adap, u16 addr, · line 536
Quoted 6 lines
drivers/i2c/busses/i2c-piix4.c +3
-0@@ -523,6 +531,42 @@ 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). +* Returns negative errno on error.
Block comments look like:
/*
* Handles access to multiple...
*
Quoted 65 lines
drivers/i2c/busses/i2c-piix4.c +51
-0+* +* Note: The selected port must be returned to the initial selection to avoid +* problems on certain systems. +*/ +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); +u8 smba_en_lo, smb_en =0x2c ; +u8 port; +int retval; + +mutex_lock (&piix4_mutex_sb800); + +outb_p (smb_en, piix4_smb_idx_sb800); + smba_en_lo =inb_p (piix4_smb_idx_sb800 +1 ); + + port = adapdata->port; +if ((smba_en_lo &6 ) != (port <<1 )) +outb_p ((smba_en_lo & ~6 ) | (port <<1 ), + piix4_smb_idx_sb800 +1 ); + + retval =piix4_access (adap, addr, flags, read_write, + command, size, data); + +outb_p (smba_en_lo, piix4_smb_idx_sb800 +1 ); + +mutex_unlock (&piix4_mutex_sb800); + +return retval; +} +static u32 piix4_func (struct i2c_adapter *adapter) { return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |@@ -535,6 +579,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 ) },@@ -610,6 +659,42 @@ 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;
drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 684
Quoted 10 lines
drivers/i2c/busses/i2c-piix4.c +10
-0+ + 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" );
You don't need to split the error string here. It makes grepping the error messages harder. Just write it like:
dev_err(&dev->dev, "Error setting up SB800 adapters. Unregistering all adapters!\n");
You may want to shorten the error message, though.
Quoted 50 lines
drivers/i2c/busses/i2c-piix4.c +25
-5+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) {unsigned short smba_idx =0xcd6 ;@@ -629,19 +714,26 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)/* base address location etc changed in SB800 */ retval =piix4_setup_sb800 (dev, id,0 ); +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; + +/* 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; -/* Try to register main SMBus adapter, give up if we can't */ - retval =piix4_add_adapter (dev, retval, &piix4_main_adapters[0 ]); -if (retval <0 ) -return retval; -/* Check for auxiliary SMBus on some AMD chipsets */ retval = -ENODEV ;Signature · 1 line
1.9.1
Re: [PATCH v2 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800
In reply to Christian FetzerQuoted 32 lines · 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_enregister (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).
+static int
piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)drivers/i2c/busses/i2c-piix4.c +8
-0+{ + 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]);
drivers/i2c/busses/i2c-piix4.c +4
-0+if (retval <0 ) + goto error; + + piix4_main_adapters[port]->algo =
&piix4_smbus_algorithm_sb800;(quoted diff) +2
-0+ + adapdata =
i2c_get_adapdata(piix4_main_adapters[port]);
drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 684
Quoted 8 lines
drivers/i2c/busses/i2c-piix4.c +8
-0+ adapdata->port = port; + } + +return retval; + +error: +dev_err (&dev->dev,"Error setting up SB800 adapters. " +"Unregistering all adapters!\n" );
This one Mika told already about.
You might use as well
dev_err(&dev->dev,
"…\n")
drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 685
Quoted 1 line
drivers/i2c/busses/i2c-piix4.c +1
-0+for (port--; port >=0 ; port--) {
Isn't look complicated? Like I comment in one of previous patches:
while (--port) {
Quoted 22 lines
(quoted diff) +1
-0+ adapdata =
i2c_get_adapdata(piix4_main_adapters[port]);drivers/i2c/busses/i2c-piix4.c +11
-0+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)
{
unsigned shortsmba_idx=0xcd6;(quoted diff) +0
-0@@ -629,19 +714,26 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
/* base address location etc changed in SB800 */ retval =piix4_setup_sb800(dev, id, 0);
drivers/i2c/busses/i2c-piix4.c static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 721
Quoted 5 lines
drivers/i2c/busses/i2c-piix4.c +5
-0+if (retval <0 ) +return retval; + +/* Try to register multiplexed main SMBus adapter, +* give up if we can't */
Block comment
/*
* text1
* text2
*/
Quoted 26 lines
drivers/i2c/busses/i2c-piix4.c +1
-0+ retval =piix4_add_adapters_sb800 (dev, retval);} else {
retval =piix4_setup(dev, id);drivers/i2c/busses/i2c-piix4.c +4
-0+if (retval <0 ) +return retval; + +/* Try to register main SMBus adapter, give up if we can't */
drivers/i2c/busses/i2c-piix4.c +2
-0+ retval =piix4_add_adapter (dev, retval, + &piix4_main_adapters[0]);}
/* If no main SMBus found, give up */
if (retval < 0)
return retval;(quoted diff) +0
-1- /* Try to register main SMBus adapter, give up if we can't*/
(quoted diff) +0
-1- retval = piix4_add_adapter(dev, retval,&piix4_main_adapters[0]);
drivers/i2c/busses/i2c-piix4.c +0
-3-if (retval <0 ) -return retval; -/* Check for auxiliary SMBus on some AMD chipsets */ retval =
-ENODEV;
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH v2 5/5 i2c-piix4: Add adapter port name support for SB800 chipset
In reply to Christian FetzerThis 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).
--- drivers/i2c/busses/i2c-piix4.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +10 -4
@@ -132,6 +132,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {/* SB800 globals */ DEFINE_MUTEX (piix4_mutex_sb800);static unsigned short piix4_smb_idx_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;
@@ -613,7 +617,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;
@@ -642,7 +646,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);
@@ -667,6 +671,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)for (port =0 ; port <PIIX4_MAX_ADAPTERS ; port++) { retval =piix4_add_adapter (dev, smba, + piix4_main_port_names_sb800[port], &piix4_main_adapters[port]);if (retval <0 ) goto error;
@@ -726,7 +731,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, + retval =piix4_add_adapter (dev, retval,"" , &piix4_main_adapters[0 ]); }
@@ -755,7 +760,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 ;
Signature · 1 line
1.9.1
Re: [PATCH v2 5/5] i2c-piix4: Add adapter port name support for SB800 chipset
Quoted 52 lines · 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 <[email protected]>--- drivers/i2c/busses/i2c-piix4.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)drivers/i2c/busses/i2c-piix4.c +7
-2@@ -132,6 +132,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {/* SB800 globals */ DEFINE_MUTEX (piix4_mutex_sb800);static unsigned short piix4_smb_idx_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;@@ -613,7 +617,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;@@ -642,7 +646,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);@@ -667,6 +671,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)for (port =0 ; port <PIIX4_MAX_ADAPTERS ; port++) { retval =piix4_add_adapter (dev, smba, + piix4_main_port_names_sb800[port], &piix4_main_adapters[port]);if (retval <0 ) goto error;
drivers/i2c/busses/i2c-piix4.c static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 734
Quoted 5 lines
drivers/i2c/busses/i2c-piix4.c +1
-1@@ -726,7 +731,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, + retval = piix4_add_adapter(dev, retval, "",
Why not use some useful name here instead of "" ?
Otherwise looks good,
Quoted 15 lines
drivers/i2c/busses/i2c-piix4.c +2
-1&piix4_main_adapters[0]); }@@ -755,7 +760,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 ;Signature · 1 line
1.9.1
Re: [PATCH v2 5/5] i2c-piix4: Add adapter port name support for SB800 chipset
In reply to Christian FetzerQuoted 9 lines · 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).+static const char *piix4_main_port_names_sb800[4] = {
Would it be constant you defined instead of magic number?
Quoted 5 lines
drivers/i2c/busses/i2c-piix4.c +3
-0+ "SDA0", "SDA2", "SDA3", "SDA4" +}; +static const char *piix4_aux_port_name_sb800 ="SDA1" ;
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy