On Mon, 4 Jun 2012 21:49:24 -0400, Andrew Armenia wrote:
> Some AMD chipsets, such as the SP5100, have an auxiliary SMBus with a second
> set of registers. This patch adds support for the SP5100 and should work on
> similar chipsets. Tested on ASUS KCMA-D8 motherboard.
The SP5100 isn't even documented as being supported. Can you please add
it to Documentation/i2c/busses/i2c-piix4, drivers/i2c/busses/Kconfig,
and the header comment? Or is it a different code name for an already
supported south bridge? I admit I stopped following AMD chipsets some
times ago.
>
> Signed-off-by: Andrew Armenia <andrew@xxxxxxxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-piix4.c | 83 ++++++++++++++++++++++++++++++++++++----
> 1 file changed, 76 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 8a87b3f..972b604 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -25,7 +25,8 @@
> AMD Hudson-2
> SMSC Victory66
>
> - Note: we assume there can only be one device, with one SMBus interface.
> + Note: we assume there can only be one device, with one or two SMBus
> + interfaces.
> */
>
> #include <linux/module.h>
> @@ -66,6 +67,7 @@
> #define SMBSHDW1 0x0D4
> #define SMBSHDW2 0x0D5
> #define SMBREV 0x0D6
> +#define SMBAUXBA 0x058
Keeping the list sorted by register offset would seem reasonable.
>
> /* Other settings */
> #define MAX_TIMEOUT 500
> @@ -130,6 +132,8 @@ struct i2c_piix4_adapdata {
> unsigned short smba;
> };
>
> +static void piix4_adap_remove(struct i2c_adapter *adap);
> +
Please just reorder the functions (ideally at the time you introduce
them) so that you don't need this forward declaration.
> static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
> const struct pci_device_id *id,
> unsigned short *smba)
> @@ -300,6 +304,45 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> return 0;
> }
>
> +static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
> + const struct pci_device_id *id,
> + unsigned short base_reg_addr,
> + unsigned short *smba)
> +{
> + /* Set up auxiliary SMBus controllers found on some AMD
> + * chipsets e.g. SP5100 */
> + unsigned short piix4_smba;
> +
> + /* Read address of auxiliary SMBus controller */
> + pci_read_config_word(PIIX4_dev, base_reg_addr, &piix4_smba);
> + piix4_smba &= 0xffe0;
> +
> + if (piix4_smba == 0) {
> + dev_err(&PIIX4_dev->dev, "Aux SMBus base address "
> + "uninitialized - upgrade BIOS\n");
This case could be OK if the board vendor simply did not need a second
SMBus channel. So we shouldn't spit an error message in this case,
maybe a debug message if you want but that's about it.
> + return -ENODEV;
> + }
> +
> + if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
> + return -ENODEV;
> +
> + if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
> + dev_err(&PIIX4_dev->dev, "Aux SMBus region 0x%x already"
> + " in use!\n", piix4_smba);
White space at end of first half please, for consistency.
> + return -EBUSY;
> + }
> +
> + dev_info(&PIIX4_dev->dev,
> + "Auxiliary SMBus Host Controller at 0x%x\n",
You should probably write "Auxiliary" in the previous message messages
too, for consistency and readability.
> + piix4_smba
> + );
> +
> + *smba = piix4_smba;
> +
> + return 0;
> +}
> +
> +
> static int piix4_transaction(unsigned short piix4_smba)
> {
> int temp;
> @@ -505,6 +548,7 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
> MODULE_DEVICE_TABLE (pci, piix4_ids);
>
> static struct i2c_adapter *piix4_main_adapter;
> +static struct i2c_adapter *piix4_aux_adapter;
>
> /* register piix4 I2C adapter at the given base address */
> static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> @@ -562,9 +606,33 @@ static int __devinit piix4_probe(struct pci_dev *dev,
> retval = piix4_setup(dev, id, &smba);
>
> if (retval)
> - return retval;
> + goto error;
> +
> + retval = piix4_add_adapter(dev, smba, &piix4_main_adapter);
> + if (retval)
> + goto error;
>
> - return piix4_add_adapter(dev, smba, &piix4_main_adapter);
> + /* check for AMD SP5100 (maybe others) with aux SMBus port */
> + if (dev->vendor == PCI_VENDOR_ID_ATI &&
> + dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
> + dev->revision == 0x3d) {
Hmm, that would be an ATI SB600 or SB700, so not a recent chip? This
strict check on revision is likely to exclude some systems where this
code should run. Given that the SB800 started at revision 0x40 as far
as I know, shouldn't we test for < 0x40 here?
> +
> + retval = piix4_setup_aux(dev, id, SMBAUXBA, &smba);
> + if (retval)
> + goto error;
> +
> + retval = piix4_add_adapter(dev, smba, &piix4_aux_adapter);
> + if (retval)
> + goto error;
I don't get the logic here. If we fail to register the auxiliary SMBus,
it is certainly not a reason to give up the working main SMBus.
Especially with my comment above that the Auxiliary SMBus may have been
disabled by the vendor on purpose.
> + }
> +
> + return 0;
> +
> +error:
> + /* clean up any adapters that were already added */
> + piix4_adap_remove(piix4_main_adapter);
> + piix4_adap_remove(piix4_aux_adapter);
You're not clearing the pointers, this could cause trouble on hot-plug.
> + return retval;
> }
>
> /* Remove the adapter and its associated IO region */
> @@ -572,6 +640,9 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
> {
> struct i2c_piix4_adapdata *adapdata;
>
> + if (adap == NULL)
> + return;
> +
If you want to do that, it would be better done right in patch 2/3, to
avoid changing code back and forth.
> adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);
> if (adapdata->smba) {
> i2c_del_adapter(adap);
> @@ -585,10 +656,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>
> static void __devexit piix4_remove(struct pci_dev *dev)
> {
> - if (piix4_main_adapter) {
> - piix4_adap_remove(piix4_main_adapter);
> - piix4_main_adapter = NULL;
> - }
> + piix4_adap_remove(piix4_main_adapter);
> + piix4_adap_remove(piix4_aux_adapter);
Here again you're no longer clearing the pointers afterward, this could
cause trouble (unlikely, but better safe than sorry.)
> }
>
> static struct pci_driver piix4_driver = {
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
