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

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

On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> Hi Valentine,
>
> Did you ever look at this adv7611 driver:
>
> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12

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.

>
> It adds adv761x support to the adv7604 in a pretty clean way.
>
> Thinking it over I prefer to use that code (although you will have to
> add the soc-camera hack for the time being) over your driver.
>
> Others need adv7611 support as well, but with all the dv_timings etc. features
> that are removed in your driver. So I am thinking that it is easier to merge
> the xilinx version and add whatever you need on top of that.
>

Thanks,
Val.

> Regards,
>
> 	Hans
>
> On 11/20/13 13:24, Valentine wrote:
>> On 11/20/2013 03:19 PM, Hans Verkuil wrote:
>>> Hi Valentine,
>>
>> Hi Hans,
>>
>>>
>>> On 11/20/13 11:14, Valentine wrote:
>>>> On 11/19/2013 01:50 PM, Hans Verkuil wrote:
>>>>> Hi Valentine,
>>>>
>>>> Hi Hans,
>>>> thanks for your review.
>>>>
>>>>>
>>>>> I don't entirely understand how you use this driver with soc-camera.
>>>>> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
>>>>> act on it. Did you hack soc-camera to do this?
>>>>
>>>> I did not. The format is queried before reading the frame by the user-space.
>>>> I'm not sure if there's some kind of generic interface to notify the camera
>>>> layer about format change events. Different subdevices use different FMT_CHANGE
>>>> defines for that. I've implemented the format change notifier based on the adv7604
>>>> in hope that it may be useful later.
>>>
>>> Yes, I need to generalize the FMT_CHANGE event.
>>>
>>> But what happens if you are streaming and the HDMI connector is unplugged?
>>> Or plugged back in again, possibly with a larger resolution? I'm not sure
>>> if the soc_camera driver supports such scenarios.
>>
>> It doesn't. Currently it's up to the UI to poll the format and do the necessary changes.
>> Otherwise the picture will be incorrect.
>>
>>>
>>>>
>>>>>
>>>>> The way it stands I would prefer to see a version of the driver without
>>>>> soc-camera support. I wouldn't have a problem merging that as this driver
>>>>> is a good base for further development.
>>>>
>>>> I've tried to implement the driver base good enough to work with both SoC
>>>> and non-SoC cameras since I don't think having 2 separate drivers for
>>>> different camera models is a good idea.
>>>>
>>>> The problem is that I'm using it with R-Car VIN SoC camera driver and don't
>>>> have any other h/w. Having a platform data quirk for SoC camera in
>>>> the subdevice driver seemed simple and clean enough.
>>>
>>> I hate it, but it isn't something you can do anything about. So it will have
>>> to do for now.
>>>
>>>> Hacking SoC camera to make it support both generic and SoC cam subdevices
>>>> doesn't seem that straightforward to me.
>>>
>>> Guennadi, what is the status of this? I'm getting really tired of soc-camera
>>> infecting sub-devices. Subdev drivers should be independent of any bridge
>>> driver using them, but soc-camera keeps breaking that. It's driving me nuts.
>>>
>>> I'll be honest, it's getting to the point that I want to just NACK any
>>> future subdev drivers that depend on soc-camera, just to force a solution.
>>> There is no technical reason for this dependency, it just takes some time
>>> to fix soc-camera.
>>>
>>>> Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
>>>> involves a lot of regression testing with other R-Car boards that use different
>>>> subdevices with VIN.
>>>>
>>>> What would you suggest?
>>>
>>> Let's leave it as-is for now :-(
>>>
>>> I'm not happy, but as I said, it's not your fault.
>>
>> OK, thanks.
>> Once a better solution is available we can remove the quirk.
>>
>>>
>>> Regards,
>>>
>>>      Hans
>>
>> Thanks,
>> Val.
>>
>>>
>>>>
>>>>>
>>>>> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
>>>>> control. It's easy to implement and that way apps can be notified when
>>>>> the hotplug changes value.
>>>>
>>>> OK, thanks.
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>>       Hans
>>>>
>>>> Thanks,
>>>> Val.
>>>>
>>>>>
>>>>> On 11/15/13 13:54, Valentine Barshak wrote:
>>>>>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>>>>>> support. Only one HDMI port is supported on ADV7612.
>>>>>>
>>>>>> The code is based on the ADV7604 driver, and ADV7612 patch by
>>>>>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>>>>>
>>>>>> Changes in version 2:
>>>>>> * Used platform data for I2C addresses setup. The driver
>>>>>>      should work with both SoC and non-SoC camera models.
>>>>>> * Dropped unnecessary code and unsupported callbacks.
>>>>>> * Implemented IRQ handling for format change detection.
>>>>>>
>>>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>>
>>
>