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.
--
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
