On 11/27/2013 01:32 PM, Valentine wrote: > 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 GPIO driver should set up the GPIO pin as a interrupt pin when the interrupt is requested. We should not have to add hacks to adv7604 driver to workaround a broken GPIO driver. > >> >>> >>>>> 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. Use a threaded interrupt instead of workqueue + disable_irq_nosync, that should work fine. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
