Re: [PATCH 1/3] media: i2c: Add ADV761X support

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

On 09/25/2013 10:33 PM, Guennadi Liakhovetski wrote:
> Hi Valentine,
>
> On Wed, 25 Sep 2013, Valentine wrote:
>
>> On 09/25/2013 08:31 PM, Guennadi Liakhovetski wrote:
>>> On Wed, 25 Sep 2013, Valentine wrote:
>>>
>>>> On 09/25/2013 06:08 PM, Guennadi Liakhovetski wrote:
>>>
>>> [snip]
>>>
>>>>>>> Using module parameters has a disadvantage, that all instances of
>>>>>>> this
>>>>>>> driver will get the same values, and it is quite possible to have
>>>>>>> several
>>>>>>> HDMI receivers in a system, I believe? Wouldn't it be better to pass
>>>>>>> these
>>>>>>> addresses from the platform data / DT?
>>>>>>
>>>>>> Yes, that doesn't look quite right, but I couldn't find a better
>>>>>> solution
>>>>>> ATM.
>>>>>>
>>>>>> The problem is that this subdevice is going to be used by a soc_camera
>>>>>> driver,
>>>>>> and the soc_camera interface uses its own platform data for all I2C
>>>>>> subdevices.
>>>>>> Thus, if I pass the I2C addresses in client->dev.platform_data here
>>>>>> (as in
>>>>>> adv7604), it's doing to be overwritten by the soc_camera_i2c_init()
>>>>>> function
>>>>>> with a soc_camera_subdev_desc data.
>>>>>>
>>>>>> Not sure what the correct solution should be. Any suggestions are
>>>>>> greatly
>>>>>> appreciated.
>>>>>
>>>>> You don't think, that soc-camera makes it impossible to pass
>>>>> device-specific platform data to subdevices, right? For an example have
>>>>> a
>>>>> look e.g. at mt9v022 and arch/arm/mach-pxa/pcm990-baseboard.c or at
>>>>> mt9t111 and arch/arm/mach-shmobile/board-armadillo800eva.c.
>>>>
>>>> Yes, but in this case adv761x.c has to be a soc_camera i2c subdevice
>>>> driver.
>>>> Which means it will get its platform data from ssdd->drv_priv like mt9v022
>>>> AOT
>>>> client->dev.platform_data, like adv7604 does.
>>>>
>>>> What if a non-soc_camera needs to use the adv7612 decoder?
>>>> IMHO, Looks like there's a conflict in the soc/non-soc camera driver
>>>> subdevice
>>>> usage.
>>>
>>> I don't think, that just using soc-camera platform data struct will make
>>> it impossible for non-soc-camera bridge / video input drivers to use this
>>> subdevice driver.
>>
>> This is the only problem I can see at the moment.
>> Do you see any other issues?
>>
>> As I've said earlier the driver is based much on the adv7604 which is used for
>> non-soc cameras.
>> I guess, implementing dv_timings and ISR for adv7612 will make it look even
>> closer.
>> Other subdevice drivers (like 7180) seem to work with both soc/non-soc
>> cameras.
>> Are you proposing to make it soc-cam-specific, move it to i2c/soc_camera/ and
>> deal with a non-soc variant later if needed?
>
> I wouldn't call it soc-camera specific. It would just include an
> soc-camera header and use one soc-camera struct. It wouldn't even require
> loading the soc-camera core module, let alone using soc-camera driver
> binding schemes. So, it would be a very mild dependency. As for where you
> put it - I don't care that much either.

OK, thanks.

>
>>> In fact, looking a bit more at the driver, I've got a couple more
>>> questions.
>>>
>>> 1. Hotplug handling: you've got comments like "Pull down the hotplug pin,"
>>> but I don't see any code that would actually pull the pin? Am I missing it
>>> or is it permanently low?
>>
>> It is supposed to be done by the camera driver that receives the hotplug
>> notification.
>
> Hm, seems strange to me - that pin belongs to the HDMI interface AFAICS
> and the camera host / bridge driver knows nothing about HDMI... E.g.
> adv7842_delayed_work_enable_hotplug(), IIUC, does enable the hotplug
> detection pin itself. The adv7604 handling looks suspicious to me...
>
> BTW, the free ADV7612 "datasheet" with no register information and just a
> general description does mention hotplug control pins, so, I really think
> it should be a task of the HDMI decoder driver to control those.

