On 09/25/2013 10:36 PM, Valentine wrote: > 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. Guennadi, I still think this really, really sucks. And it would be great if soc-camera would just be able to use regular subdev drivers that no longer need to use anything from soc-camera. The dependencies have weakened a lot in recent times, so if a final push could be made to remove them completely, then that would be very helpful. > As for where you >> put it - I don't care that much either. > > OK, thanks. Please put it in media/i2c. There where all the other receivers/transmitters live. > >> >>>> 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. The adv7604 does not control the hotplug pin, so that's why the notify is there: only the bridge driver knows how it is hooked up. The adv7842 on the other hand controls the hotplug pin directly, so that one has no hotplug notify. So it depends on the adv761x hardware whether or not a notify is needed. How can you use this with soc-camera, BTW? soc-camera has no notifier, so cannot control the hotplug pin for you. And if you cannot control the hotplug pin, then you cannot set the EDID. And if you cannot set the EDID you cannot receive HDMI. Which makes it pretty pointless. > >> >>>> 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. All other i2c receivers/transmitters use include/media. Let's keep it like that for now for consistency within the media drivers. We might move all those headers to include/linux/platform_data at some point in the future. Regards, Hans >>>>> >>>>> 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. > > -- > 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 > -- 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
