Hi Valentine,
On Wed, 25 Sep 2013, Valentine wrote:
> On 09/24/2013 07:54 PM, Guennadi Liakhovetski wrote:
> > Hi Valentine,
> >
>
> Hi Guennadi,
>
> > On Tue, 24 Sep 2013, Valentine Barshak wrote:
> >
> > > This adds ADV7611/ADV7612 Dual Port Xpressview
> > > 225 MHz HDMI Receiver support.
> > >
> > > The code is based on the ADV7604 driver, and ADV7612 patch
> > > by Shinobu Uehara <shinobu.uehara.xc@xxxxxxxxxxx>
> > >
> > > Signed-off-by: Valentine Barshak <valentine.barshak@xxxxxxxxxxxxxxxxxx>
> >
> > IIRC, Laurent is reviewing all new media I2C drivers, I added him to cc. A
> > couple of minor comments from me too, while at it.
>
> Thanks!
>
> >
> > > ---
> > > drivers/media/i2c/Kconfig | 11 +
> > > drivers/media/i2c/Makefile | 1 +
> > > drivers/media/i2c/adv761x.c | 1060
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > include/media/adv761x.h | 28 ++
> > > 4 files changed, 1100 insertions(+)
> > > create mode 100644 drivers/media/i2c/adv761x.c
> > > create mode 100644 include/media/adv761x.h
> > >
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index d18be19..8eede00 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -206,6 +206,17 @@ config VIDEO_ADV7604
> > > To compile this driver as a module, choose M here: the
> > > module will be called adv7604.
> > >
> > > +config VIDEO_ADV761X
> > > + tristate "Analog Devices ADV761X decoder"
> > > + depends on VIDEO_V4L2 && I2C
> > > + ---help---
> > > + Support for the Analog Devices ADV7611/ADV7612 video decoder.
> > > +
> > > + This is an Analog Devices Dual Port Xpressview HDMI Receiver.
> >
> > Are you sure this driver can handle all adv761x devices? One of the
> > differences even between 7611 and 7612 is, that only 7612 is dual-port,
> > 7611 is single-port. What about 7619?
>
> It doesn't claim to support 7619. The help message says that only 7611/7612
> are supported. The driver doesn't support port B of the 7612
> as it is not used on the h/w I have -- Just like the 7604 driver, this one is
> based on. This is a preliminary ADV761x support, and more functionality could
> be added later if needed (including 7619).
What I meant, is that the name adv761x implicitly includes any device in
the range adv7610-adv7619, which isn't what you support. Maybe it would be
better to call the driver adv7611 (or adv7612) and just explain in
comments, that you also support the other chip - if you really do. You
really can handle single- and double-port cases as well as other
differences between them?
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called adv761x.
> > > +
> > > config VIDEO_ADV7842
> > > tristate "Analog Devices ADV7842 decoder"
> > > depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > index 9f462df..393eb0c 100644
> > > --- a/drivers/media/i2c/Makefile
> > > +++ b/drivers/media/i2c/Makefile
> > > @@ -26,6 +26,7 @@ obj-$(CONFIG_VIDEO_ADV7183) += adv7183.o
> > > obj-$(CONFIG_VIDEO_ADV7343) += adv7343.o
> > > obj-$(CONFIG_VIDEO_ADV7393) += adv7393.o
> > > obj-$(CONFIG_VIDEO_ADV7604) += adv7604.o
> > > +obj-$(CONFIG_VIDEO_ADV761X) += adv761x.o
> > > obj-$(CONFIG_VIDEO_ADV7842) += adv7842.o
> > > obj-$(CONFIG_VIDEO_AD9389B) += ad9389b.o
> > > obj-$(CONFIG_VIDEO_ADV7511) += adv7511.o
> > > diff --git a/drivers/media/i2c/adv761x.c b/drivers/media/i2c/adv761x.c
> > > new file mode 100644
> > > index 0000000..bc3dfc3
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/adv761x.c
> > > @@ -0,0 +1,1060 @@
> > > +/*
> > > + * 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 can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > > + */
> > > +
> > > +#include <linux/errno.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/videodev2.h>
> > > +#include <media/adv761x.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-device.h>
> > > +#include <media/v4l2-ioctl.h>
> > > +
> > > +#define ADV761X_DRIVER_NAME "adv761x"
> > > +
> > > +/* VERT_FILTER_LOCKED and DE_REGEN_FILTER_LOCKED flags */
> > > +#define ADV761X_HDMI_F_LOCKED(v) (((v) & 0xa0) == 0xa0)
> > > +
> > > +/* Maximum supported resolution */
> > > +#define ADV761X_MAX_WIDTH 1920
> > > +#define ADV761X_MAX_HEIGHT 1080
> > > +
> > > +static int debug;
> > > +module_param(debug, int, 0644);
> > > +MODULE_PARM_DESC(debug, "debug level (0-2)");
> > > +
> > > +/* I2C map addresses */
> > > +static u8 i2c_cec = 0x40;
> > > +module_param(i2c_cec, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_cec, "CEC map 7-bit I2C address (default: 0x40)");
> > > +
> > > +static u8 i2c_inf = 0x3e;
> > > +module_param(i2c_inf, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_inf, "InfoFrame map 7-bit I2C address (default:
> > > 0x3e)");
> > > +
> > > +static u8 i2c_dpll = 0x26;
> > > +module_param(i2c_dpll, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_dpll, "DPLL map 7-bit I2C address (default: 0x20)");
> > > +
> > > +static u8 i2c_rep = 0x32;
> > > +module_param(i2c_rep, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_rep, "Repeater map 7-bit I2C address (default:
> > > 0x32)");
> > > +
> > > +static u8 i2c_edid = 0x36;
> > > +module_param(i2c_edid, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_edid, "EDID map 7-bit I2C address (default: 0x36)");
> > > +
> > > +static u8 i2c_hdmi = 0x34;
> > > +module_param(i2c_hdmi, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_hdmi, "HDMI map 7-bit I2C address (default: 0x34)");
> > > +
> > > +static u8 i2c_cp = 0x22;
> > > +module_param(i2c_cp, byte, 0644);
> > > +MODULE_PARM_DESC(i2c_cp, "CP map 7-bit I2C address (default: 0x22)");
> >
> > 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.
> > > +
> > > +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.
> > > + if (ret >= 0)
> > > + return ret;
> > > + }
> > > +
> > > + v4l_err(client, "error reading addr:%02x reg:%02x\n",
> > > + client->addr, command);
> > > + return ret;
> > > +}
> > > +
> > > +static s32 adv_smbus_write_byte_data(struct i2c_client *client,
> > > + u8 command, u8 value)
> > > +{
> > > + s32 ret, i;
> > > +
> > > + for (i = 0; i < 3; i++) {
> > > + ret = i2c_smbus_write_byte_data(client, command, value);
> >
> > ditto
>
> Please see my comment above,
> thanks.
>
> >
> > > + if (!ret)
> > > + return 0;
> > > + }
> > > +
> > > + v4l_err(client, "error writing addr:%02x reg:%02x val:%02x\n",
> > > + client->addr, command, value);
> > > + return ret;
> > > +}
> >
> > [snip]
> >
> > > +static inline int edid_write_block(struct v4l2_subdev *sd,
> > > + unsigned len, const u8 *val)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > + struct adv761x_state *state = to_state(sd);
> > > + int ret = 0;
> > > + int i;
> > > +
> > > + v4l2_dbg(2, debug, sd, "writing EDID block (%d bytes)\n", len);
> > > +
> > > + v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
> > > +
> > > + /* Disable I2C access to internal EDID ram from DDC port */
> > > + rep_write(sd, 0x74, 0x0);
> > > +
> > > + for (i = 0; !ret && i < len; i += I2C_SMBUS_BLOCK_MAX)
> > > + ret = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
> > > + I2C_SMBUS_BLOCK_MAX, val + i);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* adv761x calculates the checksums and enables I2C access
> > > + * to internal EDID ram from DDC port.
> > > + */
> > > + rep_write(sd, 0x74, 0x01);
> > > +
> > > + for (i = 0; i < 1000; i++) {
> > > + if (rep_read(sd, 0x76) & 0x1) {
> > > + /* enable hotplug after 100 ms */
> > > + queue_delayed_work(state->work_queue,
> > > + &state->enable_hotplug, HZ / 10);
> > > + return 0;
> > > + }
> > > + schedule();
> >
> > This doesn't look pretty to me. Other drivers use at least an msleep(1)
> > here, which doesn't look particularly exciting, but at least it makes some
> > sense. Whereas a call to schedule() here doesn't really do much, I think.
>
> IMHO msleep(1) looks even less pretty. In fact we can't use msleep for less
> than 20mS delays.
> This is described in Documentation/timers/timers-howto.txt
> On the other hand, schedule() does the exact same thing the msleep(1) would.
> It preempts the current task and gives other tasks a chance to run, giving as
> a small delay before re-reading EDID status.
Ok, it looks wrong to me, but I don't have a better solution. Normally you
would be sleeping on something like a completion and an asynchronous event
would wake you up, but I don't see such an event here, do you?
> > > + }
> > > +
> > > + v4l_err(client, "error enabling edid\n");
> > > + return -EIO;
> > > +}
> >
> > [snip]
> >
> > > +static int adv761x_s_mbus_fmt(struct v4l2_subdev *sd,
> > > + struct v4l2_mbus_framefmt *mf)
> > > +{
> > > + struct adv761x_state *state = to_state(sd);
> > > + int i, ret;
> > > + u8 progressive;
> > > + u32 width;
> > > + u32 height;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(adv761x_cfmts); i++) {
> > > + if (mf->code == adv761x_cfmts[i].code) {
> > > + state->cfmt = &adv761x_cfmts[i];
> > > + break;
> > > + }
> > > + }
> > > + if (i >= ARRAY_SIZE(adv761x_cfmts))
> > > + return -EINVAL;
> > > +
> > > + /* Get video information */
> > > + ret = adv761x_get_vid_info(sd, &progressive, &width, &height);
> > > + if (ret < 0) {
> > > + width = ADV761X_MAX_WIDTH;
> > > + height = ADV761X_MAX_HEIGHT;
> > > + progressive = 1;
> > > + }
> > > +
> > > + state->width = width;
> > > + state->height = height;
> > > + state->scanmode = (progressive) ?
> >
> > Superfluous parenthesis
>
> Indeed, thanks!
>
> >
> > > + V4L2_FIELD_NONE : V4L2_FIELD_INTERLACED;
> > > +
> > > + mf->width = state->width;
> > > + mf->height = state->height;
> > > + mf->code = state->cfmt->code;
> > > + mf->field = state->scanmode;
> > > + mf->colorspace = state->cfmt->colorspace;
> > > +
> > > + return 0;
> > > +}
> >
> > [snip]
> >
> > > +static struct i2c_client *adv761x_dummy_client(struct v4l2_subdev *sd,
> > > + u8 addr, u8 io_reg)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > +
> > > + io_write(sd, io_reg, addr << 1);
> > > +
> > > + return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
> >
> > Do you really have to re-read? Cannot you just use addr?
>
> I can. don't see much of a difference, though, since it's only done once at
> start-up.
It "just" would remove a redundant IO access and simplify the code a bit.
> > > +}
> >
> > [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.
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(adv761x_pm_ops, adv761x_suspend,
> > > adv761x_resume);
> > > +#define ADV761X_PM_OPS (&adv761x_pm_ops)
> > > +#else /* CONFIG_PM_SLEEP */
> > > +#define ADV761X_PM_OPS NULL
> > > +#endif /* CONFIG_PM_SLEEP */
> > > +
> > > +static const struct i2c_device_id adv761x_id[] = {
> > > + { ADV761X_DRIVER_NAME, 0 },
> > > + {},
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(i2c, adv761x_id);
> > > +
> > > +static struct i2c_driver adv761x_driver = {
> > > + .driver = {
> > > + .owner = THIS_MODULE,
> > > + .name = ADV761X_DRIVER_NAME,
> > > + .pm = ADV761X_PM_OPS,
> > > + },
> > > + .probe = adv761x_probe,
> > > + .remove = adv761x_remove,
> > > + .id_table = adv761x_id,
> > > +};
> > > +
> > > +module_i2c_driver(adv761x_driver);
> > > +
> > > +MODULE_DESCRIPTION("HDMI Receiver ADV761X video decoder driver");
> > > +MODULE_LICENSE("GPL v2");
> > > 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.
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
