Re: [PATCH V2] media: i2c: Add ADV761X support

Lars-Peter Clausen
2013-11-27T09:58:58Z

[...]
>> I had to implement the IRQ handler since the soc_camera model does not use
>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
>> interrupt routed to a GPIO pin.
>> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.
>
> For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead
> a register has to be read to figure out which device actually produced the irq. So I
> want to keep the interrupt_service_routine(). However, adding a gpio field to the
> platform_data that, if set, will tell the driver to request an irq and setup a
> workqueue that calls interrupt_service_routine() would be fine with me. That will
> benefit a lot of people since using gpios is much more common.

I'll look into adding that since I'm using a GPIO for the interrupt on my
platform as well.

>
>> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
>> seem to handle them correctly.
>> According to the docs:
>> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller
>> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."
>>
>> However, the interrupt_service_routine doesn't account for that.
>> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
>> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
>> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
>> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.
>
> I'm not sure if there is much that can be done about this. The code reads the
> interrupt status, then clears the interrupts right after. There is always a
> race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter
> has a better idea?
>

As far as I understand it the interrupts lines are level triggered and will
stay asserted as long as there are unmasked, non-acked IRQS. So the
interrupt handler should be re-entered if there are still pending
interrupts. Is it possible that you setup a edge triggered interrupt, in
that case the handler wouldn't be reentered if the signal stays asserted?

But that's just my understanding from the manual, I'll have to verify
whether the hardware does indeed work like that.

- Lars