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. > > 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. > > 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. > > > > > Thanks > > Guennadi > > --- > > Guennadi Liakhovetski, Ph.D. > > Freelance Open-Source Software Developer > > http://www.open-technology.de/ > > > > Thanks, > Val. > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
