Jean Delvare wrote:
> Hi Troy,
>
> On Fri, 25 Apr 2008 09:58:13 -0700, Troy Kisky wrote:
>> If wait_for_completion_interruptible_timeout exits due
>> to a signal, the i2c bus was locking up.
>
> What kind of signal (coming from where) are you talking about?
With the user space i2c interface, a ^c was
locking the bus.
>
> If you don't want to be interrupted, why don't you simply use
> wait_for_completion_timeout() instead of
> wait_for_completion_interruptible_timeout()?
I didn't make that choice. Perhaps wait_for_completion_timeout()
would be better. I just preferred fixing the lockup if a signal
happened. It seemed like a safer change to me. Can wait_for_completion_timeout()
return for any reason other the successful completion or timeout? Will
an explicit kill of the process return? Do you want it changed to
use wait_for_completion_timeout()?
>> +static inline void terminate_read(struct davinci_i2c_dev *dev)
>> +{
>> + if (dev->buf_len)
>> + dev->buf_len--;
>> + if (dev->buf_len) {
>
> Please explain (in a comment) what you are doing here. Can't you just
> test for (dev->buf_len > 1)?
Or maybe no test, just always set the nak bit and throw away the data??
>
>> + u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> + w |= DAVINCI_I2C_MDR_NACK;
>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>> + }
>> + /* Throw away data */
>> + davinci_i2c_read_reg(dev, DAVINCI_I2C_DRR_REG);
>> + if (!dev->terminate)
>> + dev_err(dev->dev, "RDR IRQ while no data requested\n");
>> +}
>> +static inline void terminate_write(struct davinci_i2c_dev *dev)
>> +{
>> + u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> + w |= DAVINCI_I2C_MDR_RM|DAVINCI_I2C_MDR_STP;
>
> Coding-style: spaces around |.
OK
>
>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>> +
>> + if (!dev->terminate)
>> + dev_err(dev->dev, "TDR IRQ while no data to send\n");
>> +}
>
> I don't see the point of inlining these functions explicitly... They
> are only called in an unlikely event so this is certainly not
> performance-critical.
OK
>> @@ -419,9 +460,10 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
>> *dev_id)
>> davinci_i2c_write_reg(dev,
>> DAVINCI_I2C_IMR_REG,
>> w);
>> - } else
>> - dev_err(dev->dev, "TDR IRQ while no data to "
>> - "send\n");
>> + } else {
>> + /* signal can terminate transfer */
>> + terminate_write(dev);
>> + }
>> break;
>>
>> case DAVINCI_I2C_IVR_SCD:
>
> Apparently you are using dev->buf_len = 0 and dev->buf = NULL to notify
> particular conditions accross the driver? This should be clearly
> documented, otherwise other developers are likely to break the driver
> while working on it.
>
Agreed.
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
