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

[Valentine <valentine.barshak@cogentembedded.com>]

On 11/27/2013 04:14 PM, Hans Verkuil wrote:
> Hi Laurent,
>
> On 11/27/13 12:39, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> 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/610b9d5de22ae7c0047c65a07e4a
>>>>>> fa42af2daa12
>>>>>
>>>>> 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.
>
>>
>>> 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.

In this case the GPIO should be configured as interrupt source in the platform
code. But this doesn't seem to work with R-Car GPIO since it is initialized
later, and the gpio_to_irq function returns an error.
The simplest way seemed to use a GPIO number in the platform data
to have the adv driver configure the pin and request the IRQ.
I'm not sure how to easily defer I2C board info IRQ setup (and camera/subdevice probing)
until GPIO driver is ready.

>
>>
>>>> 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 could
>>> loose an interrupt that way.
>>
>> Wouldn't level-trigerred interrupts fix the issue ?

In this case we need to disable the IRQ line in the IRQ handler and re-enable it in the workqueue.
(we can't call the interrupt service routine from the interrupt context.)

This however didn't seem to work with R-Car GPIO.
Calling disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler doesn't seem
to disable it for some reason.

Also if the isr is called by the upper level camera driver, we assume that it needs
special handling (disabling/enabling) for the ADV76xx interrupt although it uses the API
interrupt_service_routine callback. Not a big deal, but still doesn't look pretty to me.

>
> See my earlier reply.
>
> Regards,
>
> 	Hans
>

Thanks,
Val.