IIUC, these pins are supposed to be controlled by the camera or SOC-GPIO.

>
>>> 2. You're using an own work queue just to queue a work to notify users
>>> about the event. Wouldn't it suffice to just use the scheduler work queue?
>>
>> It probably would. I just didn't want to deviate much from the code that is
>> already there (adv7604/adv7842/ad9389b/adv7511/cx25840).
>>
>>>
>>>>>>>> diff --git a/include/media/adv761x.h b/include/media/adv761x.h
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..e6e6aea
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/include/media/adv761x.h
>>>>>>>> @@ -0,0 +1,28 @@
>>>>>>>> +/*
>>>>>>>> + * adv761x Analog Devices ADV761X HDMI receiver driver
>>>>>>>> + *
>>>>>>>> + * Copyright (C) 2013 Cogent Embedded, Inc.
>>>>>>>> + * Copyright (C) 2013 Renesas Electronics Corporation
>>>>>>>> + *
>>>>>>>> + * This program is free software; you may redistribute it and/or
>>>>>>>> modify
>>>>>>>> + * it under the terms of the GNU General Public License as
>>>>>>>> published
>>>>>>>> by
>>>>>>>> + * the Free Software Foundation; version 2 of the License.
>>>>>>>> + *
>>>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>>>>>>>> KIND,
>>>>>>>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
>>>>>>>> WARRANTIES OF
>>>>>>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>>>>>>>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>>>>>>>> HOLDERS
>>>>>>>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
>>>>>>>> IN AN
>>>>>>>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
>>>>>>>> IN
>>>>>>>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>>>>>> THE
>>>>>>>> + * SOFTWARE.
>>>>>>>> + *
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#ifndef _ADV761X_H_
>>>>>>>> +#define _ADV761X_H_
>>>>>>>> +
>>>>>>>> +/* notify events */
>>>>>>>> +#define ADV761X_HOTPLUG		1
>>>>>>>
>>>>>>> Is this header needed at all and - if so - does it have to be
>>>>>>> exported
>>>>>>> under include/ or would it be enough to put it under drivers/media/?
>>>>>>
>>>>>> Well, the ADV761X_HOTPLUG is supposed to be used by a camera driver
>>>>>> for
>>>>>> hotplug notification events (as in adv7604). If we find a way to use
>>>>>> platform
>>>>>> data with soc_camera, it should go into this header as well.
>>>>>
>>>>> As well or instead? Do you really need to export ADV761X_HOTPLUG? And
>>>>> for
>>>>> platform data it's now preferable to use the
>>>>> include/linux/platform_data/
>>>>> directory, I think.
>>>>
>>>> As well.
>>>> This code is based on the ADV7604 driver. The define is for other drivers
>>>> to
>>>> use (the ones that need to be notified of the EDID change, for example).
>>>> As it
>>>> is not used with rcar_vin, I have no problem with dropping the EDID
>>>> handling
>>>> altogether.
>>>
>>> Uhm, so, that code is untested? Then yes, personally, I'd rather drop it
>>> for now.
>>
>> OK.
>> BTW, the EDID read/write code *is* tested. It is just not used for the
>> soc_camera.
>> (It is impossible to use it for a soc_camera, as it doesn't support subdev
>> user-space API)
>
> Ok, if it's been tested, it's good to keep it.

OK, thanks.

>
>>
>>>
>>> Thanks
>>> Guennadi
>>> ---
>>> Guennadi Liakhovetski, Ph.D.
>>> Freelance Open-Source Software Developer
>>> http://www.open-technology.de/
>>>

Thanks,
Val.