[PATCH 01/03] i2c-i801: Add basic interrupt support

[PATCH 01/03] i2c-i801: Add basic interrupt support

[Ivo Manca]

[-- Attachment #1: Type: text/plain, Size: 144 bytes --]

This patch adds basic interrupt support to the i2c-i801 driver.

Signed-off-by: Ivo Manca <pinkel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---


[-- Attachment #2: patch1.diff --]
[-- Type: text/plain, Size: 5450 bytes --]

--- i2c-i801.c.git	2008-07-03 15:47:32.000000000 +0200
+++ i2c-i801.c	2008-07-07 15:04:25.000000000 +0200
@@ -63,6 +63,7 @@
 #include <linux/delay.h>
 #include <linux/ioport.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <asm/io.h>
 
@@ -100,6 +101,7 @@
 
 /* I801 command constants */
 #define I801_QUICK		0x00
+#define I801_HST_CNT_INTREN	0x01
 #define I801_BYTE		0x04
 #define I801_BYTE_DATA		0x08
 #define I801_WORD_DATA		0x0C
@@ -121,23 +123,65 @@
 #define SMBHSTSTS_INTR		0x02
 #define SMBHSTSTS_HOST_BUSY	0x01
 
+/* mask for events we normally handle */
+#define I801_HST_STS_MASK_NORM ( \
+		SMBHSTSTS_FAILED | \
+		SMBHSTSTS_BUS_ERR | \
+		SMBHSTSTS_DEV_ERR | \
+		SMBHSTSTS_INTR)
+
+/* mask for all events */
+#define I801_HST_STS_MASK_ALL ( \
+		SMBHSTSTS_BYTE_DONE | \
+		SMBHSTSTS_SMBALERT_STS | \
+		SMBHSTSTS_FAILED | \
+		SMBHSTSTS_BUS_ERR | \
+		SMBHSTSTS_DEV_ERR | \
+		SMBHSTSTS_INTR)
+
 static unsigned long i801_smba;
 static unsigned char i801_original_hstcfg;
+static struct i2c_adapter i801_adapter;
 static struct pci_driver i801_driver;
 static struct pci_dev *I801_dev;
 
+struct i2c_i801_algo_data {
+	spinlock_t lock;
+	wait_queue_head_t waitq;
+	int status; /* copy of h/w register */
+	int irq;
+};
+
+static struct i2c_i801_algo_data i801_algo_data;
+
 #define FEATURE_SMBUS_PEC	(1 << 0)
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
 #define FEATURE_I2C_BLOCK_READ	(1 << 3)
 static unsigned int i801_features;
 
+/* fetch & consume host status out of algo_data */
+static inline int i801_get_status(struct i2c_i801_algo_data *algo_data)
+{
+	unsigned long flags;
+	int status;
+
+	spin_lock_irqsave(&algo_data->lock, flags);
+	status = algo_data->status;
+	algo_data->status = 0;
+	spin_unlock_irqrestore(&algo_data->lock, flags);
+
+	return status;
+}
+
 static int i801_transaction(int xact)
 {
 	int temp;
 	int result = 0;
 	int timeout = 0;
 
+	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
+
 	dev_dbg(&I801_dev->dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
 		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
@@ -159,6 +203,13 @@ static int i801_transaction(int xact)
 
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * INTREN, SMBSCMD are passed in xact */
+	if (algo_data->irq >= 0) {
+		outb_p(xact | I801_START | I801_HST_CNT_INTREN, SMBHSTCNT);
+
+		wait_event_interruptible_timeout(algo_data->waitq,
+					((temp = i801_get_status(algo_data))
+					& I801_HST_STS_MASK_NORM), HZ/2);
+	} else {
 	outb_p(xact | I801_START, SMBHSTCNT);
 
 	/* We will always wait for a fraction of a second! */
@@ -177,6 +228,7 @@ static int i801_transaction(int xact)
 		msleep(1);
 		outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL), SMBHSTCNT);
 	}
+	}
 
 	if (temp & SMBHSTSTS_FAILED) {
 		result = -1;
@@ -574,6 +626,7 @@ static struct i2c_adapter i801_adapter =
 	.id		= I2C_HW_SMBUS_I801,
 	.class		= I2C_CLASS_HWMON,
 	.algo		= &smbus_algorithm,
+	.algo_data	= &i801_algo_data
 };
 
 static struct pci_device_id i801_ids[] = {
@@ -597,8 +650,38 @@ static struct pci_device_id i801_ids[] =
 
 MODULE_DEVICE_TABLE (pci, i801_ids);
 
+static irqreturn_t i801_isr(int irq, void *dev_id)
+{
+	u8 status = inb(SMBHSTSTS);
+
+	/* bail if it's not ours */
+	if (!(status & I801_HST_STS_MASK_ALL)) {
+		dev_dbg(&I801_dev->dev, "BAILING interrupt\n");
+		return IRQ_NONE;
+	}
+
+	dev_dbg(&I801_dev->dev, "INTERRUPT: IRQ status: 0x%02x\n", status);
+
+	/* ACK */
+	outb((status & I801_HST_STS_MASK_ALL), SMBHSTSTS);
+
+	if (status & I801_HST_STS_MASK_NORM) {
+		struct i2c_adapter *adap = dev_id;
+		struct i2c_i801_algo_data *algo_data = adap->algo_data;
+		unsigned long flags;
+
+		spin_lock_irqsave(&algo_data->lock, flags);
+		algo_data->status = status;
+		spin_unlock_irqrestore(&algo_data->lock, flags);
+		wake_up_interruptible(&algo_data->waitq);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int __devinit i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
+	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
 	unsigned char temp;
 	int err;
 
@@ -656,10 +739,27 @@ static int __devinit i801_probe(struct p
 	}
 	pci_write_config_byte(I801_dev, SMBHSTCFG, temp);
 
-	if (temp & SMBHSTCFG_SMB_SMI_EN)
+	if (temp & SMBHSTCFG_SMB_SMI_EN) {
 		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
-	else
+		algo_data->irq = -1;
+	} else {
 		dev_dbg(&dev->dev, "SMBus using PCI Interrupt\n");
+		if ((request_irq(I801_dev->irq, i801_isr, IRQF_SHARED,
+				 i801_adapter.name, &i801_adapter))) {
+			dev_err(&dev->dev,
+				"request irq %d failed!\n", I801_dev->irq);
+			algo_data->irq = -1;
+		} else {
+			dev_dbg(&dev->dev,
+				"SMBus base address: 0x%04lx, IRQ: %d\n",
+				i801_smba, I801_dev->irq);
+
+			algo_data->irq = I801_dev->irq;
+			algo_data->status = 0;
+			init_waitqueue_head(&algo_data->waitq);
+			spin_lock_init(&algo_data->lock);
+		}
+	}
 
 	/* Clear special mode bits */
 	if (i801_features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))
@@ -686,6 +786,10 @@ exit:
 
 static void __devexit i801_remove(struct pci_dev *dev)
 {
+	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
+	if (algo_data->irq >= 0)
+		free_irq(dev->irq, &i801_adapter);
+
 	i2c_del_adapter(&i801_adapter);
 	pci_write_config_byte(I801_dev, SMBHSTCFG, i801_original_hstcfg);
 	pci_release_region(dev, SMBBAR);

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 01/03] i2c-i801: Add basic interrupt support

[Jean Delvare]

On Wed, 23 Jul 2008 18:56:40 +0200, Ivo Manca wrote:
> This patch adds basic interrupt support to the i2c-i801 driver.
> 
> Signed-off-by: Ivo Manca <pinkel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

Quick review:

> --- i2c-i801.c.git	2008-07-03 15:47:32.000000000 +0200
> +++ i2c-i801.c	2008-07-07 15:04:25.000000000 +0200

Please provide patches that can be applied with patch -p1 from the root
of the kernel source tree.

> @@ -63,6 +63,7 @@
>  #include <linux/delay.h>
>  #include <linux/ioport.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/i2c.h>
>  #include <asm/io.h>
>  
> @@ -100,6 +101,7 @@
>  
>  /* I801 command constants */
>  #define I801_QUICK		0x00
> +#define I801_HST_CNT_INTREN	0x01

Having this here is a bit confusing, you should leave all the
transaction types (from I801_QUICK to I801_I2C_BLOCK_LAST) as a block.
I801_HST_CNT_INTREN (which could probably be renamed to just
I801_INTREN for brevity) would go first.

Also note that the drivers already had a define for this flag, named
ENABLE_INT9, although it was disabled. I agree that I801_INTREN is a
better name, but it would probably be a good idea to delete ENABLE_INT9
and all references thereto first, otherwise the code becomes a little
confusing.

>  #define I801_BYTE		0x04
>  #define I801_BYTE_DATA		0x08
>  #define I801_WORD_DATA		0x0C
> @@ -121,23 +123,65 @@
>  #define SMBHSTSTS_INTR		0x02
>  #define SMBHSTSTS_HOST_BUSY	0x01
>  
> +/* mask for events we normally handle */
> +#define I801_HST_STS_MASK_NORM ( \
> +		SMBHSTSTS_FAILED | \
> +		SMBHSTSTS_BUS_ERR | \
> +		SMBHSTSTS_DEV_ERR | \
> +		SMBHSTSTS_INTR)
> +
> +/* mask for all events */
> +#define I801_HST_STS_MASK_ALL ( \
> +		SMBHSTSTS_BYTE_DONE | \
> +		SMBHSTSTS_SMBALERT_STS | \
> +		SMBHSTSTS_FAILED | \
> +		SMBHSTSTS_BUS_ERR | \
> +		SMBHSTSTS_DEV_ERR | \
> +		SMBHSTSTS_INTR)
> +
>  static unsigned long i801_smba;
>  static unsigned char i801_original_hstcfg;
> +static struct i2c_adapter i801_adapter;
>  static struct pci_driver i801_driver;
>  static struct pci_dev *I801_dev;
>  
> +struct i2c_i801_algo_data {
> +	spinlock_t lock;
> +	wait_queue_head_t waitq;
> +	int status; /* copy of h/w register */
> +	int irq;

You never really use the irq value, only whether it is -1 or not. So it
might make more sense to turn it into a flag and rename it to use_irq?

> +};
> +
> +static struct i2c_i801_algo_data i801_algo_data;
> +
>  #define FEATURE_SMBUS_PEC	(1 << 0)
>  #define FEATURE_BLOCK_BUFFER	(1 << 1)
>  #define FEATURE_BLOCK_PROC	(1 << 2)
>  #define FEATURE_I2C_BLOCK_READ	(1 << 3)
>  static unsigned int i801_features;
>  
> +/* fetch & consume host status out of algo_data */
> +static inline int i801_get_status(struct i2c_i801_algo_data *algo_data)
> +{
> +	unsigned long flags;
> +	int status;
> +
> +	spin_lock_irqsave(&algo_data->lock, flags);
> +	status = algo_data->status;
> +	algo_data->status = 0;
> +	spin_unlock_irqrestore(&algo_data->lock, flags);
> +
> +	return status;
> +}
> +
>  static int i801_transaction(int xact)
>  {
>  	int temp;
>  	int result = 0;
>  	int timeout = 0;
>  
> +	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
> +
>  	dev_dbg(&I801_dev->dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
>  		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
>  		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
> @@ -159,6 +203,13 @@ static int i801_transaction(int xact)
>  
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * INTREN, SMBSCMD are passed in xact */
> +	if (algo_data->irq >= 0) {
> +		outb_p(xact | I801_START | I801_HST_CNT_INTREN, SMBHSTCNT);
> +
> +		wait_event_interruptible_timeout(algo_data->waitq,
> +					((temp = i801_get_status(algo_data))
> +					& I801_HST_STS_MASK_NORM), HZ/2);

Please make this HZ/2 a define so that it's easy to change. Ideally
this define would replace MAX_TIMEOUT (in a separate patch) so that both
the poll-based and the interrupt-driven paths have the same timeout
value. Right now, the timeout handling of the poll-based path is
rather ugly (the actual timeout depends on the value of HZ.)

Not sure why you make this wait interruptible. The poll-based path
isn't interruptible, and you do not handle the interrupted case anyway.

The polled-based path has additional logic to handle the timeout case
(by resetting the controller.) I would guess you want the same for the
interrupt-driven path?

> +	} else {
>  	outb_p(xact | I801_START, SMBHSTCNT);
>  
>  	/* We will always wait for a fraction of a second! */
> @@ -177,6 +228,7 @@ static int i801_transaction(int xact)
>  		msleep(1);
>  		outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL), SMBHSTCNT);
>  	}
> +	}
>  
>  	if (temp & SMBHSTSTS_FAILED) {
>  		result = -1;
> @@ -574,6 +626,7 @@ static struct i2c_adapter i801_adapter =
>  	.id		= I2C_HW_SMBUS_I801,
>  	.class		= I2C_CLASS_HWMON,
>  	.algo		= &smbus_algorithm,
> +	.algo_data	= &i801_algo_data

Please add a trailing comma.

>  };
>  
>  static struct pci_device_id i801_ids[] = {
> @@ -597,8 +650,38 @@ static struct pci_device_id i801_ids[] =
>  
>  MODULE_DEVICE_TABLE (pci, i801_ids);
>  
> +static irqreturn_t i801_isr(int irq, void *dev_id)
> +{
> +	u8 status = inb(SMBHSTSTS);
> +
> +	/* bail if it's not ours */
> +	if (!(status & I801_HST_STS_MASK_ALL)) {
> +		dev_dbg(&I801_dev->dev, "BAILING interrupt\n");
> +		return IRQ_NONE;
> +	}
> +
> +	dev_dbg(&I801_dev->dev, "INTERRUPT: IRQ status: 0x%02x\n", status);
> +
> +	/* ACK */
> +	outb((status & I801_HST_STS_MASK_ALL), SMBHSTSTS);
> +
> +	if (status & I801_HST_STS_MASK_NORM) {
> +		struct i2c_adapter *adap = dev_id;
> +		struct i2c_i801_algo_data *algo_data = adap->algo_data;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&algo_data->lock, flags);
> +		algo_data->status = status;
> +		spin_unlock_irqrestore(&algo_data->lock, flags);
> +		wake_up_interruptible(&algo_data->waitq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int __devinit i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> +	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
>  	unsigned char temp;
>  	int err;
>  
> @@ -656,10 +739,27 @@ static int __devinit i801_probe(struct p
>  	}
>  	pci_write_config_byte(I801_dev, SMBHSTCFG, temp);
>  
> -	if (temp & SMBHSTCFG_SMB_SMI_EN)
> +	if (temp & SMBHSTCFG_SMB_SMI_EN) {
>  		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
> -	else
> +		algo_data->irq = -1;
> +	} else {
>  		dev_dbg(&dev->dev, "SMBus using PCI Interrupt\n");
> +		if ((request_irq(I801_dev->irq, i801_isr, IRQF_SHARED,
> +				 i801_adapter.name, &i801_adapter))) {

i801_adapter.name is a rather long string, I think I'd rather use
i801_driver.name to register the IRQ.

> +			dev_err(&dev->dev,
> +				"request irq %d failed!\n", I801_dev->irq);
> +			algo_data->irq = -1;
> +		} else {
> +			dev_dbg(&dev->dev,
> +				"SMBus base address: 0x%04lx, IRQ: %d\n",
> +				i801_smba, I801_dev->irq);
> +
> +			algo_data->irq = I801_dev->irq;
> +			algo_data->status = 0;
> +			init_waitqueue_head(&algo_data->waitq);
> +			spin_lock_init(&algo_data->lock);
> +		}
> +	}
>  
>  	/* Clear special mode bits */
>  	if (i801_features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))
> @@ -686,6 +786,10 @@ exit:
>  
>  static void __devexit i801_remove(struct pci_dev *dev)
>  {
> +	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
> +	if (algo_data->irq >= 0)
> +		free_irq(dev->irq, &i801_adapter);
> +
>  	i2c_del_adapter(&i801_adapter);

I think you have a race here. You free the irq before removing the i2c
adapter. What will happen if someone initiates an I2C transaction after
the IRQ has been freed? It would probably be better to remove the i2c
adapter first.

>  	pci_write_config_byte(I801_dev, SMBHSTCFG, i801_original_hstcfg);
>  	pci_release_region(dev, SMBBAR);

Other than that, it looks good to me, good work. Note though that I am
in no way an expert of interrupt handling, as this is the first
PC-style SMBus controller driver which gets converted to use interrupts
instead of polling.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 01/03] i2c-i801: Add basic interrupt support

[Ivo Manca]

Hey Jean,
> Please provide patches that can be applied with patch -p1 from the root
> of the kernel source tree.
>   
Will do, sorry about that

> Having this here is a bit confusing, you should leave all the
> transaction types (from I801_QUICK to I801_I2C_BLOCK_LAST) as a block.
>   
Oops, not a smart place to put this constant, I agree. Moved to

> I801_HST_CNT_INTREN (which could probably be renamed to just
> I801_INTREN for brevity) would go first.
>
> Also note that the drivers already had a define for this flag, named
> ENABLE_INT9, although it was disabled. I agree that I801_INTREN is a
> better name, but it would probably be a good idea to delete ENABLE_INT9
> and all references thereto first, otherwise the code becomes a little
> confusing.
>   
Will do. I'll just delete ENABLE_INT9 all together.

> You never really use the irq value, only whether it is -1 or not. So it
> might make more sense to turn it into a flag and rename it to use_irq?
>   
Yup, seems like a waste to store the value :)

> Please make this HZ/2 a define so that it's easy to change. 
Done

> Ideally this define would replace MAX_TIMEOUT (in a separate patch) so 
> that both
> the poll-based and the interrupt-driven paths have the same timeout 
> value. Right now,
> the timeout handling of the poll-based path is rather ugly (the actual 
> timeout depends on
> the value of HZ.)
Hm, seems like a very sensible thing to do. However, I have no idea how 
to implement that, since you don't really know how long msleep slept, or 
not? I sadly don't really have time to look into this right now, sorry.

> Not sure why you make this wait interruptible. The poll-based path
> isn't interruptible, and you do not handle the interrupted case anyway.
>   
I'm not an expert, but I think this is the right way to use it.
The interupt handler i801_isr catches the interrupt, saves the status to 
algo_data and calls wake_up_interruptable. This will wake up the wait.

I'm not sure whether this is the correct way or not. Any input from an 
interrupt expert is very much welcome.
I did test it without the event_interruptable, which produces nonsense, 
like:

====
polling
====
[root@localhost busses]# time i2cdetect -y 0
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- --
50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- --
70: -- -- -- UU -- -- -- --

real    0m0.235s
user    0m0.000s
sys     0m0.003s

====
wait_event_interruptable_timeout
====
[root@localhost busses]# time i2cdetect -y 0
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- --
50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- --
70: -- -- -- UU -- -- -- --

real    0m0.130s
user    0m0.001s
sys     0m0.009s

====
wait_event_timeout
====
[root@localhost busses]# time i2cdetect -y 0
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- 0a -- -- -- -- --
10: 10 -- -- -- -- -- -- -- -- 19 -- -- -- -- 1e --
20: -- -- -- -- 24 -- -- -- -- 29 -- -- -- -- -- --
30: 30 -- -- 33 -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

real    0m4.090s
user    0m0.000s
sys     0m0.004s

> The polled-based path has additional logic to handle the timeout case
> (by resetting the controller.) I would guess you want the same for the
> interrupt-driven path?
>   
Agree. After merging to 2.6.27-rc3, I noticed a helper function called 
i801_check_post was added. Seems to be the ideal place to pass the 
return data to.

> Please add a trailing comma.
>   
Done
> i801_adapter.name is a rather long string, I think I'd rather use
> i801_driver.name to register the IRQ.
>   
Ok
> I think you have a race here. You free the irq before removing the i2c
> adapter. What will happen if someone initiates an I2C transaction after
> the IRQ has been freed? It would probably be better to remove the i2c
> adapter first.
>   
Quite an obvious race as well... Changed already.
> Other than that, it looks good to me, good work. Note though that I am
> in no way an expert of interrupt handling, as this is the first
> PC-style SMBus controller driver which gets converted to use interrupts
> instead of polling.
>   
Thanks. I'm by no means an expert as well, it's all quite new to me. 
Thanks a lot for your patience.

I'll send you an updated patch later this evening. I don't know anything 
about config files though, so that'll have to wait a bit... The module 
parameter will be added though.

Ivo

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 01/03] i2c-i801: Add basic interrupt support

[Ivo Manca]

Ivo Manca wrote: 
> I'm not an expert, but I think this is the right way to use it.
> The interupt handler i801_isr catches the interrupt, saves the status 
> to algo_data and calls wake_up_interruptable. This will wake up the wait.
>
> I'm not sure whether this is the correct way or not. Any input from an 
> interrupt expert is very much welcome.
> I did test it without the event_interruptable, which produces 
> nonsense, like:
>
> bla bla
>
Ugh, sorry, please discard this comment since it's utter nonsense ;). 
Reading wait.h cleared things up for me. I shouldn't indeed use the 
interruptible function.
The errors were due to a coding mistake from my side.

Ivo

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 01/03] i2c-i801: Add basic interrupt support

[Jean Delvare]

On Wed, 13 Aug 2008 21:17:22 +0200, Ivo Manca wrote:
> Hey Jean,
> > Ideally this define would replace MAX_TIMEOUT (in a separate patch) so 
> > that both
> > the poll-based and the interrupt-driven paths have the same timeout 
> > value. Right now,
> > the timeout handling of the poll-based path is rather ugly (the actual 
> > timeout depends on
> > the value of HZ.)
>
> Hm, seems like a very sensible thing to do. However, I have no idea how 
> to implement that, since you don't really know how long msleep slept, or 
> not? I sadly don't really have time to look into this right now, sorry.

The trick is to not rely on the duration of the msleep(). As you
rightly said, we have no clue how long it slept. Instead, we remember
the initial value of jiffies, and check after each iteration whether
the timeout has been exceeded or not. Something like:

	unsigned long start = jiffies, now;

	while (!time_after(now, jiffies + timeout)
	 && !<other condition>) {
		msleep(1);
		<do something>
	}

But if you don't have the time, this can be done later, no worry. It
doesn't belong to your patch anyway.

> (...)
> I'll send you an updated patch later this evening. I don't know anything 
> about config files though, so that'll have to wait a bit... The module 
> parameter will be added though.

The Kconfig language is pretty easy. But don't worry about it if you
don't have the time to look into it: I'll add a patch on top of yours
implementing my idea, no problem.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c