On Thu, Oct 09, 2014 at 07:35:07PM +0200, Christophe Henri RICARD wrote: > Hi Jason, > > > If your chip is sane like other TPMs the IRQ pin will *only* be > > asserted while there is data pending in the out command FIFO, > > reading the FIFO *should naturally clear the IRQ *and the acking > > process may be entirely unnecessary and can be removed. > I believe this assessment is wrong according to existing > specifications and current implementation and I will explain you > why: It does sound like it is not how the ST33 chip operates. > Our TPM is managing the TPM TIS registers Interrupt Enable and > Interrupt Status. The TIS register mapping was intended for environments where register access is not expensive, and can be done from an ISR. I2C is not such an environment, which is why other vendors are not using such a strict mapping of the TIS registers. That doesn't really change anything, it just means the driver has to do more I2C transactions to manage this extra chip state. > According to which one is triggered, the wait queue to wake up is > different it could be chip->vendor.read_queue or > chip->vendor.int_queue. These different queues are not needed, the driver knows what it is waiting for when it enables the interrupt handler, and can manipulate the interrupt mask if necessary for special cases. Multiple queues make more sense when the ISR can cheaply read the status register, which is not true for I2C. > I would point out as well the int_queue initialization in the > i2c_nuvoton_probe which is never used in the tpm_i2c_nuvoton.c nor > in any tpm core file. Chip structure members that are not used in the core code are hold overs from the ancient core design. Drivers ideally should not use them, favoring their own structures in their driver private allocation. Someday that will get cleaned up. Don't get confused that int_queue and read_queue are in the global chip, they have no special meaning, modern drivers should not use them at all. > However, I am not in favor to change to non-threaded irq unless I > have a clear and convincing argument to do so. The need for the udelay should be all the convincing required. The reason for the udelay clearly shows the driver has a synchronization problem - and sleeping to solve synchronization problem is rarely correct. This is especially true since I've already explained how to design so everything is synchronous and solve the issue. Again: In TPM the interrupt is not delivering an asynchronous notification, everything is synchronous to the driver state, ISRs happen only to indicate completion of driver initiated actions. This is why the synchronous flow I suggested is much safer and better, the driver just can't get the situation where the IRQ cannot safely run because the main driver thread has changed the TPM state. To summarize, the flow for an interrupt wait becomes very simple, ie to wait for command ready: - Do write to clear all interrupts - do read on command ready - if not ready then write to enable only the command ready occured interrupt - enable_irq - wait for irq w/ timer - do read on command ready - if not ready and no timeout, do write to clear all interrupts, enable_irq, loop. - if not ready and timeout, disable irq, return error All sleepables follow the same synchronous pattern. This makes the driver compeltely single threaded so no analysis for thread problems is need. No locking primitives are needed. The inter-locking problem with request_locality goes away. Jason -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
