Hi Shane,
On Fri, 20 Feb 2009 17:43:47 +0800, Shane Huang wrote:
> The original patch is updated with Jean's nice suggestions,
> please check it.
>
> Thanks.
> Shane
>
> === CUT HERE ===
> This version of driver adds support for the AMD SB800 Family series of products.
> Major changes include the changes to addressing the SMBUS registers at different
> location from the locations in the previous compatible parts from AMD such as
> SB400/SB600/SB700. For SB800, the main features and register definitions of
> SMBUS and other interfaces are still compatible with the previous products with
> the only change being in how to access the internal registers for these blocks
> may differ.
>
> Signed-off-by: Shane Huang <shane.huang@xxxxxxx>
This new version of the patch looks acceptable to me. Even though I
find it unfortunate that ATI/AMD changed the hardware implementation to
something which is more complex to handle with no good reason that I
can see :(
>
> diff -ruN a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> --- a/drivers/i2c/busses/i2c-piix4.c 2009-02-20 01:08:40.000000000 +0800
> +++ b/drivers/i2c/busses/i2c-piix4.c 2009-02-20 01:34:45.000000000 +0800
> @@ -226,6 +226,68 @@
> return 0;
> }
>
> +static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> + const struct pci_device_id *id)
> +{
> + unsigned short smba_idx = 0xcd6;
> + u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
> +
> + /* SB800 SMBus does not support forcing address */
> + if (force || force_addr)
> + dev_info(&PIIX4_dev->dev, "SB800 SMBus does not support "
> + "forcing address!\n");
I would promote this to dev_warn(), or even dev_err() and fail.
> +
> + /* Determine the address of the SMBus areas */
> + 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);
> +
> + if ((smba_en_lo & 1) == 0) {
> + dev_err(&PIIX4_dev->dev,
> + "Host SMBus controller not enabled!\n");
> + return -ENODEV;
> + }
> +
> + piix4_smba = ((smba_en_hi << 8) | smba_en_lo) & 0xffe0;
> + if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
> + return -EBUSY;
> +
> + if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
> + dev_err(&PIIX4_dev->dev, "SMBus region 0x%x already in use!\n",
> + piix4_smba);
> + return -EBUSY;
> + }
> +
> + /* Request the SMBus I2C bus config region */
> + if (!request_region(piix4_smba + i2ccfg_offset, 1, "i2ccfg")) {
> + dev_err(&PIIX4_dev->dev, "SMBus I2C bus config region "
> + "0x%x already in use!\n", piix4_smba + i2ccfg_offset);
> + release_region(piix4_smba, SMBIOSIZE);
> + piix4_smba = 0;
> + return -EBUSY;
> + }
> + i2ccfg = inb_p(piix4_smba + i2ccfg_offset);
> + release_region(piix4_smba + i2ccfg_offset, 1);
> +
> + if (i2ccfg & 1)
> + dev_dbg(&PIIX4_dev->dev, "Using IRQ for SMBus.\n");
> + else
> + dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus.\n");
> +
> + dev_info(&PIIX4_dev->dev,
> + "SMBus Host Controller at 0x%x, revision %d\n",
> + piix4_smba, i2ccfg >> 4);
> +
> + return 0;
> +}
> +
> static int piix4_transaction(void)
> {
> int temp;
> @@ -434,7 +496,14 @@
> {
> int retval;
>
> - retval = piix4_setup(dev, id);
> + if ((dev->vendor == PCI_VENDOR_ID_ATI) &&
> + (dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS) &&
> + (dev->revision >= 0x40))
> + /* base address location etc changed in SB800 */
> + retval = piix4_setup_sb800(dev, id);
> + else
> + retval = piix4_setup(dev, id);
> +
> if (retval)
> return retval;
>
All the rest looks good, I'll apply your patch now.
--
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
