Hi Alek,
On Tue, 20 May 2008 13:56:35 +0800, Alek Du wrote:
> Here is my patch v3, please help to review it:
Here you go. It's getting a lot better but a couple things still need
to be fixed:
> This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) i2c bus
> support.
>
> Signed-off-by: Alek Du <[EMAIL PROTECTED]>
> ---
> drivers/i2c/busses/Kconfig | 10 ++
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-isch.c | 335
> +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 346 insertions(+), 0 deletions(-)
> create mode 100644 drivers/i2c/busses/i2c-isch.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 48438cc..c052b14 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -267,6 +267,16 @@ config I2C_IOP3XX
> This driver can also be built as a module. If so, the module
> will be called i2c-iop3xx.
>
> +config I2C_ISCH
> + tristate "Intel SCH SMBus 1.0"
> + depends on PCI
> + help
> + Say Y if you want to use IIC/SMBus controller on
Please avoid the "IIC" spelling and always use "I2C". In this
particular case, it's not correct anyway: the SCH includes an SMBus 1
controller, NOT an I2C controller.
> + the Intel SCH based systems.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-isch.
> +
> config I2C_IXP2000
> tristate "IXP2000 GPIO-Based I2C Interface (DEPRECATED)"
> depends on ARCH_IXP2000
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index e8c882a..bb3c3f6 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_I2C_I801) += i2c-i801.o
> obj-$(CONFIG_I2C_I810) += i2c-i810.o
> obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
> obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o
> +obj-$(CONFIG_I2C_ISCH) += i2c-isch.o
> obj-$(CONFIG_I2C_IXP2000) += i2c-ixp2000.o
> obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
> obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> new file mode 100644
> index 0000000..41540c5
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -0,0 +1,335 @@
> +/*
> + i2c-isch.c - Linux kernel driver for Intel SCH chipset SMBus
> + - Based on i2c-piix4.c
> + Copyright (c) 1998 - 2002 Frodo Looijaard <[EMAIL PROTECTED]> and
> + Philip Edelbrock <[EMAIL PROTECTED]>
> + - Intel SCH support
> + Copyright (c) 2007 - 2008 Jacob Jun Pan <[EMAIL PROTECTED]>
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License version 2 as
> + published by the Free Software Foundation.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +
> +/*
> + Supports:
> + Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L)
> + Note: we assume there can only be one device, with one SMBus interface.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/stddef.h>
> +#include <linux/ioport.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +
> +/* SCH SMBus address offsets */
> +#define SMBHSTCNT (0 + sch_smba)
> +#define SMBHSTSTS (1 + sch_smba)
> +#define SMBHSTADD (4 + sch_smba) /* TSA */
> +#define SMBHSTCMD (5 + sch_smba)
> +#define SMBHSTDAT0 (6 + sch_smba)
> +#define SMBHSTDAT1 (7 + sch_smba)
> +#define SMBBLKDAT (0x20 + sch_smba)
> +
> +/* count for request_region */
> +#define SMBIOSIZE 64
> +
> +/* PCI Address Constants */
> +#define SMBBA_SCH 0x40
> +
> +/* Other settings */
> +#define MAX_TIMEOUT 500
> +
> +/* I2C constants */
> +#define SCH_QUICK 0x00
> +#define SCH_BYTE 0x01
> +#define SCH_BYTE_DATA 0x02
> +#define SCH_WORD_DATA 0x03
> +#define SCH_BLOCK_DATA 0x05
> +
> +static unsigned short sch_smba;
> +static struct pci_driver sch_driver;
> +static struct i2c_adapter sch_adapter;
> +
> +/*
> + * Start the i2c transaction -- the i2c_access will prepare the transaction
> + * and this function will execute it.
> + * return 0 for success and others for failure.
> + */
> +static int sch_transaction(void)
> +{
> + int temp;
> + int result = 0;
> + int timeout = 0;
> +
> + dev_dbg(&sch_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
> + "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb(SMBHSTCNT),
> + inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0),
> + inb(SMBHSTDAT1));
> +
> + /* Make sure the SMBus host is ready to start transmitting */
> + temp = inb(SMBHSTSTS) & 0x0f;
> + if (temp) {
> + /* Can not be busy since we checked it in sch_access */
> + if (temp & 0x01) {
> + dev_dbg(&sch_adapter.dev, "Completion (%02x). "
> + "clear...\n", temp);
I suggest an upper-case C to "Clear" for consistency.
> + } else if (temp & 0x06) {
Not sure why there would be an "else" here - both cases can happen at
the same time, can't they?
> + dev_dbg(&sch_adapter.dev, "SMBus error (%02x). "
> + "Resetting...\n", temp);
> + }
> + outb(temp, SMBHSTSTS);
> + temp = inb(SMBHSTSTS) & 0x0f;
> + if (temp) {
> + dev_err(&sch_adapter.dev,
> + "SMBus is not ready: (%02x)\n", temp);
> + return -EPERM;
-EPERM doesn't look correct. That would be -EAGAIN if there's a hope
for the problem to be solved after some time, else maybe -EBUSY or just
-EIO.
> + }
> + }
> +
> + /* start the transaction by setting bit 4 */
> + outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
> +
> + do {
> + msleep(1);
> + temp = inb(SMBHSTSTS) & 0x0f;
> + } while ((temp & 0x08) && (timeout++ < MAX_TIMEOUT));
> +
> + /* If the SMBus is still busy, we give up */
> + if (timeout >= MAX_TIMEOUT) {
> + dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
> + result = -EAGAIN;
> + }
> + if (temp & 0x04) {
> + result = -EIO;
> + dev_dbg(&sch_adapter.dev, "Bus collision! SMBus may be "
> + "locked until next hard reset. (sorry!)\n");
> + /* Clock stops and slave is stuck in mid-transmission */
> + } else if (temp & 0x02) {
> + result = -ENXIO;
> + dev_dbg(&sch_adapter.dev, "Error: no response!\n");
> + } else if (temp & 0x01) {
> + dev_dbg(&sch_adapter.dev, "Post complete!\n");
> + outb(temp, SMBHSTSTS);
> + temp = inb(SMBHSTSTS) & 0x07;
> + if (temp & 0x06) {
> + /* Completion clear failed */
> + dev_dbg(&sch_adapter.dev, "Failed reset at end of "
> + "transaction (%02x), Bus error\n", temp);
> + }
> + } else {
> + result = -EIO;
> + /* have to use dev_dbg here, dev_err will mess up i2cdetect */
> + dev_dbg(&sch_adapter.dev, "Transaction failed.\n");
Oh, really? i2cdetect should receive nacks when devices aren't there,
and this is supposedly covered by (temp & 0x02) above, where I had you
return -ENXIO. If this isn't the case then (temp & 0x02) should return
-EIO and print an error message, and the fallback case should return
-ENXIO. As a general rule, we want dev_dbg() and -ENXIO for the nack
case, and dev_err() for all other error cases.
> + }
> + dev_dbg(&sch_adapter.dev, "Transaction (post): CNT=%02x, CMD=%02x, "
> + "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb(SMBHSTCNT),
> + inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0),
> + inb(SMBHSTDAT1));
> + return result;
> +}
> +
> +/*
> + * This is the main access entry for i2c-sch access
> + * adap is i2c_adapter pointer, addr is the i2c device bus address,
> read_write
> + * (0 for read and 1 for write), size is i2c transaction type and data is the
> + * union of transaction for data to be transfered or data read from bus.
> + * return 0 for success and others for failure.
> + */
> +static s32 sch_access(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data)
> +{
> + int i, len, temp, rc;
> +
> + /* Make sure the SMBus host is not busy */
> + temp = inb(SMBHSTSTS) & 0x0f;
> + if (temp & 0x08) {
> + dev_dbg(&sch_adapter.dev, "SMBus busy (%02x)\n", temp);
> + return -EAGAIN;
> + }
> + dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
> + (read_write)?"READ":"WRITE");
> + switch (size) {
> + case I2C_SMBUS_QUICK:
> + outb((addr << 1) | read_write, SMBHSTADD);
> + size = SCH_QUICK;
> + break;
> + case I2C_SMBUS_BYTE:
> + outb((addr << 1) | read_write, SMBHSTADD);
> + if (read_write == I2C_SMBUS_WRITE)
> + outb(command, SMBHSTCMD);
> + size = SCH_BYTE;
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + outb((addr << 1) | read_write, SMBHSTADD);
> + outb(command, SMBHSTCMD);
> + if (read_write == I2C_SMBUS_WRITE)
> + outb(data->byte, SMBHSTDAT0);
> + size = SCH_BYTE_DATA;
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + outb((addr << 1) | read_write, SMBHSTADD);
> + outb(command, SMBHSTCMD);
> + if (read_write == I2C_SMBUS_WRITE) {
> + outb(data->word & 0xff, SMBHSTDAT0);
> + outb((data->word & 0xff00) >> 8, SMBHSTDAT1);
> + }
> + size = SCH_WORD_DATA;
> + break;
> + case I2C_SMBUS_BLOCK_DATA:
> + outb((addr << 1) | read_write, SMBHSTADD);
> + outb(command, SMBHSTCMD);
> + if (read_write == I2C_SMBUS_WRITE) {
> + len = data->block[0];
> + if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
> + return -EINVAL;
> + outb(len, SMBHSTDAT0);
> + for (i = 1; i <= len; i++)
> + outb(data->block[i], SMBBLKDAT+i-1);
> + }
> + size = SCH_BLOCK_DATA;
> + break;
> + default:
> + dev_err(&adap->dev, "I2C transaction not supported!\n");
The standard error message for this had become:
dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
(See http://lists.lm-sensors.org/pipermail/i2c/2008-May/003715.html )
> + return -EOPNOTSUPP;
> + }
> + dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
> + outb((inb(SMBHSTCNT) & 0xb0) | (size & 0x7), SMBHSTCNT);
> +
> + rc = sch_transaction();
> + if (rc) /* Error in transaction */
> + return rc;
> +
> + if ((read_write == I2C_SMBUS_WRITE) || (size == SCH_QUICK))
> + return 0;
> +
> + switch (size) {
> + case SCH_BYTE:
> + case SCH_BYTE_DATA:
> + data->byte = inb(SMBHSTDAT0);
> + break;
> + case SCH_WORD_DATA:
> + data->word = inb(SMBHSTDAT0) + (inb(SMBHSTDAT1) << 8);
> + break;
> + case SCH_BLOCK_DATA:
> + data->block[0] = inb(SMBHSTDAT0);
> + if (data->block[0] == 0 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
> + return -EPROTO;
> + for (i = 1; i <= data->block[0]; i++)
> + data->block[i] = inb(SMBBLKDAT+i-1);
> + break;
> + }
> + return 0;
> +}
> +
> +static u32 sch_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm smbus_algorithm = {
> + .smbus_xfer = sch_access,
> + .functionality = sch_func,
> +};
> +
> +static struct i2c_adapter sch_adapter = {
> + .owner = THIS_MODULE,
> + .class = I2C_CLASS_HWMON,
> + .algo = &smbus_algorithm,
> +};
> +
> +static struct pci_device_id sch_ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC),},
The comma before the closing parenthesis should instead be a space.
> + { 0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, sch_ids);
> +
> +static int __devinit sch_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + int retval;
> +
> + /* driver_data might come from user-space, so check it */
> + if (id->driver_data & 1 || id->driver_data > 0xff)
> + return -EINVAL;
You no longer use driver_data so this test should go away.
> +
> + pci_read_config_word(dev, SMBBA_SCH, &sch_smba);
> + if (sch_smba == 0) {
> + dev_err(&dev->dev, "SMBus base address uninitialized\n");
> + return -ENODEV;
> + }
> +
> + if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> + dev_err(&dev->dev, "SMBus region 0x%x already in use!\n",
> + sch_smba);
> + return -EBUSY;
> + }
> + dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
> +
> + /* set up the sysfs linkage to our parent device */
> + sch_adapter.dev.parent = &dev->dev;
> +
> + snprintf(sch_adapter.name, sizeof(sch_adapter.name),
> + "SMBus SCH adapter at %04x", sch_smba);
> +
> + retval = i2c_add_adapter(&sch_adapter);
> + if (retval) {
> + dev_err(&dev->dev, "Couldn't register adapter!\n");
> + release_region(sch_smba, SMBIOSIZE);
> + sch_smba = 0;
> + }
> +
> + return retval;
> +}
> +
> +static void __devexit sch_remove(struct pci_dev *dev)
> +{
> + if (sch_smba) {
> + i2c_del_adapter(&sch_adapter);
> + release_region(sch_smba, SMBIOSIZE);
> + sch_smba = 0;
> + }
> +}
> +
> +static struct pci_driver sch_driver = {
> + .name = "isch_smbus",
> + .id_table = sch_ids,
> + .probe = sch_probe,
> + .remove = __devexit_p(sch_remove),
> + .dynids.use_driver_data = 1,
No longer needed.
> +};
> +
> +static int __init i2c_sch_init(void)
> +{
> + return pci_register_driver(&sch_driver);
> +}
> +
> +static void __exit i2c_sch_exit(void)
> +{
> + pci_unregister_driver(&sch_driver);
> +}
> +
> +MODULE_AUTHOR("Jacob Pan <[EMAIL PROTECTED]>");
> +MODULE_DESCRIPTION("Intel SCH SMBus driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_sch_init);
> +module_exit(i2c_sch_exit);
Please send a final update of this patch and I'll include it in my i2c
tree (so it goes in linux-next.)
--
Jean Delvare
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
