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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Wednesday 27 November 2013 13:14:41 Hans Verkuil wrote:
> On 11/27/13 12:39, Laurent Pinchart wrote:
> > On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> >> On 11/26/2013 10:28 PM, Valentine wrote:
> >>> On 11/20/2013 07:53 PM, Valentine wrote:
> >>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> >>>>> Hi Valentine,
> >>> 
> >>> Hi Hans,
> >>> 
> >>>>> Did you ever look at this adv7611 driver:
> >>>>> 
> >>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e
> >>>>> 4afa42af2daa12
> >>>> 
> >>>> No, I missed that one somehow, although I did search for the
> >>>> adv7611/7612 before implementing this one. I'm going to look closer at
> >>>> the patch and test it.
> >>> 
> >>> I've tried the patch and I doubt that it was ever tested on adv7611.
> >>> I haven't been able to make it work so far. Here's the description of
> >>> some of the issues I've encountered.
> >>> 
> >>> The patch does not apply cleanly so I had to make small adjustments just
> >>> to make it apply without changing the functionality.
> >>> 
> >>> First of all the driver (adv7604_dummy_client function) does not set
> >>> default I2C slave addresses in the I/O map in case they are not set in
> >>> the platform data.
> >>> This is not needed for 7604, since the default addresses are already set
> >>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
> >>> after power up, and we always have to set it manually.
> >> 
> >> So, the platform data for the 7611/2 should always give i2c addresses.
> >> That seems reasonable.
> >> 
> >>> 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.
> > 
> > Where is that register located ? Shouldn't it be modeled as an interrupt
> > controller ?
> 
> It's a PCIe interrupt whose handler needs to read several FPGA registers
> in order to figure out which interrupt was actually triggered. I don't
> know enough about interrupt controller to understand whether it can be
> modeled as a 'standard' interrupt.

I've replied to this separately.

> >> 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.
> > 
> > We should use the i2c_board_info.irq field for that, not a field in the
> > platform data structure. The IRQ line could be hooked up to a non-GPIO
> > IRQ.
> 
> Yes, of course. Although the adv7604 has two interrupt lines, so if you
> would want to use the second, then that would still have to be specified
> through the platform data.

I believe we should then extend the I2C interrupt support. The reason for 
doing so is that we want to use the interrupt DT bindings for DT platforms, 
and those are handled by the I2C core.

> >>> 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?
> >> 
> >> What can be improved, though, is to clear not just the interrupts that
> >> were read, but all the interrupts that are unmasked. You are right, you
> >> couldloose an interrupt that way.
> > 
> > Wouldn't level-trigerred interrupts fix the issue ?
> 
> See my earlier reply.

-- 
Regards,

Laurent Pinchart