* [PATCH] i2c: Add Intel SCH I2C SMBus support
@ 2008-04-24 2:05 Alek Du
[not found] ` <20080424100510.09cc2d4b-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Alek Du @ 2008-04-24 2:05 UTC (permalink / raw)
To: i2c-GZX6beZjE8VD60Wz+7aTrA, khali-PUYAD+kWke1g9hUCZPvPmw
i2c: Add Intel SCH I2C SMBus support
This patch adds Intel SCH chipsets (US15W, US15L, UL11L) i2c bus support.
For include/linux/pci_ids.h, Jesse Barnes will apply it to PCI subsytem.
Signed-off-by: Alek Du <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
include/linux/pci_ids.h | 14 ++++++++++++++
drivers/i2c/busses/Kconfig | 7 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-sch.c | 419 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 441 insertions(+), 0 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-sch.c
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 70eb3c8..b72b3b4 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2413,6 +2413,8 @@
#define PCI_DEVICE_ID_INTEL_82443GX_0 0x71a0
#define PCI_DEVICE_ID_INTEL_82443GX_2 0x71a2
#define PCI_DEVICE_ID_INTEL_82372FB_1 0x7601
+#define PCI_DEVICE_ID_INTEL_SCH_LPC 0x8119
+#define PCI_DEVICE_ID_INTEL_SCH_IDE 0x811a
#define PCI_DEVICE_ID_INTEL_82454GX 0x84c4
#define PCI_DEVICE_ID_INTEL_82450GX 0x84c5
#define PCI_DEVICE_ID_INTEL_82451NX 0x84ca
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5fa9c3c..7aad5d1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -244,6 +244,13 @@ config I2C_PIIX4
This driver can also be built as a module. If so, the module
will be called i2c-piix4.
+config I2C_SCH
+ tristate "Intel SCH SMBUS 1.0"
+ depends on I2C && PCI
+ help
+ If you say Y or M to this option, support will be included for the
+ Intel SCH based systems. Module will be called i2c-sch.ko.
+
config I2C_IBM_IIC
tristate "IBM PPC 4xx on-chip I2C interface"
depends on IBM_OCP
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ea7068f..3165bf9 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_I2C_PASEMI) += i2c-pasemi.o
obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o
obj-$(CONFIG_I2C_PMCMSP) += i2c-pmcmsp.o
+obj-$(CONFIG_I2C_SCH) += i2c-sch.o
obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
obj-$(CONFIG_I2C_PROSAVAGE) += i2c-prosavage.o
obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
diff --git a/drivers/i2c/busses/i2c-sch.c b/drivers/i2c/busses/i2c-sch.c
new file mode 100644
index 0000000..4293882
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sch.c
@@ -0,0 +1,419 @@
+/*
+ i2c-sch.c - Part of lm_sensors, Linux kernel modules for hardware
+ monitoring
+ - Based on i2c-piix4.c
+ Copyright (c) 1998 - 2002 Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and
+ Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
+ - Intel SCH support
+ Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+
+ 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
+
+ Function i2c_sch_init and i2c_sch_exit are module init/exit entries, and
+ sch_probe will be called for device initialization, In probe, SCH i2c
+ adapter will be setup by sch_setup and added by i2c_add_adapter. sch_access
+ is the main entry for the i2c-sch bus.
+ Note: we assume there can only be one device, with one SMBus interface.
+*/
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/stddef.h>
+#include <linux/sched.h>
+#include <linux/ioport.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/apm_bios.h>
+#include <linux/io.h>
+
+
+struct sd {
+ const unsigned short mfr;
+ const unsigned short dev;
+ const unsigned char fn;
+ const char *name;
+};
+/* 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 8
+
+/* PCI Address Constants */
+#define SMBBA_SCH 0x040
+
+/* Other settings */
+#define MAX_TIMEOUT 500
+#define ENABLE_INT9 0
+
+/* 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
+
+/* insmod parameters */
+
+/* If force is set to anything different from 0, we forcibly enable the
+ SCH. DANGEROUS! */
+static int force;
+module_param(force, int, 0);
+MODULE_PARM_DESC(force, "Forcibly enable the I2C. DANGEROUS!");
+
+
+static int sch_transaction(void);
+
+static unsigned short sch_smba;
+static struct pci_driver sch_driver;
+static struct i2c_adapter sch_adapter;
+
+/*
+ * sch_probe will call this function to get SMBus base address
+ * sch_dev and id are the arguments from probe functions
+ * return 0 for success and -ENODEV for failure
+ */
+static int __devinit sch_setup(struct pci_dev *sch_dev,
+ const struct pci_device_id *id)
+{
+ unsigned short smbase;
+ if (sch_dev->device != PCI_DEVICE_ID_INTEL_SCH_LPC) {
+ /* match up the function */
+ if (PCI_FUNC(sch_dev->devfn) != id->driver_data)
+ return -ENODEV;
+ dev_info(&sch_dev->dev, "Found %s device\n", pci_name(sch_dev));
+ } else {
+ dev_info(&sch_dev->dev, "Found SCH SMBUS %s device\n",
+ pci_name(sch_dev));
+ /* find SMBUS base address */
+ pci_read_config_word(sch_dev, 0x40, &smbase);
+ dev_info(&sch_dev->dev, "SCH SM base = 0x%04x\n", smbase);
+ }
+
+
+ /* Determine the address of the SMBus areas */
+ if (sch_dev->device == PCI_DEVICE_ID_INTEL_SCH_LPC)
+ pci_read_config_word(sch_dev, SMBBA_SCH, &sch_smba);
+ else
+ sch_smba = 0;
+ sch_smba &= 0xfff0;
+ if (sch_smba == 0) {
+ dev_err(&sch_dev->dev, "SMB base address "
+ "uninitialized - upgrade BIOS or use "
+ "force_addr=0xaddr\n");
+ return -ENODEV;
+ }
+
+ if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
+ dev_err(&sch_dev->dev, "SMB region 0x%x already in use!\n",
+ sch_smba);
+ return -ENODEV;
+ }
+
+ dev_dbg(&sch_dev->dev, "SMBA = 0x%X\n", sch_smba);
+
+ return 0;
+}
+
+/*
+ * 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);
+ if (temp) {
+ if (temp == 1) {
+ dev_dbg(&sch_adapter.dev, "Completion (%02x). "
+ "clear...\n", temp);
+ outb(temp, SMBHSTSTS);
+ } else if (temp & 0xe) {
+ dev_dbg(&sch_adapter.dev, "SMBus error (%02x). "
+ "Resetting...\n", temp);
+ outb(temp, SMBHSTSTS);
+ }
+ temp = inb(SMBHSTSTS);
+ if (temp) {
+ dev_err(&sch_adapter.dev,
+ "SMBus is not ready: (%02x)\n", temp);
+ return -EPERM;
+ } else {
+ dev_dbg(&sch_adapter.dev, "Successfull!\n");
+ }
+ }
+
+ /* start the transaction by setting bit 4 */
+ outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
+
+ do {
+ msleep(1);
+ temp = inb(SMBHSTSTS);
+ } 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 = -EPERM;
+ }
+
+ if (temp & 0x10) {
+ result = -EPERM;
+ dev_err(&sch_adapter.dev, "Error: Failed bus transaction\n");
+ }
+
+ if (temp & 0x08) {
+ result = -EPERM;
+ 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 */
+ }
+
+ if (temp & 0x04) {
+ result = -EPERM;
+ dev_dbg(&sch_adapter.dev, "Error: no response!\n");
+ }
+ temp = inb(SMBHSTSTS);
+ if (temp) {
+ if (temp == 0x1) {
+ dev_dbg(&sch_adapter.dev, "post complete!\n");
+ outb(temp, SMBHSTSTS);
+ } else if (temp & 0xe) {
+ dev_dbg(&sch_adapter.dev, "Error: bus, etc!\n");
+ outb(inb(SMBHSTSTS), SMBHSTSTS);
+ }
+ }
+ msleep(1);
+ temp = inb(SMBHSTSTS);
+ if (temp & 0xe) {
+ /* BSY, device or bus error */
+ dev_err(&sch_adapter.dev, "Failed reset at end of "
+ "transaction (%02x), Bus error\n", temp);
+ }
+ 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;
+ dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
+ (read_write)?"READ":"WRITE");
+ switch (size) {
+ case I2C_SMBUS_PROC_CALL:
+ dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
+ return -EPERM;
+ case I2C_SMBUS_QUICK:
+ outb(((addr & 0x7f) << 1) | (read_write & 0x01),
+ SMBHSTADD);
+ size = SCH_QUICK;
+ break;
+ case I2C_SMBUS_BYTE:
+ outb(((addr & 0x7f) << 1) | (read_write & 0x01),
+ SMBHSTADD);
+ if (read_write == I2C_SMBUS_WRITE)
+ outb(command, SMBHSTCMD);
+ size = SCH_BYTE;
+ break;
+ case I2C_SMBUS_BYTE_DATA:
+ outb(((addr & 0x7f) << 1) | (read_write & 0x01),
+ 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 & 0x7f) << 1) | (read_write & 0x01),
+ 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 & 0x7f) << 1) | (read_write & 0x01),
+ SMBHSTADD);
+ outb(command, SMBHSTCMD);
+ if (read_write == I2C_SMBUS_WRITE) {
+ len = data->block[0];
+ if (len < 0)
+ len = 0;
+ if (len > 32)
+ len = 32;
+ outb(len, SMBHSTDAT0);
+ i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
+ for (i = 1; i <= len; i++)
+ outb(data->block[i], SMBBLKDAT);
+ }
+ size = SCH_BLOCK_DATA;
+ break;
+ }
+ dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
+ outb((size & 0x7), SMBHSTCNT);
+
+ if (sch_transaction()) /* Error in transaction */
+ return -EPERM;
+
+ if ((read_write == I2C_SMBUS_WRITE) || (size == SCH_QUICK))
+ return 0;
+
+
+ switch (size) {
+ case SCH_BYTE:
+ /* Where is the result put? I assume here it is in
+ SMBHSTDAT0 but it might just as well be in the
+ SMBHSTCMD. No clue in the docs */
+ data->byte = inb(SMBHSTDAT0);
+ break;
+ 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);
+ i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
+ for (i = 1; i <= data->block[0]; i++)
+ data->block[i] = inb(SMBBLKDAT);
+ 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),
+ .driver_data = 0xf8 },
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, sch_ids);
+
+static int __devinit sch_probe(struct pci_dev *dev,
+ const struct pci_device_id *id)
+{
+ int retval;
+ retval = sch_setup(dev, id);
+ if (retval)
+ return retval;
+
+ /* set up the driverfs linkage to our parent device */
+ sch_adapter.dev.parent = &dev->dev;
+
+ snprintf(sch_adapter.name, I2C_NAME_SIZE,
+ "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 = "sch_smbus",
+ .id_table = sch_ids,
+ .probe = sch_probe,
+ .remove = __devexit_p(sch_remove),
+};
+
+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("Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and "
+ "Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org> and "
+ "Jacob Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ");
+MODULE_DESCRIPTION("Intel SCH SMBus driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_sch_init);
+module_exit(i2c_sch_exit);
--
1.5.2.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080424100510.09cc2d4b-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
@ 2008-04-25 11:49 ` Rudolf Marek
2008-04-25 21:00 ` Rudolf Marek
1 sibling, 0 replies; 15+ messages in thread
From: Rudolf Marek @ 2008-04-25 11:49 UTC (permalink / raw)
To: Alek Du; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
Hi Alek,
Is the documentation here?
http://www.intel.com/products/centrino/atom/techdocs.htm
It seems so. I will look into your driver soon.
Thanks,
Rudolf
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080424100510.09cc2d4b-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-04-25 11:49 ` Rudolf Marek
@ 2008-04-25 21:00 ` Rudolf Marek
[not found] ` <48124673.8020808-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
1 sibling, 1 reply; 15+ messages in thread
From: Rudolf Marek @ 2008-04-25 21:00 UTC (permalink / raw)
To: Alek Du; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hello Alek,
Please check the review bellow. It took me hour and half which was quite
unexpected when I started. Jean would you please take a look too?
Thanks,
Rudolf
> +/*
> + i2c-sch.c - Part of lm_sensors, Linux kernel modules for hardware
> + monitoring
> + - Based on i2c-piix4.c
> + Copyright (c) 1998 - 2002 Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and
> + Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
> + - Intel SCH support
> + Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> +
> + 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
> +
> + Function i2c_sch_init and i2c_sch_exit are module init/exit entries, and
> + sch_probe will be called for device initialization, In probe, SCH i2c
> + adapter will be setup by sch_setup and added by i2c_add_adapter. sch_access
> + is the main entry for the i2c-sch bus.
> + Note: we assume there can only be one device, with one SMBus interface.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pci.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/stddef.h>
> +#include <linux/sched.h>
> +#include <linux/ioport.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/apm_bios.h>
> +#include <linux/io.h>
> +
> +
> +struct sd {
> + const unsigned short mfr;
> + const unsigned short dev;
> + const unsigned char fn;
> + const char *name;
> +};
> +/* 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 8
No it is 64 bytes
> +
> +/* PCI Address Constants */
> +#define SMBBA_SCH 0x040
> +
> +/* Other settings */
> +#define MAX_TIMEOUT 500
> +#define ENABLE_INT9 0
> +
> +/* 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
Maybe your mailer did something to the tabs. You can run:
indent -kr -i8 i2ci-sch.c
It will fix all coding style issues. (I think except that it put labels
indented, you may revert that). We take here also text attachments, which are
fine through thunderbird. You seem to use clawmail so I cant help much.
> +
> +/* insmod parameters */
> +
> +/* If force is set to anything different from 0, we forcibly enable the
> + SCH. DANGEROUS! */
> +static int force;
> +module_param(force, int, 0);
> +MODULE_PARM_DESC(force, "Forcibly enable the I2C. DANGEROUS!");
Hmm not sure if it is good now. The newer BIOSes are much more saner than the
old - but here it is some left_over - check bellow.
> +
> +
> +static int sch_transaction(void);
> +
> +static unsigned short sch_smba;
> +static struct pci_driver sch_driver;
> +static struct i2c_adapter sch_adapter;
> +
> +/*
> + * sch_probe will call this function to get SMBus base address
> + * sch_dev and id are the arguments from probe functions
> + * return 0 for success and -ENODEV for failure
> + */
> +static int __devinit sch_setup(struct pci_dev *sch_dev,
> + const struct pci_device_id *id)
> +{
> + unsigned short smbase;
> + if (sch_dev->device != PCI_DEVICE_ID_INTEL_SCH_LPC) {
> + /* match up the function */
> + if (PCI_FUNC(sch_dev->devfn) != id->driver_data)
> + return -ENODEV;
> + dev_info(&sch_dev->dev, "Found %s device\n", pci_name(sch_dev));
> + } else {
> + dev_info(&sch_dev->dev, "Found SCH SMBUS %s device\n",
> + pci_name(sch_dev));
> + /* find SMBUS base address */
> + pci_read_config_word(sch_dev, 0x40, &smbase);
> + dev_info(&sch_dev->dev, "SCH SM base = 0x%04x\n", smbase);
> + }
> +
> +
> + /* Determine the address of the SMBus areas */
> + if (sch_dev->device == PCI_DEVICE_ID_INTEL_SCH_LPC)
> + pci_read_config_word(sch_dev, SMBBA_SCH, &sch_smba);
> + else
> + sch_smba = 0;
> + sch_smba &= 0xfff0;
> + if (sch_smba == 0) {
> + dev_err(&sch_dev->dev, "SMB base address "
> + "uninitialized - upgrade BIOS or use "
> + "force_addr=0xaddr\n");
> + return -ENODEV;
> + }
> +
> + if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> + dev_err(&sch_dev->dev, "SMB region 0x%x already in use!\n",
> + sch_smba);
> + return -ENODEV;
> + }
> +
Hmm its difficult for me to understand what are you trying to do with this
function? It seems you are trying to handle the case when the PCI_ID came from
user space? Also it seems force_addr parameter is gone.
Maybe we can do that like it is done now in i2c-viapro.c. But with the exception
that there seems to be no SMBUs enable bit except the bit32 in the BAR. To make
it compatible with some future versions where the BAR 0x40 wont be 0x40 please
use the driver_data to hold the register register value for the BAR and guard it
with:
/* driver_data might come from user-space, so check it */
if (id->driver_data & 3 || id->driver_data > 0xff)
return -EINVAL;
and then allow the force_addr like this:
/* Determine the address of the SMBus areas */
if (force_addr) {
sch_smba = force_addr & 0xffc0;
pci_write_config_dword(sch_dev, id->driver_data,
sch_smba | 0x80000000);
}
If force or force_addr was used I think we should put back registers after
sleep (resume). Maybe we should save/load the BAR0x40 because the BIOS might forgot?
pci_read_config_byte(sch_dev, id->driver_data, &tmp);
/* and check if user wants to just enable the controller without forcing any
address */
if (force) {
tmp |= 0x80;
pci_write_config_byte(sch_dev, id->driver_data, tmp);
}
if (!(tmp & 0x80))
dev_err(&sch_dev->dev, "SMBus controller not enabled */
return -ENODEV (or use goto style maybe better)
}
> + dev_dbg(&sch_dev->dev, "SMBA = 0x%X\n", sch_smba);
> +
> + return 0;
> +}
> +
> +/*
> + * 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);
> + if (temp) {
> + if (temp == 1) {
not sure if undocumented bit should be masked out
> + dev_dbg(&sch_adapter.dev, "Completion (%02x). "
> + "clear...\n", temp);
> + outb(temp, SMBHSTSTS);
> + } else if (temp & 0xe) {
> + dev_dbg(&sch_adapter.dev, "SMBus error (%02x). "
> + "Resetting...\n", temp);
You cant access the controller when the BUSY is set, nor you can try to clear
it. The error logic before transaction start must be fixed. check also the
access function for the BUSY bit.
> + outb(temp, SMBHSTSTS);
> + }
> + temp = inb(SMBHSTSTS);
> + if (temp) {
and here too (reserved regs usage)
> + dev_err(&sch_adapter.dev,
> + "SMBus is not ready: (%02x)\n", temp);
> + return -EPERM;
> + } else {
> + dev_dbg(&sch_adapter.dev, "Successfull!\n");
spelling mistake
> + }
> + }
> +
> + /* start the transaction by setting bit 4 */
> + outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
> +
> + do {
> + msleep(1);
> + temp = inb(SMBHSTSTS);
> + } 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 = -EPERM;
> + }
> +
> + if (temp & 0x10) {
> + result = -EPERM;
> + dev_err(&sch_adapter.dev, "Error: Failed bus transaction\n");
> + }
bit 0x10 is reserved. Maybe 0x4?
> +
> + if (temp & 0x08) {
> + result = -EPERM;
> + 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 */
> + }
I think if we are still busy we wont get here because we escape through the timeout.
> +
> + if (temp & 0x04) {
> + result = -EPERM;
> + dev_dbg(&sch_adapter.dev, "Error: no response!\n");
> + }
I think this should be 0x2 - device error.
> + temp = inb(SMBHSTSTS);
> + if (temp) {
> + if (temp == 0x1) {
> + dev_dbg(&sch_adapter.dev, "post complete!\n");
again here reserved registers are assumed to be zero.
> + outb(temp, SMBHSTSTS);
> + } else if (temp & 0xe) {
> + dev_dbg(&sch_adapter.dev, "Error: bus, etc!\n");
> + outb(inb(SMBHSTSTS), SMBHSTSTS);
> + }
> + }
> + msleep(1);
why?
> + temp = inb(SMBHSTSTS);
> + if (temp & 0xe) {
> + /* BSY, device or bus error */
> + dev_err(&sch_adapter.dev, "Failed reset at end of "
> + "transaction (%02x), Bus error\n", temp);
Maybe write that transaction did not complete. Also quite a lot device error
would be when probing the bus with i2cdetect command. The unclaimed device
cycles should be marked as debug so we dont waste log when probing for the
device. Also maybe we can mark it as real error only if the quick write was not
used? Jean, what do you think?
> + }
> + 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;
> + dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
> + (read_write)?"READ":"WRITE");
You _cant_ access the controller if busy is set. Maybe some check here? with
timeout?
> + switch (size) {
> + case I2C_SMBUS_PROC_CALL:
> + dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> + return -EPERM;
I think this should not happen.
> + case I2C_SMBUS_QUICK:
> + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> + SMBHSTADD);
> + size = SCH_QUICK;
> + break;
> + case I2C_SMBUS_BYTE:
> + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> + SMBHSTADD);
> + if (read_write == I2C_SMBUS_WRITE)
> + outb(command, SMBHSTCMD);
> + size = SCH_BYTE;
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> + 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 & 0x7f) << 1) | (read_write & 0x01),
> + 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 & 0x7f) << 1) | (read_write & 0x01),
> + SMBHSTADD);
> + outb(command, SMBHSTCMD);
> + if (read_write == I2C_SMBUS_WRITE) {
> + len = data->block[0];
> + if (len < 0)
> + len = 0;
> + if (len > 32)
> + len = 32;
> + outb(len, SMBHSTDAT0);
> + i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
Note that this is not documented in datasheet. And I think it works different
way. Not as FIFO but as registers.
> + for (i = 1; i <= len; i++)
> + outb(data->block[i], SMBBLKDAT);
No never would work. You did not tested that didn't you?
MAybe you can test the driver with following commands. Assuming that you have
SPD eeprom on 0x50.
modprobe i2c-dev
modprobe i2c-sch
i2cdump -l
will list the busses, assuming it is 0 please try following:
i2cdetect 0
It should detect all devices, maybe on 0x50 or 0x51 SPD eeprom. Use this device
for other tests:
i2cdump 0 0x50 b
i2cdump 0 0x50 c
i2cdump 0 0x50 W
i2cdump 0 0x50 s
This commands should produce same results.
i2cdump 0 0x50 w
You should see above in daisy chained mode. If the byte mode was:
00: 80 08 07 0d 0a 02 40 00 04 50 60 00 82 08 00 01 ??????@.?P`.??.?
Now you should see:
00: 0880 0708 0d07 0a0d 020a 4002 0040 0400
08: 5004 6050 0060 8200 0882 0008 0100 0e01 (e1 is from offset 0x10)
CHeck log aftewards if there is nothing unusual.
> + }
> + size = SCH_BLOCK_DATA;
> + break;
> + }
> + dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
> + outb((size & 0x7), SMBHSTCNT);
You are overwriting here one reserved bit.
> +
> + if (sch_transaction()) /* Error in transaction */
> + return -EPERM;
> +
> + if ((read_write == I2C_SMBUS_WRITE) || (size == SCH_QUICK))
> + return 0;
Well maybe the write quick success/error handling is here.
> +
> +
> + switch (size) {
> + case SCH_BYTE:
> + /* Where is the result put? I assume here it is in
> + SMBHSTDAT0 but it might just as well be in the
> + SMBHSTCMD. No clue in the docs */
Well check with my test ;)
> + data->byte = inb(SMBHSTDAT0);
> + break;
> + 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);
> + i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
> + for (i = 1; i <= data->block[0]; i++)
> + data->block[i] = inb(SMBBLKDAT);
> + break;
No way. It is different now. Fix it please.
> + }
> + 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),
> + .driver_data = 0xf8 },
Please set it to 0x40 as suggested above.
> + { 0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, sch_ids);
> +
> +static int __devinit sch_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + int retval;
> + retval = sch_setup(dev, id);
> + if (retval)
> + return retval;
> +
Maybe you can merge it with function above.
> + /* set up the driverfs linkage to our parent device */
> + sch_adapter.dev.parent = &dev->dev;
> +
> + snprintf(sch_adapter.name, I2C_NAME_SIZE,
> + "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 = "sch_smbus",
> + .id_table = sch_ids,
> + .probe = sch_probe,
> + .remove = __devexit_p(sch_remove),
.dynids.use_driver_data = 1,
and this.
> +};
> +
> +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("Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and "
> + "Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org> and "
> + "Jacob Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ");
> +MODULE_DESCRIPTION("Intel SCH SMBus driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_sch_init);
> +module_exit(i2c_sch_exit);
> --
> 1.5.2.5
> --
>
> _______________________________________________
> i2c mailing list
> i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIEkZh3J9wPJqZRNURAm7YAKC3xmN59rlFm2Qxxpk4U4HEkA6QqwCfURR8
7xpG07f3CBCY//eRd92o26Y=
=s0xF
-----END PGP SIGNATURE-----
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <48124673.8020808-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
@ 2008-04-26 7:50 ` Jean Delvare
[not found] ` <20080426095011.2d27b75b-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-04-28 6:45 ` Alek Du
2008-05-12 19:40 ` [PATCH] " Jean Delvare
2 siblings, 1 reply; 15+ messages in thread
From: Jean Delvare @ 2008-04-26 7:50 UTC (permalink / raw)
To: Rudolf Marek; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA, Alek Du
Hi Rudolf,
thanks for the review.
One preliminary note about the driver name. I'm a bit worried by the
name "i2c-sch" because there is another manufacturer (SMSC) making
chips named SCH-something which could include an SMBus controller. This
could be a bit confusing for the users. Maybe we could rename this
driver to i2c-isch, to make it clearer that it's for Intel SCH chips?
On Fri, 25 Apr 2008 23:00:35 +0200, Rudolf Marek wrote:
> Hello Alek,
>
> Please check the review bellow. It took me hour and half which was quite
> unexpected when I started. Jean would you please take a look too?
I'll review the next iteration. Some comments on what you wrote:
> > +/*
> > + i2c-sch.c - Part of lm_sensors, Linux kernel modules for hardware
> > + monitoring
These drivers are no longer "part of lm_sensors".
> > (...)
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/pci.h>
> > +#include <linux/kernel.h>
> > +#include <linux/delay.h>
> > +#include <linux/stddef.h>
> > +#include <linux/sched.h>
> > +#include <linux/ioport.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/apm_bios.h>
I'm very curious why you'd need this one? Presumably a leftover from
the i2c-piix4 driver...
> > +#include <linux/io.h>
> (...)
> Maybe your mailer did something to the tabs. You can run:
>
> indent -kr -i8 i2ci-sch.c
>
> It will fix all coding style issues. (I think except that it put labels
> indented, you may revert that). We take here also text attachments, which are
> fine through thunderbird. You seem to use clawmail so I cant help much.
There's scripts/Lindent which has all the options.
> > +
> > +/* insmod parameters */
> > +
> > +/* If force is set to anything different from 0, we forcibly enable the
> > + SCH. DANGEROUS! */
> > +static int force;
> > +module_param(force, int, 0);
> > +MODULE_PARM_DESC(force, "Forcibly enable the I2C. DANGEROUS!");
>
> Hmm not sure if it is good now. The newer BIOSes are much more saner than the
> old - but here it is some left_over - check bellow.
I agree with Rudolf here, we don't want any "force" parameter that
isn't strictly necessary. And even then we'd rather complain to the
manufacturers quickly, so that the BIOS can be fixed and the driver can
be kept clean.
> > (...)
> > + switch (size) {
> > + case I2C_SMBUS_PROC_CALL:
> > + dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> > + return -EPERM;
>
> I think this should not happen.
It could happen, but there's no need to handle unsupported cases
explicitly, just have a default case at the end which catches all the
unsupported sizes and returns -EINVAL.
> (...)
> MAybe you can test the driver with following commands. Assuming that you have
> SPD eeprom on 0x50.
>
> modprobe i2c-dev
> modprobe i2c-sch
>
> i2cdump -l
i2cdetect -l
> will list the busses, assuming it is 0 please try following:
>
> i2cdetect 0
>
> It should detect all devices, maybe on 0x50 or 0x51 SPD eeprom. Use this device
> for other tests:
>
> i2cdump 0 0x50 b
> i2cdump 0 0x50 c
> i2cdump 0 0x50 W
> i2cdump 0 0x50 s
>
> This commands should produce same results.
>
> i2cdump 0 0x50 w
>
> You should see above in daisy chained mode. If the byte mode was:
> 00: 80 08 07 0d 0a 02 40 00 04 50 60 00 82 08 00 01 ??????@.?P`.??.?
>
> Now you should see:
>
> 00: 0880 0708 0d07 0a0d 020a 4002 0040 0400
> 08: 5004 6050 0060 8200 0882 0008 0100 0e01 (e1 is from offset 0x10)
>
> CHeck log aftewards if there is nothing unusual.
This is very important to check that the contents of the EEPROMs have
not been modified by the dumps. If they are, it means that there's a
bug in the driver, and also your system probably won't boot next time
because the memory module won't be recognized. Better do you tests with
a cheap memory module ;)
> > (...)
> > + switch (size) {
> > + case SCH_BYTE:
> > + /* Where is the result put? I assume here it is in
> > + SMBHSTDAT0 but it might just as well be in the
> > + SMBHSTCMD. No clue in the docs */
>
> Well check with my test ;)
The comment above is taken directly from the i2c-piix4 driver, does it
apply to the SCH at all? We should probably remove it from the
i2c-piix4 driver anyway, we know the answer by now.
> > (...)
> > +static struct pci_device_id sch_ids[] = {
> > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC),
> > + .driver_data = 0xf8 },
>
> Please set it to 0x40 as suggested above.
Or just don't use driver_data at all for now? With a single device
supported at the moment, I don't see the idea.
> > (...)
> > +MODULE_AUTHOR("Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and "
> > + "Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org> and "
> > + "Jacob Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ");
You can remove Frodo and Philip here, they aren't the authors of _this_
driver.
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080426095011.2d27b75b-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2008-04-27 13:56 ` Alek Du
0 siblings, 0 replies; 15+ messages in thread
From: Alek Du @ 2008-04-27 13:56 UTC (permalink / raw)
To: Jean Delvare; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
Hi Rudolf, Jean
Thanks for your kind review. Will submit second round patch for review.
On Sat, 26 Apr 2008 09:50:11 +0200
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Rudolf,
>
> thanks for the review.
>
> One preliminary note about the driver name. I'm a bit worried by the
> name "i2c-sch" because there is another manufacturer (SMSC) making
> chips named SCH-something which could include an SMBus controller. This
> could be a bit confusing for the users. Maybe we could rename this
> driver to i2c-isch, to make it clearer that it's for Intel SCH chips?
>
> On Fri, 25 Apr 2008 23:00:35 +0200, Rudolf Marek wrote:
> > Hello Alek,
> >
> > Please check the review bellow. It took me hour and half which was quite
> > unexpected when I started. Jean would you please take a look too?
>
> I'll review the next iteration. Some comments on what you wrote:
>
> > > +/*
> > > + i2c-sch.c - Part of lm_sensors, Linux kernel modules for hardware
> > > + monitoring
>
> These drivers are no longer "part of lm_sensors".
>
> > > (...)
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/stddef.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/ioport.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/apm_bios.h>
>
> I'm very curious why you'd need this one? Presumably a leftover from
> the i2c-piix4 driver...
>
> > > +#include <linux/io.h>
>
> > (...)
> > Maybe your mailer did something to the tabs. You can run:
> >
> > indent -kr -i8 i2ci-sch.c
> >
> > It will fix all coding style issues. (I think except that it put labels
> > indented, you may revert that). We take here also text attachments, which are
> > fine through thunderbird. You seem to use clawmail so I cant help much.
>
> There's scripts/Lindent which has all the options.
>
> > > +
> > > +/* insmod parameters */
> > > +
> > > +/* If force is set to anything different from 0, we forcibly enable the
> > > + SCH. DANGEROUS! */
> > > +static int force;
> > > +module_param(force, int, 0);
> > > +MODULE_PARM_DESC(force, "Forcibly enable the I2C. DANGEROUS!");
> >
> > Hmm not sure if it is good now. The newer BIOSes are much more saner than the
> > old - but here it is some left_over - check bellow.
>
> I agree with Rudolf here, we don't want any "force" parameter that
> isn't strictly necessary. And even then we'd rather complain to the
> manufacturers quickly, so that the BIOS can be fixed and the driver can
> be kept clean.
>
> > > (...)
> > > + switch (size) {
> > > + case I2C_SMBUS_PROC_CALL:
> > > + dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> > > + return -EPERM;
> >
> > I think this should not happen.
>
> It could happen, but there's no need to handle unsupported cases
> explicitly, just have a default case at the end which catches all the
> unsupported sizes and returns -EINVAL.
>
> > (...)
> > MAybe you can test the driver with following commands. Assuming that you have
> > SPD eeprom on 0x50.
> >
> > modprobe i2c-dev
> > modprobe i2c-sch
> >
> > i2cdump -l
>
> i2cdetect -l
>
> > will list the busses, assuming it is 0 please try following:
> >
> > i2cdetect 0
> >
> > It should detect all devices, maybe on 0x50 or 0x51 SPD eeprom. Use this device
> > for other tests:
> >
> > i2cdump 0 0x50 b
> > i2cdump 0 0x50 c
> > i2cdump 0 0x50 W
> > i2cdump 0 0x50 s
> >
> > This commands should produce same results.
> >
> > i2cdump 0 0x50 w
> >
> > You should see above in daisy chained mode. If the byte mode was:
> > 00: 80 08 07 0d 0a 02 40 00 04 50 60 00 82 08 00 01 ??????@.?P`.??.?
> >
> > Now you should see:
> >
> > 00: 0880 0708 0d07 0a0d 020a 4002 0040 0400
> > 08: 5004 6050 0060 8200 0882 0008 0100 0e01 (e1 is from offset 0x10)
> >
> > CHeck log aftewards if there is nothing unusual.
>
> This is very important to check that the contents of the EEPROMs have
> not been modified by the dumps. If they are, it means that there's a
> bug in the driver, and also your system probably won't boot next time
> because the memory module won't be recognized. Better do you tests with
> a cheap memory module ;)
>
> > > (...)
> > > + switch (size) {
> > > + case SCH_BYTE:
> > > + /* Where is the result put? I assume here it is in
> > > + SMBHSTDAT0 but it might just as well be in the
> > > + SMBHSTCMD. No clue in the docs */
> >
> > Well check with my test ;)
>
> The comment above is taken directly from the i2c-piix4 driver, does it
> apply to the SCH at all? We should probably remove it from the
> i2c-piix4 driver anyway, we know the answer by now.
>
> > > (...)
> > > +static struct pci_device_id sch_ids[] = {
> > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC),
> > > + .driver_data = 0xf8 },
> >
> > Please set it to 0x40 as suggested above.
>
> Or just don't use driver_data at all for now? With a single device
> supported at the moment, I don't see the idea.
>
> > > (...)
> > > +MODULE_AUTHOR("Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and "
> > > + "Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org> and "
> > > + "Jacob Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ");
>
> You can remove Frodo and Philip here, they aren't the authors of _this_
> driver.
>
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <48124673.8020808-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
2008-04-26 7:50 ` Jean Delvare
@ 2008-04-28 6:45 ` Alek Du
[not found] ` <20080428144514.53aa54fd-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-05-12 19:40 ` [PATCH] " Jean Delvare
2 siblings, 1 reply; 15+ messages in thread
From: Alek Du @ 2008-04-28 6:45 UTC (permalink / raw)
To: Rudolf Marek; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
Rudolf,
Here are my i2cdetect and i2cdump results (with the patch I posted, I did not
change), please help to see if they are expected results -- for me, the
i2cdump 0 0x50 c/b/w work fine, but i2cdump 0 0x50 s crashes.
(T: m1)root@ume:/home/ume# i2cdetect -l
i2c-2 i2c intel drm LVDSDDC_C I2C adapter
i2c-1 i2c intel drm LVDSBLC_B I2C adapter
i2c-0 smbus SMBus POULSBO adapt SMBus adapter
(T: m1)root@ume:/home/ume# i2cdetect 0
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0.
I will probe address range 0x03-0x77.
Continue? [Y/n] y
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
30: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
40: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f
50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
60: 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f
70: 70 71 72 73 74 75 76 77
(T: m1)root@ume:/home/ume# i2cdump 0 0x50 b
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0, address 0x50, mode byte
Continue? [Y/n] y
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 80 08 08 0d 0a 61 40 00 05 3d 50 00 82 10 00 00 ?????a@.?=P.??..
10: 0c 08 18 01 04 00 01 50 50 00 00 3c 28 3c 2d 80 ?????.?PP..<(<-?
20: 25 37 10 22 3c 1e 1e 00 06 3c 7f 80 1e 28 00 00 %7?"<??.?<???(..
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 2a ..............?*
40: 2c 00 00 00 00 00 00 00 08 38 48 54 46 31 32 38 ,.......?8HTF128
50: 36 34 48 44 59 2d 35 33 45 45 31 01 00 07 38 df 64HDY-53EE1?.?8?
60: 11 b8 fd 00 00 00 00 00 00 00 00 00 00 00 00 00 ???.............
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
(T: m1)root@ume:/home/ume# i2cdump 0 0x50 c
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0, address 0x50, mode byte consecutive read
Continue? [Y/n] y
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 80 08 08 0d 0a 61 40 00 05 3d 50 00 82 10 00 00 ?????a@.?=P.??..
10: 0c 08 18 01 04 00 01 50 50 00 00 3c 28 3c 2d 80 ?????.?PP..<(<-?
20: 25 37 10 22 3c 1e 1e 00 06 3c 7f 80 1e 28 00 00 %7?"<??.?<???(..
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 2a ..............?*
40: 2c 00 00 00 00 00 00 00 08 38 48 54 46 31 32 38 ,.......?8HTF128
50: 36 34 48 44 59 2d 35 33 45 45 31 01 00 07 38 df 64HDY-53EE1?.?8?
60: 11 b8 fd 00 00 00 00 00 00 00 00 00 00 00 00 00 ???.............
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
(T: m1)root@ume:/home/ume# i2cdump 0 0x50 w
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0, address 0x50, mode word
Continue? [Y/n] y
0,8 1,9 2,a 3,b 4,c 5,d 6,e 7,f
00: 0880 0808 0d08 0a0d 610a 4061 0040 0500
08: 3d05 503d 0050 8200 1082 0010 0000 0c00
10: 080c 1808 0118 0401 0004 0100 5001 5050
18: 0050 0000 3c00 283c 3c28 2d3c 802d 2580
20: 3725 1037 2210 3c22 1e3c 1e1e 001e 0600
28: 3c06 7f3c 807f 1e80 281e 0028 0000 0000
30: 0000 0000 0000 0000 0000 0000 0000 0000
38: 0000 0000 0000 0000 0000 1200 2a12 2c2a
40: 002c 0000 0000 0000 0000 0000 0000 0800
48: 3808 4838 5448 4654 3146 3231 3832 3638
50: 3436 4834 4448 5944 2d59 352d 3335 4533
58: 4545 3145 0131 0001 0700 3807 df38 11df
60: b811 fdb8 00fd 0000 0000 0000 0000 0000
68: 0000 0000 0000 0000 0000 0000 0000 0000
70: 0000 0000 0000 0000 0000 0000 0000 0000
78: 0000 0000 0000 0000 0000 0000 0000 ff00
80: ffff ffff ffff ffff ffff ffff ffff ffff
88: ffff ffff ffff ffff ffff ffff ffff ffff
90: ffff ffff ffff ffff ffff ffff ffff ffff
98: ffff ffff ffff ffff ffff ffff ffff ffff
a0: ffff ffff ffff ffff ffff ffff ffff ffff
a8: ffff ffff ffff ffff ffff ffff ffff ffff
b0: ffff ffff ffff ffff ffff ffff ffff ffff
b8: ffff ffff ffff ffff ffff ffff ffff ffff
c0: ffff ffff ffff ffff ffff ffff ffff ffff
c8: ffff ffff ffff ffff ffff ffff ffff ffff
d0: ffff ffff ffff ffff ffff ffff ffff ffff
d8: ffff ffff ffff ffff ffff ffff ffff ffff
e0: ffff ffff ffff ffff ffff ffff ffff ffff
e8: ffff ffff ffff ffff ffff ffff ffff ffff
f0: ffff ffff ffff ffff ffff ffff ffff ffff
f8: ffff ffff ffff ffff ffff ffff ffff 80ff
(T: m1)root@ume:/home/ume# i2cdump 0 0x50 s
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0, address 0x50, mode smbus block
Continue? [Y/n] y
Segmentation fault
"i2cdump 0 0x50 s" crashes!!
Thanks,
Alek
On Sat, 26 Apr 2008 05:00:35 +0800
"Rudolf Marek" <r.marek-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org> wrote:
> Re: [i2c] [PATCH] i2c: Add Intel SCH I2C SMBus support
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hello Alek,
>
> Please check the review bellow. It took me hour and half which was quite
> unexpected when I started. Jean would you please take a look too?
>
> Thanks,
> Rudolf
>
> > +/*
> > + i2c-sch.c - Part of lm_sensors, Linux kernel modules for hardware
> > + monitoring
> > + - Based on i2c-piix4.c
> > + Copyright (c) 1998 - 2002 Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and
> > + Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
> > + - Intel SCH support
> > + Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > +
> > + 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
> > +
> > + Function i2c_sch_init and i2c_sch_exit are module init/exit entries, and
> > + sch_probe will be called for device initialization, In probe, SCH i2c
> > + adapter will be setup by sch_setup and added by i2c_add_adapter. sch_access
> > + is the main entry for the i2c-sch bus.
> > + Note: we assume there can only be one device, with one SMBus interface.
> > +*/
> > +
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/pci.h>
> > +#include <linux/kernel.h>
> > +#include <linux/delay.h>
> > +#include <linux/stddef.h>
> > +#include <linux/sched.h>
> > +#include <linux/ioport.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/apm_bios.h>
> > +#include <linux/io.h>
> > +
> > +
> > +struct sd {
> > + const unsigned short mfr;
> > + const unsigned short dev;
> > + const unsigned char fn;
> > + const char *name;
> > +};
> > +/* 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 8
>
> No it is 64 bytes
>
> > +
> > +/* PCI Address Constants */
> > +#define SMBBA_SCH 0x040
> > +
> > +/* Other settings */
> > +#define MAX_TIMEOUT 500
> > +#define ENABLE_INT9 0
> > +
> > +/* 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
>
> Maybe your mailer did something to the tabs. You can run:
>
> indent -kr -i8 i2ci-sch.c
>
> It will fix all coding style issues. (I think except that it put labels
> indented, you may revert that). We take here also text attachments, which are
> fine through thunderbird. You seem to use clawmail so I cant help much.
>
> > +
> > +/* insmod parameters */
> > +
> > +/* If force is set to anything different from 0, we forcibly enable the
> > + SCH. DANGEROUS! */
> > +static int force;
> > +module_param(force, int, 0);
> > +MODULE_PARM_DESC(force, "Forcibly enable the I2C. DANGEROUS!");
>
> Hmm not sure if it is good now. The newer BIOSes are much more saner than the
> old - but here it is some left_over - check bellow.
>
> > +
> > +
> > +static int sch_transaction(void);
> > +
> > +static unsigned short sch_smba;
> > +static struct pci_driver sch_driver;
> > +static struct i2c_adapter sch_adapter;
> > +
> > +/*
> > + * sch_probe will call this function to get SMBus base address
> > + * sch_dev and id are the arguments from probe functions
> > + * return 0 for success and -ENODEV for failure
> > + */
> > +static int __devinit sch_setup(struct pci_dev *sch_dev,
> > + const struct pci_device_id *id)
> > +{
> > + unsigned short smbase;
> > + if (sch_dev->device != PCI_DEVICE_ID_INTEL_SCH_LPC) {
> > + /* match up the function */
> > + if (PCI_FUNC(sch_dev->devfn) != id->driver_data)
> > + return -ENODEV;
> > + dev_info(&sch_dev->dev, "Found %s device\n", pci_name(sch_dev));
> > + } else {
> > + dev_info(&sch_dev->dev, "Found SCH SMBUS %s device\n",
> > + pci_name(sch_dev));
> > + /* find SMBUS base address */
> > + pci_read_config_word(sch_dev, 0x40, &smbase);
> > + dev_info(&sch_dev->dev, "SCH SM base = 0x%04x\n", smbase);
> > + }
> > +
> > +
> > + /* Determine the address of the SMBus areas */
> > + if (sch_dev->device == PCI_DEVICE_ID_INTEL_SCH_LPC)
> > + pci_read_config_word(sch_dev, SMBBA_SCH, &sch_smba);
> > + else
> > + sch_smba = 0;
> > + sch_smba &= 0xfff0;
> > + if (sch_smba == 0) {
> > + dev_err(&sch_dev->dev, "SMB base address "
> > + "uninitialized - upgrade BIOS or use "
> > + "force_addr=0xaddr\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> > + dev_err(&sch_dev->dev, "SMB region 0x%x already in use!\n",
> > + sch_smba);
> > + return -ENODEV;
> > + }
> > +
>
>
> Hmm its difficult for me to understand what are you trying to do with this
> function? It seems you are trying to handle the case when the PCI_ID came from
> user space? Also it seems force_addr parameter is gone.
>
> Maybe we can do that like it is done now in i2c-viapro.c. But with the exception
> that there seems to be no SMBUs enable bit except the bit32 in the BAR. To make
> it compatible with some future versions where the BAR 0x40 wont be 0x40 please
> use the driver_data to hold the register register value for the BAR and guard it
> with:
>
> /* driver_data might come from user-space, so check it */
> if (id->driver_data & 3 || id->driver_data > 0xff)
> return -EINVAL;
>
> and then allow the force_addr like this:
>
> /* Determine the address of the SMBus areas */
> if (force_addr) {
> sch_smba = force_addr & 0xffc0;
> pci_write_config_dword(sch_dev, id->driver_data,
> sch_smba | 0x80000000);
>
> }
>
>
> If force or force_addr was used I think we should put back registers after
> sleep (resume). Maybe we should save/load the BAR0x40 because the BIOS might forgot?
>
> pci_read_config_byte(sch_dev, id->driver_data, &tmp);
>
> /* and check if user wants to just enable the controller without forcing any
> address */
> if (force) {
> tmp |= 0x80;
> pci_write_config_byte(sch_dev, id->driver_data, tmp);
> }
>
> if (!(tmp & 0x80))
> dev_err(&sch_dev->dev, "SMBus controller not enabled */
> return -ENODEV (or use goto style maybe better)
> }
>
> > + dev_dbg(&sch_dev->dev, "SMBA = 0x%X\n", sch_smba);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * 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);
> > + if (temp) {
> > + if (temp == 1) {
>
> not sure if undocumented bit should be masked out
>
> > + dev_dbg(&sch_adapter.dev, "Completion (%02x). "
> > + "clear...\n", temp);
> > + outb(temp, SMBHSTSTS);
> > + } else if (temp & 0xe) {
> > + dev_dbg(&sch_adapter.dev, "SMBus error (%02x). "
> > + "Resetting...\n", temp);
>
> You cant access the controller when the BUSY is set, nor you can try to clear
> it. The error logic before transaction start must be fixed. check also the
> access function for the BUSY bit.
>
> > + outb(temp, SMBHSTSTS);
> > + }
> > + temp = inb(SMBHSTSTS);
> > + if (temp) {
>
> and here too (reserved regs usage)
>
> > + dev_err(&sch_adapter.dev,
> > + "SMBus is not ready: (%02x)\n", temp);
> > + return -EPERM;
> > + } else {
> > + dev_dbg(&sch_adapter.dev, "Successfull!\n");
>
> spelling mistake
>
> > + }
> > + }
> > +
> > + /* start the transaction by setting bit 4 */
> > + outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
> > +
> > + do {
> > + msleep(1);
> > + temp = inb(SMBHSTSTS);
> > + } 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 = -EPERM;
> > + }
> > +
> > + if (temp & 0x10) {
> > + result = -EPERM;
> > + dev_err(&sch_adapter.dev, "Error: Failed bus transaction\n");
> > + }
>
> bit 0x10 is reserved. Maybe 0x4?
>
> > +
> > + if (temp & 0x08) {
> > + result = -EPERM;
> > + 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 */
> > + }
>
> I think if we are still busy we wont get here because we escape through the timeout.
>
> > +
> > + if (temp & 0x04) {
> > + result = -EPERM;
> > + dev_dbg(&sch_adapter.dev, "Error: no response!\n");
> > + }
>
> I think this should be 0x2 - device error.
>
> > + temp = inb(SMBHSTSTS);
> > + if (temp) {
> > + if (temp == 0x1) {
> > + dev_dbg(&sch_adapter.dev, "post complete!\n");
>
> again here reserved registers are assumed to be zero.
>
> > + outb(temp, SMBHSTSTS);
> > + } else if (temp & 0xe) {
> > + dev_dbg(&sch_adapter.dev, "Error: bus, etc!\n");
> > + outb(inb(SMBHSTSTS), SMBHSTSTS);
> > + }
> > + }
> > + msleep(1);
>
> why?
>
> > + temp = inb(SMBHSTSTS);
> > + if (temp & 0xe) {
> > + /* BSY, device or bus error */
> > + dev_err(&sch_adapter.dev, "Failed reset at end of "
> > + "transaction (%02x), Bus error\n", temp);
>
> Maybe write that transaction did not complete. Also quite a lot device error
> would be when probing the bus with i2cdetect command. The unclaimed device
> cycles should be marked as debug so we dont waste log when probing for the
> device. Also maybe we can mark it as real error only if the quick write was not
> used? Jean, what do you think?
>
> > + }
> > + 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;
> > + dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
> > + (read_write)?"READ":"WRITE");
>
> You _cant_ access the controller if busy is set. Maybe some check here? with
> timeout?
>
> > + switch (size) {
> > + case I2C_SMBUS_PROC_CALL:
> > + dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> > + return -EPERM;
>
> I think this should not happen.
>
> > + case I2C_SMBUS_QUICK:
> > + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> > + SMBHSTADD);
> > + size = SCH_QUICK;
> > + break;
> > + case I2C_SMBUS_BYTE:
> > + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> > + SMBHSTADD);
> > + if (read_write == I2C_SMBUS_WRITE)
> > + outb(command, SMBHSTCMD);
> > + size = SCH_BYTE;
> > + break;
> > + case I2C_SMBUS_BYTE_DATA:
> > + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> > + 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 & 0x7f) << 1) | (read_write & 0x01),
> > + 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 & 0x7f) << 1) | (read_write & 0x01),
> > + SMBHSTADD);
> > + outb(command, SMBHSTCMD);
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + len = data->block[0];
> > + if (len < 0)
> > + len = 0;
> > + if (len > 32)
> > + len = 32;
> > + outb(len, SMBHSTDAT0);
> > + i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
>
> Note that this is not documented in datasheet. And I think it works different
> way. Not as FIFO but as registers.
>
> > + for (i = 1; i <= len; i++)
> > + outb(data->block[i], SMBBLKDAT);
>
> No never would work. You did not tested that didn't you?
>
> MAybe you can test the driver with following commands. Assuming that you have
> SPD eeprom on 0x50.
>
> modprobe i2c-dev
> modprobe i2c-sch
>
> i2cdump -l
> will list the busses, assuming it is 0 please try following:
>
> i2cdetect 0
>
> It should detect all devices, maybe on 0x50 or 0x51 SPD eeprom. Use this device
> for other tests:
>
> i2cdump 0 0x50 b
> i2cdump 0 0x50 c
> i2cdump 0 0x50 W
> i2cdump 0 0x50 s
>
> This commands should produce same results.
>
> i2cdump 0 0x50 w
>
> You should see above in daisy chained mode. If the byte mode was:
> 00: 80 08 07 0d 0a 02 40 00 04 50 60 00 82 08 00 01 ??????@.?P`.??.?
>
> Now you should see:
>
> 00: 0880 0708 0d07 0a0d 020a 4002 0040 0400
> 08: 5004 6050 0060 8200 0882 0008 0100 0e01 (e1 is from offset 0x10)
>
> CHeck log aftewards if there is nothing unusual.
>
>
> > + }
> > + size = SCH_BLOCK_DATA;
> > + break;
> > + }
> > + dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
> > + outb((size & 0x7), SMBHSTCNT);
>
> You are overwriting here one reserved bit.
>
> > +
> > + if (sch_transaction()) /* Error in transaction */
> > + return -EPERM;
> > +
> > + if ((read_write == I2C_SMBUS_WRITE) || (size == SCH_QUICK))
> > + return 0;
>
> Well maybe the write quick success/error handling is here.
>
> > +
> > +
> > + switch (size) {
> > + case SCH_BYTE:
> > + /* Where is the result put? I assume here it is in
> > + SMBHSTDAT0 but it might just as well be in the
> > + SMBHSTCMD. No clue in the docs */
>
> Well check with my test ;)
>
> > + data->byte = inb(SMBHSTDAT0);
> > + break;
> > + 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);
> > + i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
> > + for (i = 1; i <= data->block[0]; i++)
> > + data->block[i] = inb(SMBBLKDAT);
> > + break;
>
> No way. It is different now. Fix it please.
>
> > + }
> > + 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),
> > + .driver_data = 0xf8 },
>
> Please set it to 0x40 as suggested above.
>
> > + { 0, }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, sch_ids);
> > +
> > +static int __devinit sch_probe(struct pci_dev *dev,
> > + const struct pci_device_id *id)
> > +{
> > + int retval;
> > + retval = sch_setup(dev, id);
> > + if (retval)
> > + return retval;
> > +
>
> Maybe you can merge it with function above.
>
> > + /* set up the driverfs linkage to our parent device */
> > + sch_adapter.dev.parent = &dev->dev;
> > +
> > + snprintf(sch_adapter.name, I2C_NAME_SIZE,
> > + "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 = "sch_smbus",
> > + .id_table = sch_ids,
> > + .probe = sch_probe,
> > + .remove = __devexit_p(sch_remove),
>
> .dynids.use_driver_data = 1,
>
> and this.
>
>
> > +};
> > +
> > +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("Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and "
> > + "Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org> and "
> > + "Jacob Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ");
> > +MODULE_DESCRIPTION("Intel SCH SMBus driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(i2c_sch_init);
> > +module_exit(i2c_sch_exit);
> > --
> > 1.5.2.5
> > --
> >
> > _______________________________________________
> > i2c mailing list
> > i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
> > http://lists.lm-sensors.org/mailman/listinfo/i2c
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFIEkZh3J9wPJqZRNURAm7YAKC3xmN59rlFm2Qxxpk4U4HEkA6QqwCfURR8
> 7xpG07f3CBCY//eRd92o26Y=
> =s0xF
> -----END PGP SIGNATURE-----
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080428144514.53aa54fd-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
@ 2008-04-28 8:57 ` Jean Delvare
[not found] ` <20080428105738.06429ed2-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Jean Delvare @ 2008-04-28 8:57 UTC (permalink / raw)
To: Alek Du; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
Hi Alek,
On Mon, 28 Apr 2008 14:45:14 +0800, Alek Du wrote:
> Rudolf,
>
> Here are my i2cdetect and i2cdump results (with the patch I posted, I did not
> change), please help to see if they are expected results -- for me, the
> i2cdump 0 0x50 c/b/w work fine, but i2cdump 0 0x50 s crashes.
>
> (T: m1)root@ume:/home/ume# i2cdetect -l
> i2c-2 i2c intel drm LVDSDDC_C I2C adapter
> i2c-1 i2c intel drm LVDSBLC_B I2C adapter
> i2c-0 smbus SMBus POULSBO adapt SMBus adapter
>
> (T: m1)root@ume:/home/ume# i2cdetect 0
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-0.
> I will probe address range 0x03-0x77.
> Continue? [Y/n] y
> 0 1 2 3 4 5 6 7 8 9 a b c d e f
> 00: 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
> 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> 30: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
> 40: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f
> 50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
> 60: 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f
> 70: 70 71 72 73 74 75 76 77
This is not correct. The driver should report success only when a
device is found at a given address, if not it should return -ENODEV.
>From the output above it appears that your driver always report success.
>
> (T: m1)root@ume:/home/ume# i2cdump 0 0x50 b
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-0, address 0x50, mode byte
> Continue? [Y/n] y
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> 00: 80 08 08 0d 0a 61 40 00 05 3d 50 00 82 10 00 00 ?????a@.?=P.??..
> 10: 0c 08 18 01 04 00 01 50 50 00 00 3c 28 3c 2d 80 ?????.?PP..<(<-?
> 20: 25 37 10 22 3c 1e 1e 00 06 3c 7f 80 1e 28 00 00 %7?"<??.?<???(..
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 2a ..............?*
> 40: 2c 00 00 00 00 00 00 00 08 38 48 54 46 31 32 38 ,.......?8HTF128
> 50: 36 34 48 44 59 2d 35 33 45 45 31 01 00 07 38 df 64HDY-53EE1?.?8?
> 60: 11 b8 fd 00 00 00 00 00 00 00 00 00 00 00 00 00 ???.............
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
>
> (T: m1)root@ume:/home/ume# i2cdump 0 0x50 c
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-0, address 0x50, mode byte consecutive read
> Continue? [Y/n] y
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> 00: 80 08 08 0d 0a 61 40 00 05 3d 50 00 82 10 00 00 ?????a@.?=P.??..
> 10: 0c 08 18 01 04 00 01 50 50 00 00 3c 28 3c 2d 80 ?????.?PP..<(<-?
> 20: 25 37 10 22 3c 1e 1e 00 06 3c 7f 80 1e 28 00 00 %7?"<??.?<???(..
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 2a ..............?*
> 40: 2c 00 00 00 00 00 00 00 08 38 48 54 46 31 32 38 ,.......?8HTF128
> 50: 36 34 48 44 59 2d 35 33 45 45 31 01 00 07 38 df 64HDY-53EE1?.?8?
> 60: 11 b8 fd 00 00 00 00 00 00 00 00 00 00 00 00 00 ???.............
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
>
> (T: m1)root@ume:/home/ume# i2cdump 0 0x50 w
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-0, address 0x50, mode word
> Continue? [Y/n] y
> 0,8 1,9 2,a 3,b 4,c 5,d 6,e 7,f
> 00: 0880 0808 0d08 0a0d 610a 4061 0040 0500
> 08: 3d05 503d 0050 8200 1082 0010 0000 0c00
> 10: 080c 1808 0118 0401 0004 0100 5001 5050
> 18: 0050 0000 3c00 283c 3c28 2d3c 802d 2580
> 20: 3725 1037 2210 3c22 1e3c 1e1e 001e 0600
> 28: 3c06 7f3c 807f 1e80 281e 0028 0000 0000
> 30: 0000 0000 0000 0000 0000 0000 0000 0000
> 38: 0000 0000 0000 0000 0000 1200 2a12 2c2a
> 40: 002c 0000 0000 0000 0000 0000 0000 0800
> 48: 3808 4838 5448 4654 3146 3231 3832 3638
> 50: 3436 4834 4448 5944 2d59 352d 3335 4533
> 58: 4545 3145 0131 0001 0700 3807 df38 11df
> 60: b811 fdb8 00fd 0000 0000 0000 0000 0000
> 68: 0000 0000 0000 0000 0000 0000 0000 0000
> 70: 0000 0000 0000 0000 0000 0000 0000 0000
> 78: 0000 0000 0000 0000 0000 0000 0000 ff00
> 80: ffff ffff ffff ffff ffff ffff ffff ffff
> 88: ffff ffff ffff ffff ffff ffff ffff ffff
> 90: ffff ffff ffff ffff ffff ffff ffff ffff
> 98: ffff ffff ffff ffff ffff ffff ffff ffff
> a0: ffff ffff ffff ffff ffff ffff ffff ffff
> a8: ffff ffff ffff ffff ffff ffff ffff ffff
> b0: ffff ffff ffff ffff ffff ffff ffff ffff
> b8: ffff ffff ffff ffff ffff ffff ffff ffff
> c0: ffff ffff ffff ffff ffff ffff ffff ffff
> c8: ffff ffff ffff ffff ffff ffff ffff ffff
> d0: ffff ffff ffff ffff ffff ffff ffff ffff
> d8: ffff ffff ffff ffff ffff ffff ffff ffff
> e0: ffff ffff ffff ffff ffff ffff ffff ffff
> e8: ffff ffff ffff ffff ffff ffff ffff ffff
> f0: ffff ffff ffff ffff ffff ffff ffff ffff
> f8: ffff ffff ffff ffff ffff ffff ffff 80ff
These look OK.
>
> (T: m1)root@ume:/home/ume# i2cdump 0 0x50 s
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-0, address 0x50, mode smbus block
> Continue? [Y/n] y
> Segmentation fault
>
>
> "i2cdump 0 0x50 s" crashes!!
I guess there's a stack trace in the kernel log?
Presumably your implementation of the SMBus block read transaction is
broken, you have to check it.
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] i2c: Add Intel SCH I2C SMBus support (revised)
[not found] ` <20080428105738.06429ed2-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2008-04-29 3:06 ` Alek Du
[not found] ` <8AD95083DC3E36478732061D97524415030F8FAA@pdsmsx411.ccr.corp.intel.com>
0 siblings, 1 reply; 15+ messages in thread
From: Alek Du @ 2008-04-29 3:06 UTC (permalink / raw)
To: Jean Delvare; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
Hi Jean, Rudolf,
I modified and tested the SCH I2C driver according to your kind comments. Please help to review again:
This patch adds Intel SCH chipsets (US15W, US15L, UL11L) i2c bus support.
Signed-off-by: Alek Du <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/busses/Kconfig | 7 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-isch.c | 362 +++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 2 ++
4 files changed, 372 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..39d86f7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -247,6 +247,13 @@ config I2C_PIIX4
This driver can also be built as a module. If so, the module
will be called i2c-piix4.
+config I2C_SCH
+ tristate "Intel SCH SMBUS 1.0"
+ depends on I2C && PCI
+ help
+ If you say Y or M to this option, support will be included for the
+ Intel SCH based systems. Module will be called i2c-sch.ko.
+
config I2C_IBM_IIC
tristate "IBM PPC 4xx on-chip I2C interface"
depends on 4xx
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e8c882a..9a9ec31 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
obj-$(CONFIG_I2C_PCA_PLATFORM) += i2c-pca-platform.o
obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o
obj-$(CONFIG_I2C_PMCMSP) += i2c-pmcmsp.o
+obj-$(CONFIG_I2C_SCH) += i2c-isch.o
obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
obj-$(CONFIG_I2C_PROSAVAGE) += i2c-prosavage.o
obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
new file mode 100644
index 0000000..4535c85
--- /dev/null
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -0,0 +1,362 @@
+/*
+ i2c-sch.c - Linux kernel driver for Intel SCH chipset SMBus
+ - Based on i2c-piix4.c
+ Copyright (c) 1998 - 2002 Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and
+ Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
+ - Intel SCH support
+ Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+
+ 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
+ Note: we assume there can only be one device, with one SMBus interface.
+*/
+
+#include <linux/module.h>
+#include <linux/moduleparam.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>
+
+
+struct sd {
+ const unsigned short mfr;
+ const unsigned short dev;
+ const unsigned char fn;
+ const char *name;
+};
+/* 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 0x040
+
+/* Other settings */
+#define MAX_TIMEOUT 500
+#define ENABLE_INT9 0
+
+/* 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 int sch_transaction(void);
+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 & 0x08) {
+ dev_dbg(&sch_adapter.dev, "SMBus busy (%02x)\n", temp);
+ return -EPERM;
+ } else if (temp & 0x01) {
+ dev_dbg(&sch_adapter.dev, "Completion (%02x). "
+ "clear...\n", temp);
+ outb(temp, SMBHSTSTS);
+ } else if (temp & 0x06) {
+ 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;
+ } else {
+ dev_dbg(&sch_adapter.dev, "Successful!\n");
+ }
+
+ /* 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 = -EPERM;
+ }
+ if (temp & 0x04) {
+ result = -EPERM;
+ 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 = -EPERM;
+ 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 = -EPERM;
+ dev_dbg(&sch_adapter.dev, "Transaction failed.\n");
+ }
+ 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;
+
+ /* 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 -EPERM;
+ }
+ dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
+ (read_write)?"READ":"WRITE");
+ switch (size) {
+ case I2C_SMBUS_QUICK:
+ outb(((addr & 0x7f) << 1) | (read_write & 0x01),
+ SMBHSTADD);
+ size = SCH_QUICK;
+ break;
+ case I2C_SMBUS_BYTE:
+ outb(((addr & 0x7f) << 1) | (read_write & 0x01),
+ SMBHSTADD);
+ if (read_write == I2C_SMBUS_WRITE)
+ outb(command, SMBHSTCMD);
+ size = SCH_BYTE;
+ break;
+ case I2C_SMBUS_BYTE_DATA:
+ outb(((addr & 0x7f) << 1) | (read_write & 0x01),
+ 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 & 0x7f) << 1) | (read_write & 0x01),
+ 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 & 0x7f) << 1) | (read_write & 0x01),
+ SMBHSTADD);
+ outb(command, SMBHSTCMD);
+ if (read_write == I2C_SMBUS_WRITE) {
+ len = data->block[0];
+ if (len < 0)
+ len = 0;
+ if (len > 32)
+ len = 32;
+ outb(len, SMBHSTDAT0);
+ i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
+ 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");
+ return -EPERM;
+ }
+ dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
+ outb((size & 0x7), SMBHSTCNT);
+
+ if (sch_transaction()) /* Error in transaction */
+ return -EPERM;
+
+ if ((read_write == I2C_SMBUS_WRITE) || (size == SCH_QUICK))
+ return 0;
+
+ switch (size) {
+ case SCH_BYTE:
+ /* Where is the result put? I assume here it is in
+ SMBHSTDAT0 but it might just as well be in the
+ SMBHSTCMD. No clue in the docs */
+ data->byte = inb(SMBHSTDAT0);
+ break;
+ 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);
+ i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
+ data->block[0] = I2C_SMBUS_BLOCK_MAX;
+ 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),
+ .driver_data = 0x40 },
+ { 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;
+
+ pci_read_config_word(dev, SMBBA_SCH, &sch_smba);
+ sch_smba &= 0xfff0;
+ if (sch_smba == 0) {
+ dev_err(&dev->dev, "SMB base address uninitialized\n");
+ return -ENODEV;
+ }
+ if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
+ dev_err(&dev->dev, "SMB region 0x%x already in use!\n",
+ sch_smba);
+ return -ENODEV;
+ }
+ dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
+
+ /* set up the driverfs linkage to our parent device */
+ sch_adapter.dev.parent = &dev->dev;
+
+ snprintf(sch_adapter.name, I2C_NAME_SIZE,
+ "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 = "sch_smbus",
+ .id_table = sch_ids,
+ .probe = sch_probe,
+ .remove = __devexit_p(sch_remove),
+ .dynids.use_driver_data = 1,
+};
+
+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 <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Intel SCH SMBus driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_sch_init);
+module_exit(i2c_sch_exit);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 70eb3c8..e5a53da 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2413,6 +2413,8 @@
#define PCI_DEVICE_ID_INTEL_82443GX_0 0x71a0
#define PCI_DEVICE_ID_INTEL_82443GX_2 0x71a2
#define PCI_DEVICE_ID_INTEL_82372FB_1 0x7601
+#define PCI_DEVICE_ID_INTEL_SCH_LPC 0x8119
+#define PCI_DEVICE_ID_INTEL_SCH_IDE 0x811a
#define PCI_DEVICE_ID_INTEL_82454GX 0x84c4
#define PCI_DEVICE_ID_INTEL_82450GX 0x84c5
#define PCI_DEVICE_ID_INTEL_82451NX 0x84ca
--
1.5.2.5
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support
[not found] ` <48124673.8020808-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
2008-04-26 7:50 ` Jean Delvare
2008-04-28 6:45 ` Alek Du
@ 2008-05-12 19:40 ` Jean Delvare
2 siblings, 0 replies; 15+ messages in thread
From: Jean Delvare @ 2008-05-12 19:40 UTC (permalink / raw)
To: Alek Du; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
On Fri, 25 Apr 2008 23:00:35 +0200, Rudolf Marek wrote:
> > +/* 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 8
>
> No it is 64 bytes
The datasheet has contradictions. It says 32 bytes in section 5.4.2,
table 13. But it says 64 bytes in section 17.3.1. Given that the block
registers span from offset 32 to offset 63, the latter is obviously
correct.
Alek, can you please have table 13 fixed in the datasheet?
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: Add Intel SCH I2C SMBus support (revised)
[not found] ` <20080506095036.274e91f1-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
@ 2008-05-12 20:57 ` Jean Delvare
[not found] ` <20080512225718.7440ab42-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Jean Delvare @ 2008-05-12 20:57 UTC (permalink / raw)
To: Alek Du; +Cc: Linux I2C
Hi Alek,
Sorry for the late reply.
On Tue, 6 May 2008 09:50:36 +0800, Alek Du wrote:
> Since another patch SCH PCI IDs has been accepted by 2.6.26, I
> updated this patch to not include the pci_ids.h. Please review it:
Here you go:
> From e0526346fa96acfc16e558f6f47865f0a15ad79e Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Wed, 30 Apr 2008 11:01:17 +0800
> Subject: [PATCH] i2c: Add Intel SCH I2C SMBus Support
>
> This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) i2c bus support.
>
> Signed-off-by: Alek Du <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> drivers/i2c/busses/Kconfig | 7 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-isch.c | 362 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 370 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..39d86f7 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -247,6 +247,13 @@ config I2C_PIIX4
> This driver can also be built as a module. If so, the module
> will be called i2c-piix4.
>
> +config I2C_SCH
I'd rather name the option I2C_ISCH to match the new driver name.
> + tristate "Intel SCH SMBUS 1.0"
The usual spelling is "SMBus".
> + depends on I2C && PCI
The I2C dependency is handled at menu level so you do not have to
include it here.
> + help
> + If you say Y or M to this option, support will be included for the
This specific wording seems to come from the old 2.4 times. These days
we write instead "If you say yes to this option, support etc."
> + Intel SCH based systems. Module will be called i2c-sch.ko.
And we no longer include the extension when giving a module name.
Module name which is now i2c-isch, BTW.
> +
> config I2C_IBM_IIC
> tristate "IBM PPC 4xx on-chip I2C interface"
> depends on 4xx
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index e8c882a..9a9ec31 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
> obj-$(CONFIG_I2C_PCA_PLATFORM) += i2c-pca-platform.o
> obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o
> obj-$(CONFIG_I2C_PMCMSP) += i2c-pmcmsp.o
> +obj-$(CONFIG_I2C_SCH) += i2c-isch.o
In alphabetical order please.
> obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
> obj-$(CONFIG_I2C_PROSAVAGE) += i2c-prosavage.o
> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> new file mode 100644
> index 0000000..224e4aa
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -0,0 +1,362 @@
> +/*
> + i2c-isch.c - Linux kernel driver for Intel SCH chipset SMBus
> + - Based on i2c-piix4.c
> + Copyright (c) 1998 - 2002 Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and
> + Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
> + - Intel SCH support
> + Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> +
> + 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
> + Note: we assume there can only be one device, with one SMBus interface.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
Your driver no longer uses module parameters, so you don't need to
include this one.
> +#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>
> +
> +
> +struct sd {
> + const unsigned short mfr;
> + const unsigned short dev;
> + const unsigned char fn;
> + const char *name;
> +};
What's the point of defining a structure which you never use?
> +/* 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 0x040
> +
> +/* Other settings */
> +#define MAX_TIMEOUT 500
> +#define ENABLE_INT9 0
Not used anywhere, please delete.
> +
> +/* 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 int sch_transaction(void);
This forward declaration isn't needed.
> +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 & 0x08) {
> + dev_dbg(&sch_adapter.dev, "SMBus busy (%02x)\n", temp);
> + return -EPERM;
-EAGAIN would be more appropriate I think. If the SMBus is busy now,
the user might want to try again later. But you're already testing for
this condition in sch_access(), so why do you need to test it again
here?
> + } else if (temp & 0x01) {
> + dev_dbg(&sch_adapter.dev, "Completion (%02x). "
> + "clear...\n", temp);
> + outb(temp, SMBHSTSTS);
> + } else if (temp & 0x06) {
> + dev_dbg(&sch_adapter.dev, "SMBus error (%02x). "
> + "Resetting...\n", temp);
> + outb(temp, SMBHSTSTS);
> + }
> + temp = inb(SMBHSTSTS) & 0x0f;
Doubled space.
If the first inb() didn't report any error, there's no point in doing a
second one, is there?
> + if (temp) {
> + dev_err(&sch_adapter.dev,
> + "SMBus is not ready: (%02x)\n", temp);
> + return -EPERM;
> + } else {
> + dev_dbg(&sch_adapter.dev, "Successful!\n");
This will print a debug message on _every_ transaction. I think that
you should rework this part of the code to only read SMBHSTSTS once if
there is no error, and only log "Successful!" if you had to reset the
controller, and succeeded.
> + }
> +
> + /* 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 = -EPERM;
-ETIMEDOUT
> + }
> + if (temp & 0x04) {
> + result = -EPERM;
-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 = -EPERM;
Please return -ENXIO in this case (we're in the process of updating all
the other drivers to do so.)
> + 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*/
Missing space at end of comment.
> + dev_dbg(&sch_adapter.dev, "Failed reset at end of "
> + "transaction (%02x), Bus error\n", temp);
> + }
> + } else {
> + result = -EPERM;
-EIO
> + dev_dbg(&sch_adapter.dev, "Transaction failed.\n");
Shouldn't this be a dev_err()? It seems serious enough to let the user
know.
> + }
> + 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.
Doubled space.
> + */
> +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;
> +
> + /* Make sure the SMBus host is not busy*/
Missing space at end of comment.
> + temp = inb(SMBHSTSTS) & 0x0f;
> + if (temp & 0x08) {
> + dev_dbg(&sch_adapter.dev, "SMBus busy (%02x)\n", temp);
> + return -EPERM;
> + }
That would be -EAGAIN.
> + dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
> + (read_write)?"READ":"WRITE");
> + switch (size) {
> + case I2C_SMBUS_QUICK:
> + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
read_write is 0 or 1, so masking it is pointless. Likewise, addr is a
7-bit value, so masking it with 0x7f is a no-op. Same for all cases
below.
> + SMBHSTADD);
> + size = SCH_QUICK;
> + break;
> + case I2C_SMBUS_BYTE:
> + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> + SMBHSTADD);
> + if (read_write == I2C_SMBUS_WRITE)
> + outb(command, SMBHSTCMD);
> + size = SCH_BYTE;
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + outb(((addr & 0x7f) << 1) | (read_write & 0x01),
> + 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 & 0x7f) << 1) | (read_write & 0x01),
> + 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 & 0x7f) << 1) | (read_write & 0x01),
> + SMBHSTADD);
> + outb(command, SMBHSTCMD);
> + if (read_write == I2C_SMBUS_WRITE) {
> + len = data->block[0];
> + if (len < 0)
This will never be true, as data->block[0] is as u8.
> + len = 0;
A block length of 0 is not valid according to the SMBus specification.
> + if (len > 32)
> + len = 32;
Please use I2C_SMBUS_BLOCK_MAX instead of hard-coded 32.
And anyway, please do not silently force the length value to be valid.
If the request is invalid, just reject it with -EINVAL.
> + outb(len, SMBHSTDAT0);
> + i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
I doubt that this is really needed. The PIIX4 needed it because
SMBBLKDAT was a single I/O port. For the SCH, SMBBLKDAT is an array of
32 I/O ports, you can access each one directly, so there's nothing to
reset.
> + 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");
> + return -EPERM;
-EOPNOTSUPP
> + }
> + dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
> + outb((size & 0x7), SMBHSTCNT);
This overwrites the other bits of this register... is that OK?
> +
> + if (sch_transaction()) /* Error in transaction */
> + return -EPERM;
Please instead return the actual error value returned by
sch_transaction().
> +
> + if ((read_write == I2C_SMBUS_WRITE) || (size == SCH_QUICK))
> + return 0;
> +
> + switch (size) {
> + case SCH_BYTE:
> + /* Where is the result put? I assume here it is in
> + SMBHSTDAT0 but it might just as well be in the
> + SMBHSTCMD. No clue in the docs */
Please drop this old comment, it doesn't apply here. You know, just
because you copied the code from the old i2c-piix4 driver, doesn't mean
that you had to copy all the crap without thinking whether it applied
to your case or not :(
> + data->byte = inb(SMBHSTDAT0);
> + break;
> + case SCH_BYTE_DATA:
> + data->byte = inb(SMBHSTDAT0);
> + break;
You can merge the SCH_BYTE and SCH_BYTE_DATA cases.
> + case SCH_WORD_DATA:
> + data->word = inb(SMBHSTDAT0) + (inb(SMBHSTDAT1) << 8);
> + break;
> + case SCH_BLOCK_DATA:
> + data->block[0] = inb(SMBHSTDAT0);
> + i = inb(SMBHSTCNT); /* Reset SMBBLKDAT */
Again I doubt that this reset is needed.
> + if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
> + data->block[0] = I2C_SMBUS_BLOCK_MAX;
If data->block[0] > I2C_SMBUS_BLOCK_MAX this means that you received
invalid data, so just return -EPROTO. Same if data->block[0] == 0.
> + 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),
> + .driver_data = 0x40 },
You are not using driver_data anywhere, so what's the point? And you
really don't need it anyway. So please drop all the code related to it.
> + { 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;
> +
> + pci_read_config_word(dev, SMBBA_SCH, &sch_smba);
> + sch_smba &= 0xfff0;
This doesn't match the datasheet which says that the 6 LSBs, not 4, are
unused. But anyway according to the datasheet, these LSBs are hardcoded
to 0 so I don't think that you need this mask at all.
> + if (sch_smba == 0) {
> + dev_err(&dev->dev, "SMB base address uninitialized\n");
> + return -ENODEV;
> + }
> + if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> + dev_err(&dev->dev, "SMB region 0x%x already in use!\n",
> + sch_smba);
> + return -ENODEV;
We normally return -EBUSY in this case.
> + }
Please use "SMBus" instead of "SMB" in both error messages.
> + dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
What about bit 31 of this register? It says whether the I/O area is
enabled or not. If it's not then you're registering the device but it
won't work, that's no good.
> +
> + /* set up the driverfs linkage to our parent device */
There's no such thing as driverfs, you mean sysfs.
> + sch_adapter.dev.parent = &dev->dev;
> +
> + snprintf(sch_adapter.name, I2C_NAME_SIZE,
> + "SMBus SCH adapter at %04x", sch_smba);
Bug. I2C_NAME_SIZE is the size of i2c client names, not i2c adapter
names. Use sizeof(sch_adapter.name) instead.
> +
> + 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 = "sch_smbus",
I'd name it "isch_smbus" for consistency.
> + .id_table = sch_ids,
> + .probe = sch_probe,
> + .remove = __devexit_p(sch_remove),
> + .dynids.use_driver_data = 1,
> +};
> +
> +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 <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("Intel SCH SMBus driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_sch_init);
> +module_exit(i2c_sch_exit);
Please fix all the problems I reported and submit an updated patch.
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080512225718.7440ab42-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2008-05-20 5:56 ` Alek Du
[not found] ` <20080520135635.55238a08-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Alek Du @ 2008-05-20 5:56 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C
Hi Jean,
Modify the patch according to your last comments except for two cases:
> > +
> > + /* If the SMBus is still busy, we give up */
> > + if (timeout >= MAX_TIMEOUT) {
> > + dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
> > + result = -EPERM;
> -ETIMEDOUT
there is no -ETIMEOUT defined
> > + dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
> What about bit 31 of this register? It says whether the I/O area is
> enabled or not. If it's not then you're registering the device but it
> won't work, that's no good.
As I tested, the bit 31 of this register is always 0...
Here is my patch v3, please help to review it:
This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) i2c bus support.
Signed-off-by: Alek Du <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
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
+ 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 <frodol-B0qZmFHriGg@public.gmane.org> and
+ Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
+ - Intel SCH support
+ Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+
+ 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);
+ } else if (temp & 0x06) {
+ 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;
+ }
+ }
+
+ /* 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");
+ }
+ 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");
+ 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),},
+ { 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;
+
+ 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,
+};
+
+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 <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Intel SCH SMBus driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_sch_init);
+module_exit(i2c_sch_exit);
--
1.5.2.5
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080520135635.55238a08-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
@ 2008-05-20 9:31 ` Jean Delvare
[not found] ` <20080520113140.44e40b77-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-05-20 11:10 ` [PATCH v3] " Jean Delvare
1 sibling, 1 reply; 15+ messages in thread
From: Jean Delvare @ 2008-05-20 9:31 UTC (permalink / raw)
To: Alek Du; +Cc: Linux I2C
Hi Alek,
On Tue, 20 May 2008 13:56:35 +0800, Alek Du wrote:
> Modify the patch according to your last comments except for two cases:
>
> > > +
> > > + /* If the SMBus is still busy, we give up */
> > > + if (timeout >= MAX_TIMEOUT) {
> > > + dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
> > > + result = -EPERM;
> >
> > -ETIMEDOUT
>
> there is no -ETIMEOUT defined
ETIMEOUT is indeed not defined, but ETIMEDOUT (what I wrote) is.
> > > + dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
>
> > What about bit 31 of this register? It says whether the I/O area is
> > enabled or not. If it's not then you're registering the device but it
> > won't work, that's no good.
>
> As I tested, the bit 31 of this register is always 0...
Well, the datasheet clearly says otherwise. How did you test bit 31?
Your code uses pci_read_config_word() at the moment so you can't read
bit 31, you'd need to use pci_read_config_dword() instead. If Bit 31
really isn't used by the hardware, please report to whoever at Intel is
in charge of the technical documentation for this chip, so that they
can update the datahseet.
> Here is my patch v3, please help to review it:
I'll review it later today as my time permits.
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080520135635.55238a08-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-05-20 9:31 ` Jean Delvare
@ 2008-05-20 11:10 ` Jean Delvare
1 sibling, 0 replies; 15+ messages in thread
From: Jean Delvare @ 2008-05-20 11:10 UTC (permalink / raw)
To: Alek Du; +Cc: Linux I2C
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 <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 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 <frodol-B0qZmFHriGg@public.gmane.org> and
> + Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
> + - Intel SCH support
> + Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> +
> + 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 <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
> +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
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080520113140.44e40b77-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2008-05-21 2:21 ` Alek Du
[not found] ` <20080521102105.13d75e59-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Alek Du @ 2008-05-21 2:21 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C
Hi Jean,
On Tue, 20 May 2008 17:31:40 +0800
"Jean Delvare" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> ETIMEOUT is indeed not defined, but ETIMEDOUT (what I wrote) is.
Ok, my bad.
>
> Well, the datasheet clearly says otherwise. How did you test bit 31?
> Your code uses pci_read_config_word() at the moment so you can't read
> bit 31, you'd need to use pci_read_config_dword() instead. If Bit 31
Yes, my bad again. fixed now.
Again, here is the updated patch, thanks for your kind review:
This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) i2c bus support.
Signed-off-by: Alek Du <alek.du-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/busses/Kconfig | 10 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-isch.c | 336 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 347 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..5620849 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 SMBus controller on 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..4ae2543
--- /dev/null
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -0,0 +1,336 @@
+/*
+ i2c-isch.c - Linux kernel driver for Intel SCH chipset SMBus
+ - Based on i2c-piix4.c
+ Copyright (c) 1998 - 2002 Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> and
+ Philip Edelbrock <phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>
+ - Intel SCH support
+ Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+
+ 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);
+ }
+ if (temp & 0x06) {
+ 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 -EAGAIN;
+ }
+ }
+
+ /* 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 = -ETIMEDOUT;
+ }
+ 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 = -EIO;
+ dev_err(&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 = -ENXIO;
+ dev_dbg(&sch_adapter.dev, "No such address.\n");
+ }
+ 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_warn(&adap->dev, "Unsupported transaction %d\n", size);
+ 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) },
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, sch_ids);
+
+static int __devinit sch_probe(struct pci_dev *dev,
+ const struct pci_device_id *id)
+{
+ int retval;
+ unsigned int smba;
+
+ pci_read_config_dword(dev, SMBBA_SCH, &smba);
+ if (!(smba & (1 << 31))) {
+ dev_err(&dev->dev, "SMBus I/O space disabled!\n");
+ return -ENODEV;
+ }
+
+ sch_smba = (unsigned short)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),
+};
+
+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 <jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Intel SCH SMBus driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_sch_init);
+module_exit(i2c_sch_exit);
--
1.5.2.5
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] i2c: Add Intel SCH I2C SMBus support
[not found] ` <20080521102105.13d75e59-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
@ 2008-05-21 9:04 ` Jean Delvare
0 siblings, 0 replies; 15+ messages in thread
From: Jean Delvare @ 2008-05-21 9:04 UTC (permalink / raw)
To: Alek Du; +Cc: Linux I2C
Hi Alek,
On Wed, 21 May 2008 10:21:05 +0800, Alek Du wrote:
> On Tue, 20 May 2008 17:31:40 +0800
> "Jean Delvare" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> > ETIMEOUT is indeed not defined, but ETIMEDOUT (what I wrote) is.
> Ok, my bad.
> >
> > Well, the datasheet clearly says otherwise. How did you test bit 31?
> > Your code uses pci_read_config_word() at the moment so you can't read
> > bit 31, you'd need to use pci_read_config_dword() instead. If Bit 31
> Yes, my bad again. fixed now.
>
> Again, here is the updated patch, thanks for your kind review:
>
> This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) i2c bus support.
OK, this is the right one this time. I've added your patch to my i2c
tree as:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-add-intel-sch-i2c-smbus-support.patch
It will go in linux-next and is scheduled for merge in kernel
2.6.27-rc1.
I've also updated sensors-detect and http://www.lm-sensors.org/wiki/Devices .
Any further patch to this driver should be an incremental one based on
what I have in my tree. Things that could be improved include:
* Allocate the device data dynamically to get rid of the assumption
that only one SMBus interface is present.
* Rework the timeout mechanism so that it doesn't depend on HZ.
* Documentation. Most SMBus host controller drivers have a
documentation file under Documentation/i2c/busses, it would be nice to
have one for this new i2c-isch driver.
Thanks,
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-05-21 9:04 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-04-24 2:05 [PATCH] i2c: Add Intel SCH I2C SMBus support Alek Du
[not found] ` <20080424100510.09cc2d4b-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-04-25 11:49 ` Rudolf Marek
2008-04-25 21:00 ` Rudolf Marek
[not found] ` <48124673.8020808-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
2008-04-26 7:50 ` Jean Delvare
[not found] ` <20080426095011.2d27b75b-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-04-27 13:56 ` Alek Du
2008-04-28 6:45 ` Alek Du
[not found] ` <20080428144514.53aa54fd-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-04-28 8:57 ` Jean Delvare
[not found] ` <20080428105738.06429ed2-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-04-29 3:06 ` [PATCH] i2c: Add Intel SCH I2C SMBus support (revised) Alek Du
[not found] ` <8AD95083DC3E36478732061D97524415030F8FAA@pdsmsx411.ccr.corp.intel.com>
[not found] ` <20080430080351.57b8c21d@hyperion.delvare>
[not found] ` <20080505141807.1aa458e1@dxy.sh.intel.com>
[not found] ` <20080505110757.6454c220@hyperion.delvare>
[not found] ` <20080506095036.274e91f1@dxy.sh.intel.com>
[not found] ` <20080506095036.274e91f1-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-05-12 20:57 ` Jean Delvare
[not found] ` <20080512225718.7440ab42-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-05-20 5:56 ` [PATCH v3] i2c: Add Intel SCH I2C SMBus support Alek Du
[not found] ` <20080520135635.55238a08-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-05-20 9:31 ` Jean Delvare
[not found] ` <20080520113140.44e40b77-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-05-21 2:21 ` [PATCH v4] " Alek Du
[not found] ` <20080521102105.13d75e59-PCb9FJy6fea75v1z/vFq2g@public.gmane.org>
2008-05-21 9:04 ` Jean Delvare
2008-05-20 11:10 ` [PATCH v3] " Jean Delvare
2008-05-12 19:40 ` [PATCH] " Jean Delvare
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox