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?
In general several attempts have been made earlier to
finally make soc-camera originated subdevice drivers
soc-camera-independent. This hasn't been finalised due to a lack of a real
life use-case. As soon as one appears, finalising that work shouldn't be
too difficult.
For example, there's an adv7180 decoder on the Lager board, and its driver can
be successfully used with a soc_camera (rcar_vin) as well as
a PCI STA2X11 Video Input device. However, if the adv7180 needed a platform
data, there would be a conflict, since soc_camera uses
a different method of passing platform data to the subdevice driver.
I don't think that making adv7612 subdevice "SOC"-specific would be the way to
go here since it may lead us to code duplication for non-soc video devices
later.
+
+struct adv761x_color_format {
+ enum v4l2_mbus_pixelcode code;
+ enum v4l2_colorspace colorspace;
+};
+
+/* Supported color format list */
+static const struct adv761x_color_format adv761x_cfmts[] = {
+ {
+ .code = V4L2_MBUS_FMT_RGB888_1X24,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+};
+
+/* ADV761X descriptor structure */
+struct adv761x_state {
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct v4l2_ctrl_handler ctrl_hdl;
+
+ u8 edid[256];
+ unsigned edid_blocks;
+ const struct adv761x_color_format *cfmt;
+ u32 width;
+ u32 height;
+ enum v4l2_field scanmode;
+
+ struct workqueue_struct *work_queue;
+ struct delayed_work enable_hotplug;
+
+ /* I2C clients */
+ struct i2c_client *i2c_cec;
+ struct i2c_client *i2c_infoframe;
+ struct i2c_client *i2c_dpll;
+ struct i2c_client *i2c_repeater;
+ struct i2c_client *i2c_edid;
+ struct i2c_client *i2c_hdmi;
+ struct i2c_client *i2c_cp;
+};
+
+static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
+{
+ return &container_of(ctrl->handler, struct adv761x_state,
ctrl_hdl)->sd;
+}
+
+static inline struct adv761x_state *to_state(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct adv761x_state, sd);
+}
+
+/* I2C I/O operations */
+static s32 adv_smbus_read_byte_data(struct i2c_client *client, u8
command)
+{
+ s32 ret, i;
+
+ for (i = 0; i < 3; i++) {
+ ret = i2c_smbus_read_byte_data(client, command);
Uhm, why do you need to do this 3 times?... I see adv7842 does that
too...
but e.g. adv7604 dies this only for writing and not for reading...
Just thought it would be safe to retry in case of a failure.
Other drivers seem to retry I2C I/O as well. This is probably related to
the
possible cable issues. Not sure. Anyways, retrying doesn't seem to hurt,
does
it?
It does, because it means there's something we don't understand. IMHO it
should either work from the first time, or there should be an error, that
we understand with a following error processing, that _might_ include
re-trying. Re-trying just in case isn't good. Especially if no error has
been observed.
I have observed very rare errors when reading HDMI status. Just a couple of
times during this week. I'm not sure of the nature of those errors. Just
thought it would be OK to retry since other decoder drivers do so as well.
Ok, understand. As I said, I personally don't like that, but, I think,
Laurent will have a final word on this.
OK, thanks.
[snip]
+/* Power management operations */
+#ifdef CONFIG_PM_SLEEP
+static int adv761x_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+ /* Power off */
+ return io_write(sd, 0x0c, 0x62);
+}
+
+static int adv761x_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+ /* Initializes ADV761X to its default values */
+ return adv761x_core_init(sd);
What if a system was suspended during capture? Is this enough to resume
it
automatically?
Is it needed to auto-resume capture?
IIUC, adv7810 doesn't seem to do that in its resume callback.
Since not many decoder drivers seem to implement PM, we probably could
drop it
altogether for now (in case capture auto-resume is needed).
Yep, removing dummy PM is good, I think.
OK, I can remove it.
I wouldn't call it dummy, though, since it does change the power state of the
chip.
The question of whether it needs to auto-resume the capture is still not clear
to me.
I'm not 100% certain either, but at least what I am certain about, is that
you need to restore the configuration before the suspend instead of just
initialising to defaults. Or would the configuration be preserved. I mean
in a sequence like
set controls
set EDID
configure formats
suspend
resume
start capture
Would the controls, formats and EDID be preserved or lost?
OK, understood, thanks.
I guess PM could be dropped for now.
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.
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)
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
