Hi Daniel, On Fri, 6 Jan 2012 18:58:21 +0800, Daniel Kurtz wrote: > Add a new 'feature' to i2c-i801 to enable using i801 interrupts. > When the feature is enabled, then an isr is installed for the device's > pci irq. > > An i2c/smbus transaction is always terminated by one of the following > interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR
Missing trailing dot. > When the isr fires for one of these cases, it sets the ->status variable > and wakes up the waitq. The waitq then saves off the status code, and > clears ->status (in preperation for some future transaction). Typo: preparation. > The SMBus controller generates an INTR irq at the end of each > transaction where INTREN was set in the HST_CNT register. > > No locking is needed around accesses to priv->status since all writes to > it are serialized: it is only ever set once in the isr at the end of a > transaction, and cleared while no irqs can occur. In addition, the i2c > adapter lock guarantees that entire i2c transactions for a single > adapter are always serialized. If no locking is needed, then why do you introduce and initialize a spinlock? > For this patch, the INTREN bit is set only for smbus block, byte and word > transactions, but not for emulated i2c reads or writes. The use of the I don't understand the "emulated" in this sentence. > DS (BYTE_DONE) interrupt with byte-by-byte i2c transactions is > implemented in a subsequent patch. > > Signed-off-by: Daniel Kurtz <[email protected]> > --- > drivers/i2c/busses/i2c-i801.c | 107 > ++++++++++++++++++++++++++++++++++++++--- > 1 files changed, 100 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index f3418cf..b123133 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -59,10 +59,12 @@ > Block process call transaction no > I2C block read transaction yes (doesn't use the block buffer) > Slave mode no > + Interrupt processing yes > > See the file Documentation/i2c/busses/i2c-i801 for details. > */ > > +#include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/kernel.h> > @@ -90,6 +92,7 @@ > > /* PCI Address Constants */ > #define SMBBAR 4 > +#define SMBPCISTS 0x006 > #define SMBHSTCFG 0x040 > > /* Host configuration bits for SMBHSTCFG */ > @@ -97,6 +100,9 @@ > #define SMBHSTCFG_SMB_SMI_EN 2 > #define SMBHSTCFG_I2C_EN 4 > > +/* Host status bits for SMBHSTSTS */ I guess you meant "PCI status bits for SMBPCISTS"? > +#define SMBPCISTS_INTS 8 I'd prefer this to be expressed as an hexadecimal mask. > + > /* Auxiliary control register bits, ICH4+ only */ > #define SMBAUXCTL_CRC 1 > #define SMBAUXCTL_E32B 2 > @@ -106,7 +112,6 @@ > > /* Other settings */ > #define MAX_TIMEOUT 100 > -#define ENABLE_INT9 0 /* set to 0x01 to enable - untested */ > > /* I801 command constants */ > #define I801_QUICK 0x00 > @@ -116,7 +121,11 @@ > #define I801_PROC_CALL 0x10 /* unimplemented */ > #define I801_BLOCK_DATA 0x14 > #define I801_I2C_BLOCK_DATA 0x18 /* ICH5 and later */ > -#define I801_LAST_BYTE 0x20 > + > +/* I801 Hosts Control register bits */ > +#define I801_INTREN 0x01 > +#define I801_KILL 0x02 This is redundant with SMBHSTCNT_KILL, right? > +#define I801_LAST_BYTE 0x20 /* ICH5 and later */ Not true, this bit exists prior to ICH5. > #define I801_START 0x40 > #define I801_PEC_EN 0x80 /* ICH3 and later */ > > @@ -134,6 +143,9 @@ > SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \ > SMBHSTSTS_INTR) > > +#define STATUS_RESULT_FLAGS (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \ > + SMBHSTSTS_DEV_ERR | SMBHSTSTS_INTR) > + You could swap the definitions so you can define STATUS_FLAGS in terms of STATUS_RESULT_FLAGS. > /* Older devices have their ID defined in <linux/pci_ids.h> */ > #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS 0x1c22 > #define PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS 0x1d22 > @@ -151,6 +163,11 @@ struct i801_priv { > unsigned char original_hstcfg; > struct pci_dev *pci_dev; > unsigned int features; > + > + /* isr processing */ > + wait_queue_head_t waitq; This needs <linux/wait.h>. > + spinlock_t lock; This needs <linux/spinlock.h> (if you need this at all...) > + u8 status; > }; > > static struct pci_driver i801_driver; > @@ -159,6 +176,7 @@ static struct pci_driver i801_driver; > #define FEATURE_BLOCK_BUFFER (1 << 1) > #define FEATURE_BLOCK_PROC (1 << 2) > #define FEATURE_I2C_BLOCK_READ (1 << 3) > +#define FEATURE_IRQ (1 << 4) > /* Not really a feature, but it's convenient to handle it as such */ > #define FEATURE_IDF (1 << 15) > > @@ -167,6 +185,7 @@ static const char *i801_feature_names[] = { > "Block buffer", > "Block process call", > "I2C block read", > + "Interrupt" Trailing comma please. > }; > > static unsigned int disable_features; > @@ -207,7 +226,12 @@ static int i801_check_post(struct i801_priv *priv, int > status, int timeout) > { > int result = 0; > > - /* If the SMBus is still busy, we give up */ > + /* > + * If the SMBus is still busy, we give up > + * Note: This timeout condition only happens when using polling > + * transactions. For interrupt operation, NAK/timeout is indicated by > + * DEV_ERR. > + */ > if (timeout) { > dev_err(&priv->pci_dev->dev, "Transaction timeout\n"); > /* try to stop the current command */ > @@ -265,8 +289,15 @@ static int i801_transaction(struct i801_priv *priv, int > xact) > if (result < 0) > return result; > > + if (priv->features & FEATURE_IRQ) { > + outb(xact | I801_INTREN | I801_START, SMBHSTCNT(priv)); > + wait_event(priv->waitq, (status = priv->status)); > + priv->status = 0; > + return i801_check_post(priv, status, 0); > + } > + > /* the current contents of SMBHSTCNT can be overwritten, since PEC, > - * INTREN, SMBSCMD are passed in xact */ > + * SMBSCMD are passed in xact */ > outb_p(xact | I801_START, SMBHSTCNT(priv)); > > /* We will always wait for a fraction of a second! */ > @@ -280,6 +311,7 @@ static int i801_transaction(struct i801_priv *priv, int > xact) > return result; > > outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv)); > + > return 0; Please avoid mixing whitespace changes with real changes. > } > > @@ -318,7 +350,7 @@ static int i801_block_transaction_by_block(struct > i801_priv *priv, > outb_p(data->block[i+1], SMBBLKDAT(priv)); > } > > - status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 | > + status = i801_transaction(priv, I801_BLOCK_DATA | Dropping ENABLE_INT9 would have made a nice preliminary cleanup patch... > (hwpec ? I801_PEC_EN : 0)); > if (status) > return status; > @@ -336,6 +368,44 @@ static int i801_block_transaction_by_block(struct > i801_priv *priv, > } > > /* > + * i801 signals transaction completion with one of these interrupts: > + * INTR - Success > + * DEV_ERR - Invalid command, NAK or communication timeout > + * BUS_ERR - SMI# transaction collision > + * FAILED - transaction was canceled due to a KILL request > + * When any of these occur, update ->status and wake up the waitq. > + * ->status must be cleared before kicking off the next transaction. > + */ > +static irqreturn_t i801_isr(int irq, void *dev_id) > +{ > + struct i801_priv *priv = dev_id; > + u8 pcists, hststs; > + > + /* Confirm this is our interrupt */ > + pci_read_config_byte(priv->pci_dev, SMBPCISTS, &pcists); According to the datasheet this is a 16-bit register, you should read it with pci_read_config_word(). > + if (!(pcists & SMBPCISTS_INTS)) { > + dev_dbg(&priv->pci_dev->dev, "irq: pcists.ints not set\n"); This is expected in case of shared interrupts, right? > + return IRQ_NONE; > + } > + > + hststs = inb(SMBHSTSTS(priv)); BTW, the rest of the driver is using inb_p/outb_p instead of inb/outb. Do you believe it would be safe to use inb/outb everywhere else? > + dev_dbg(&priv->pci_dev->dev, "irq: hststs = %02x\n", hststs); > + > + /* > + * Clear irq sources and report transaction result. > + * ->status must be cleared before the next transaction is started. > + */ > + hststs &= STATUS_RESULT_FLAGS; > + if (hststs) { If not, something's seriously wrong, isn't it? > + outb(hststs, SMBHSTSTS(priv)); > + priv->status |= hststs; > + wake_up(&priv->waitq); > + } > + > + return IRQ_HANDLED; > +} > + > +/* > * i2c write uses cmd=I801_BLOCK_DATA, I2C_EN=1 > * i2c read uses cmd=I801_I2C_BLOCK_DATA > */ > @@ -370,7 +440,7 @@ static int i801_block_transaction_byte_by_byte(struct > i801_priv *priv, > for (i = 1; i <= len; i++) { > if (i == len && read_write == I2C_SMBUS_READ) > smbcmd |= I801_LAST_BYTE; > - outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv)); > + outb_p(smbcmd, SMBHSTCNT(priv)); > > if (i == 1) > outb_p(inb(SMBHSTCNT(priv)) | I801_START, > @@ -571,7 +641,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > ret = i801_block_transaction(priv, data, read_write, size, > hwpec); > else > - ret = i801_transaction(priv, xact | ENABLE_INT9); > + ret = i801_transaction(priv, xact); > > /* Some BIOSes don't like it when PEC is enabled at reboot or resume > time, so we forcibly disable it after every transaction. Turn off > @@ -805,6 +875,10 @@ static int __devinit i801_probe(struct pci_dev *dev, > break; > } > > + /* IRQ processing only tested on CougarPoint PCH */ I'll test on other chips, I have ICH3-M, ICH5, ICH7 and ICH10 here. The INTS bit in PCI Status Register is new in ICH5 so your code will not work on older chips. I think interrupts were supported before that, but probably not shared interrupts? OTOH I'm not completely sure why you check this bit in the first place... hststs == 0 should be enough to detect the not-for-us shared interrupt case? > + if (dev->device == PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS) > + priv->features |= FEATURE_IRQ; > + > /* Disable features on user request */ > for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) { > if (priv->features & disable_features & (1 << i)) > @@ -879,8 +953,24 @@ static int __devinit i801_probe(struct pci_dev *dev, > i801_probe_optional_slaves(priv); > > pci_set_drvdata(dev, priv); > + > + if (priv->features & FEATURE_IRQ) { > + init_waitqueue_head(&priv->waitq); > + spin_lock_init(&priv->lock); > + > + err = request_irq(dev->irq, i801_isr, IRQF_SHARED, > + i801_driver.name, priv); > + if (err) { > + dev_err(&dev->dev, "Failed to allocate irq %d: %d", Missing "\n". > + dev->irq, err); > + goto exit_del_adapter; > + } I believe order is wrong, and interrupt handler should be installed _before_ registering the adapter. Otherwise you have a race condition where the handler could be called before the waitqueue and spinlock are initialized. > + } > return 0; > > +exit_del_adapter: > + pci_set_drvdata(dev, NULL); > + i2c_del_adapter(&priv->adapter); > exit_release: > pci_release_region(dev, SMBBAR); > exit: > @@ -892,6 +982,9 @@ static void __devexit i801_remove(struct pci_dev *dev) > { > struct i801_priv *priv = pci_get_drvdata(dev); > > + if (priv->features & FEATURE_IRQ) > + free_irq(dev->irq, priv); > + > i2c_del_adapter(&priv->adapter); Here again I think order is wrong and you should delete the adapter first, otherwise there is a small chance that you free the irq while an SMBus transaction is being processed. > pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); > pci_release_region(dev, SMBBAR); -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
