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. 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.
[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?
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?
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?
> > > > > 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.
Thanks
Guennadi
---
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
