Re: [PATCH 1/3] media: i2c: Add ADV761X support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux