Oliver Endriss wrote:
>
>> Protect the access to the IER/ISR register of the SAA7146 by the device
>> spinlock.
>>
>
> Imho it is not necessary to protect write operations to the ISR because
> it is a single write-only operation.
>
You are right.
> SAA7146_IER_DISABLE/SAA7146_IER_ENABLE must be protected by a spinlock
> because it is a read-modify-write operation.
>
> So your patch could be replaced by the attached one.
>
> What do you think?
>
>
It exist some pieces of code, where the spinlock is already locked and
where the macros are used. It exist a second spinlock within the SAA7146
device structure. Currently, it is unused. This one can be used.
- Hartmut
diff -r a80a23f896b2 linux/include/media/saa7146.h
--- a/linux/include/media/saa7146.h Sun Oct 29 11:12:27 2006 -0300
+++ b/linux/include/media/saa7146.h Tue Oct 31 18:12:58 2006 +0100
@@ -54,10 +54,20 @@ extern unsigned int saa7146_debug;
#define DEB_INT(x) if (0!=(DEBUG_VARIABLE&0x20)) { DEBUG_PROLOG; printk x; } /* interrupt debug messages */
#define DEB_CAP(x) if (0!=(DEBUG_VARIABLE&0x40)) { DEBUG_PROLOG; printk x; } /* capture debug messages */
-#define SAA7146_IER_DISABLE(x,y) \
- saa7146_write(x, IER, saa7146_read(x, IER) & ~(y));
+#define SAA7146_IER_DISABLE(x,y) \
+ do { \
+ unsigned int flags; \
+ spin_lock_irqsave(&x->int_slock, flags); \
+ saa7146_write(x, IER, saa7146_read(x, IER) & ~(y)); \
+ spin_unlock_irqrestore(&x->int_slock, flags); \
+ } while(0)
#define SAA7146_IER_ENABLE(x,y) \
- saa7146_write(x, IER, saa7146_read(x, IER) | (y));
+ do { \
+ unsigned int flags; \
+ spin_lock_irqsave(&x->int_slock, flags); \
+ saa7146_write(x, IER, saa7146_read(x, IER) | (y)); \
+ spin_unlock_irqrestore(&x->int_slock, flags); \
+ } while(0)
#define SAA7146_ISR_CLEAR(x,y) \
saa7146_write(x, ISR, (y));
diff -r a80a23f896b2 linux/drivers/media/common/saa7146_i2c.c
--- a/linux/drivers/media/common/saa7146_i2c.c Sun Oct 29 11:12:27 2006 -0300
+++ b/linux/drivers/media/common/saa7146_i2c.c Tue Oct 31 18:16:28 2006 +0100
@@ -190,13 +190,24 @@ static int saa7146_i2c_writeout(struct s
saa7146_write(dev, I2C_TRANSFER, *dword);
dev->i2c_op = 1;
+ SAA7146_ISR_CLEAR(dev, MASK_16|MASK_17);
SAA7146_IER_ENABLE(dev, MASK_16|MASK_17);
saa7146_write(dev, MC2, (MASK_00 | MASK_16));
- wait_event_interruptible(dev->i2c_wq, dev->i2c_op == 0);
- if (signal_pending (current)) {
- /* a signal arrived */
- return -ERESTARTSYS;
+ timeout = HZ/100 + 1; /* 10ms */
+ timeout = wait_event_interruptible_timeout(dev->i2c_wq, dev->i2c_op == 0, timeout);
+ if (timeout == -ERESTARTSYS || dev->i2c_op) {
+ SAA7146_IER_DISABLE(dev, MASK_16|MASK_17);
+ SAA7146_ISR_CLEAR(dev, MASK_16|MASK_17);
+ if (timeout == -ERESTARTSYS) {
+ /* a signal arrived */
+ return -ERESTARTSYS;
+ }
+ /* this is normal when probing the bus
+ * (no answer from nonexisistant device...)
+ */
+ DEB_I2C(("saa7146_i2c_writeout: timed out waiting for end of xfer\n"));
+ return -EIO;
}
status = saa7146_read(dev, I2C_STATUS);
} else {
_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
