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 <djkurtz@xxxxxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
