[PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders

[PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders

[Guennadi Liakhovetski]

AK8814 only differs from AK8813 by included Macrovision Copy Protection
function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
encoders.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

as is easy to guess, also tested on the same ms7724se

 drivers/media/video/Kconfig     |    7 +
 drivers/media/video/Makefile    |    2 +
 drivers/media/video/ak881x.c    |  360 +++++++++++++++++++++++++++++++++++++++
 include/media/ak881x.h          |   25 +++
 include/media/v4l2-chip-ident.h |    4 +
 5 files changed, 398 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/ak881x.c
 create mode 100644 include/media/ak881x.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index be6d016..d029cc5 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -819,6 +819,13 @@ config VIDEO_CAFE_CCIC
 	  CMOS camera controller.  This is the controller found on first-
 	  generation OLPC systems.
 
+config VIDEO_AK881X
+	tristate "ak8813/ak8814 support"
+	depends on I2C
+	help
+	  This is a video output driver for AK8813 and AK8814 TV encoders
+	  from AKM
+
 config SOC_CAMERA
 	tristate "SoC camera support"
 	depends on VIDEO_V4L2 && HAS_DMA && I2C
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 2669d34..6790051 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -149,6 +149,8 @@ obj-$(CONFIG_VIDEO_CX18) += cx18/
 obj-$(CONFIG_VIDEO_VIVI) += vivi.o
 obj-$(CONFIG_VIDEO_CX23885) += cx23885/
 
+obj-$(CONFIG_VIDEO_AK881X)		+= ak881x.o
+
 obj-$(CONFIG_VIDEO_OMAP2)		+= omap2cam.o
 obj-$(CONFIG_SOC_CAMERA)		+= soc_camera.o soc_mediabus.o
 obj-$(CONFIG_SOC_CAMERA_PLATFORM)	+= soc_camera_platform.o
diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
new file mode 100644
index 0000000..b91f0f6
--- /dev/null
+++ b/drivers/media/video/ak881x.c
@@ -0,0 +1,360 @@
+/*
+ * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
+ *
+ * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * 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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/videodev2.h>
+
+#include <media/ak881x.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
+
+#define AK881X_INTERFACE_MODE	0
+#define AK881X_VIDEO_PROCESS1	1
+#define AK881X_VIDEO_PROCESS2	2
+#define AK881X_VIDEO_PROCESS3	3
+#define AK881X_DAC_MODE		5
+#define AK881X_STATUS		0x24
+#define AK881X_DEVICE_ID	0x25
+#define AK881X_DEVICE_REVISION	0x26
+
+struct ak881x {
+	struct v4l2_subdev subdev;
+	struct ak881x_pdata *pdata;
+	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
+	char revision;	/* DEVICE_REVISION content */
+};
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+		     const u8 data)
+{
+	return i2c_smbus_write_byte_data(client, reg, data);
+}
+
+static int reg_set(struct i2c_client *client, const u8 reg,
+		   const u8 data, u8 mask)
+{
+	int ret = reg_read(client, reg);
+	if (ret < 0)
+		return ret;
+	return reg_write(client, reg, (ret & ~mask) | (data & mask));
+}
+
+static struct ak881x *to_ak881x(const struct i2c_client *client)
+{
+	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
+}
+
+static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
+				struct v4l2_dbg_chip_ident *id)
+{
+	struct i2c_client *client = sd->priv;
+	struct ak881x *ak881x = to_ak881x(client);
+
+	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
+		return -EINVAL;
+
+	if (id->match.addr != client->addr)
+		return -ENODEV;
+
+	id->ident	= ak881x->id;
+	id->revision	= ak881x->revision;
+
+	return 0;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int ak881x_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = sd->priv;
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
+		return -EINVAL;
+
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	reg->val = reg_read(client, reg->reg);
+
+	if (reg->val > 0xffff)
+		return -EIO;
+
+	return 0;
+}
+
+static int ak881x_s_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = sd->priv;
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
+		return -EINVAL;
+
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	if (reg_write(client, reg->reg, reg->val) < 0)
+		return -EIO;
+
+	return 0;
+}
+#endif
+
+static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
+			    struct v4l2_mbus_framefmt *mf)
+{
+	v4l_bound_align_image(&mf->width, 0, 720, 2,
+			      &mf->height, 0, 480, 1, 0);
+	mf->field	= V4L2_FIELD_INTERLACED;
+	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
+	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
+
+	return 0;
+}
+
+static int ak881x_s_fmt(struct v4l2_subdev *sd,
+			 struct v4l2_mbus_framefmt *mf)
+{
+	if (mf->field != V4L2_FIELD_INTERLACED ||
+	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
+		return -EINVAL;
+
+	return ak881x_try_g_fmt(sd, mf);
+}
+
+static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
+			    enum v4l2_mbus_pixelcode *code)
+{
+	if (index)
+		return -EINVAL;
+
+	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
+	return 0;
+}
+
+static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
+{
+	a->bounds.left			= 0;
+	a->bounds.top			= 0;
+	a->bounds.width			= 720;
+	a->bounds.height		= 240 * 2;
+	a->defrect			= a->bounds;
+	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	a->pixelaspect.numerator	= 1;
+	a->pixelaspect.denominator	= 1;
+
+	return 0;
+}
+
+static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
+{
+	struct i2c_client *client = sd->priv;
+	u8 vp1;
+
+	switch (std) {
+	case V4L2_STD_NTSC_M:
+	default:
+		vp1 = 0;
+		break;
+	case V4L2_STD_NTSC_443:
+		vp1 = 3;
+		break;
+	case V4L2_STD_PAL_M:
+		vp1 = 5;
+		break;
+	case V4L2_STD_PAL_60:
+		vp1 = 7;
+		break;
+	case V4L2_STD_PAL_B:
+	case V4L2_STD_PAL_D:
+	case V4L2_STD_PAL_G:
+	case V4L2_STD_PAL_H:
+	case V4L2_STD_PAL_I:
+		vp1 = 0xf;
+	}
+
+	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
+
+	return 0;
+}
+
+static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct i2c_client *client = sd->priv;
+	struct ak881x *ak881x = to_ak881x(client);
+
+	if (enable) {
+		u8 dac;
+		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
+		/* Default: composite output */
+		if (ak881x->pdata->flags & AK881X_COMPONENT)
+			dac = 3;
+		else
+			dac = 4;
+		/* Turn on the DAC(s) */
+		reg_write(client, AK881X_DAC_MODE, dac);
+		dev_dbg(&client->dev, "chip status 0x%x\n",
+			reg_read(client, AK881X_STATUS));
+	} else {
+		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
+		reg_write(client, AK881X_DAC_MODE, 0);
+		dev_dbg(&client->dev, "chip status 0x%x\n",
+			reg_read(client, AK881X_STATUS));
+	}
+
+	return 0;
+}
+
+static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
+	.g_chip_ident	= ak881x_g_chip_ident,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register	= ak881x_g_register,
+	.s_register	= ak881x_s_register,
+#endif
+};
+
+static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
+	.s_mbus_fmt	= ak881x_s_fmt,
+	.g_mbus_fmt	= ak881x_try_g_fmt,
+	.try_mbus_fmt	= ak881x_try_g_fmt,
+	.cropcap	= ak881x_cropcap,
+	.enum_mbus_fmt	= ak881x_enum_fmt,
+	.s_std_output	= ak881x_s_std_output,
+	.s_stream	= ak881x_s_stream,
+};
+
+static struct v4l2_subdev_ops ak881x_subdev_ops = {
+	.core	= &ak881x_subdev_core_ops,
+	.video	= &ak881x_subdev_video_ops,
+};
+
+static int ak881x_probe(struct i2c_client *client,
+			const struct i2c_device_id *did)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct ak881x *ak881x;
+	u8 ifmode, data;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_warn(&adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+		return -EIO;
+	}
+
+	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
+	if (!ak881x)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
+
+	data = reg_read(client, AK881X_DEVICE_ID);
+
+	switch (data) {
+	case 0x13:
+		ak881x->id = V4L2_IDENT_AK8813;
+		break;
+	case 0x14:
+		ak881x->id = V4L2_IDENT_AK8814;
+		break;
+	default:
+		dev_err(&client->dev,
+			"No ak881x chip detected, register read %x\n", data);
+		i2c_set_clientdata(client, NULL);
+		kfree(ak881x);
+		return -ENODEV;
+	}
+
+	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
+	ak881x->pdata = client->dev.platform_data;
+
+	if (ak881x->pdata) {
+		if (ak881x->pdata->flags & AK881X_FIELD)
+			ifmode = 4;
+		else
+			ifmode = 0;
+
+		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
+		case AK881X_IF_MODE_BT656:
+			ifmode |= 1;
+			break;
+		case AK881X_IF_MODE_MASTER:
+			ifmode |= 2;
+			break;
+		case AK881X_IF_MODE_SLAVE:
+		default:
+			break;
+		}
+
+		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
+
+		/*
+		 * "Line Blanking No." seems to be the same as the number of
+		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
+		 * "incidentally" matches ak881x' default
+		 */
+		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
+	}
+
+	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
+		 data, ak881x->revision);
+
+	return 0;
+}
+
+static int ak881x_remove(struct i2c_client *client)
+{
+	struct ak881x *ak881x = to_ak881x(client);
+
+	i2c_set_clientdata(client, NULL);
+	kfree(ak881x);
+
+	return 0;
+}
+
+static const struct i2c_device_id ak881x_id[] = {
+	{ "ak8813", 0 },
+	{ "ak8814", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ak881x_id);
+
+static struct i2c_driver ak881x_i2c_driver = {
+	.driver = {
+		.name = "ak881x",
+	},
+	.probe		= ak881x_probe,
+	.remove		= ak881x_remove,
+	.id_table	= ak881x_id,
+};
+
+static int __init ak881x_module_init(void)
+{
+	return i2c_add_driver(&ak881x_i2c_driver);
+}
+
+static void __exit ak881x_module_exit(void)
+{
+	i2c_del_driver(&ak881x_i2c_driver);
+}
+
+module_init(ak881x_module_init);
+module_exit(ak881x_module_exit);
+
+MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
+MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/ak881x.h b/include/media/ak881x.h
new file mode 100644
index 0000000..b7f2add
--- /dev/null
+++ b/include/media/ak881x.h
@@ -0,0 +1,25 @@
+/*
+ * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
+ *
+ * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * 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.
+ */
+
+#ifndef AK881X_H
+#define AK881X_H
+
+#define AK881X_IF_MODE_MASK	(3 << 0)
+#define AK881X_IF_MODE_BT656	(0 << 0)
+#define AK881X_IF_MODE_MASTER	(1 << 0)
+#define AK881X_IF_MODE_SLAVE	(2 << 0)
+#define AK881X_FIELD		(1 << 2)
+#define AK881X_COMPONENT	(1 << 3)
+
+struct ak881x_pdata {
+	unsigned long flags;
+};
+
+#endif
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index 6cc107d..5d7b742 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -289,6 +289,10 @@ enum {
 
 	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
 	V4L2_IDENT_RJ54N1CB0C = 51980,
+
+	/* AKM AK8813/AK8814 */
+	V4L2_IDENT_AK8813 = 8813,
+	V4L2_IDENT_AK8814 = 8814,
 };
 
 #endif
-- 
1.6.2.4

[PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders from AKM

[Guennadi Liakhovetski]

AK8814 only differs from AK8813 by included Macrovision Copy Protection
function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
encoders.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

as is easy to guess, also tested on the same ms7724se

 drivers/media/video/Kconfig     |    7 +
 drivers/media/video/Makefile    |    2 +
 drivers/media/video/ak881x.c    |  360 +++++++++++++++++++++++++++++++++++++++
 include/media/ak881x.h          |   25 +++
 include/media/v4l2-chip-ident.h |    4 +
 5 files changed, 398 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/ak881x.c
 create mode 100644 include/media/ak881x.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index be6d016..d029cc5 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -819,6 +819,13 @@ config VIDEO_CAFE_CCIC
 	  CMOS camera controller.  This is the controller found on first-
 	  generation OLPC systems.
 
+config VIDEO_AK881X
+	tristate "ak8813/ak8814 support"
+	depends on I2C
+	help
+	  This is a video output driver for AK8813 and AK8814 TV encoders
+	  from AKM
+
 config SOC_CAMERA
 	tristate "SoC camera support"
 	depends on VIDEO_V4L2 && HAS_DMA && I2C
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 2669d34..6790051 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -149,6 +149,8 @@ obj-$(CONFIG_VIDEO_CX18) += cx18/
 obj-$(CONFIG_VIDEO_VIVI) += vivi.o
 obj-$(CONFIG_VIDEO_CX23885) += cx23885/
 
+obj-$(CONFIG_VIDEO_AK881X)		+= ak881x.o
+
 obj-$(CONFIG_VIDEO_OMAP2)		+= omap2cam.o
 obj-$(CONFIG_SOC_CAMERA)		+= soc_camera.o soc_mediabus.o
 obj-$(CONFIG_SOC_CAMERA_PLATFORM)	+= soc_camera_platform.o
diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
new file mode 100644
index 0000000..b91f0f6
--- /dev/null
+++ b/drivers/media/video/ak881x.c
@@ -0,0 +1,360 @@
+/*
+ * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
+ *
+ * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * 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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/videodev2.h>
+
+#include <media/ak881x.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
+
+#define AK881X_INTERFACE_MODE	0
+#define AK881X_VIDEO_PROCESS1	1
+#define AK881X_VIDEO_PROCESS2	2
+#define AK881X_VIDEO_PROCESS3	3
+#define AK881X_DAC_MODE		5
+#define AK881X_STATUS		0x24
+#define AK881X_DEVICE_ID	0x25
+#define AK881X_DEVICE_REVISION	0x26
+
+struct ak881x {
+	struct v4l2_subdev subdev;
+	struct ak881x_pdata *pdata;
+	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
+	char revision;	/* DEVICE_REVISION content */
+};
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+		     const u8 data)
+{
+	return i2c_smbus_write_byte_data(client, reg, data);
+}
+
+static int reg_set(struct i2c_client *client, const u8 reg,
+		   const u8 data, u8 mask)
+{
+	int ret = reg_read(client, reg);
+	if (ret < 0)
+		return ret;
+	return reg_write(client, reg, (ret & ~mask) | (data & mask));
+}
+
+static struct ak881x *to_ak881x(const struct i2c_client *client)
+{
+	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
+}
+
+static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
+				struct v4l2_dbg_chip_ident *id)
+{
+	struct i2c_client *client = sd->priv;
+	struct ak881x *ak881x = to_ak881x(client);
+
+	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
+		return -EINVAL;
+
+	if (id->match.addr != client->addr)
+		return -ENODEV;
+
+	id->ident	= ak881x->id;
+	id->revision	= ak881x->revision;
+
+	return 0;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int ak881x_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = sd->priv;
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
+		return -EINVAL;
+
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	reg->val = reg_read(client, reg->reg);
+
+	if (reg->val > 0xffff)
+		return -EIO;
+
+	return 0;
+}
+
+static int ak881x_s_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = sd->priv;
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
+		return -EINVAL;
+
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	if (reg_write(client, reg->reg, reg->val) < 0)
+		return -EIO;
+
+	return 0;
+}
+#endif
+
+static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
+			    struct v4l2_mbus_framefmt *mf)
+{
+	v4l_bound_align_image(&mf->width, 0, 720, 2,
+			      &mf->height, 0, 480, 1, 0);
+	mf->field	= V4L2_FIELD_INTERLACED;
+	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
+	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
+
+	return 0;
+}
+
+static int ak881x_s_fmt(struct v4l2_subdev *sd,
+			 struct v4l2_mbus_framefmt *mf)
+{
+	if (mf->field != V4L2_FIELD_INTERLACED ||
+	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
+		return -EINVAL;
+
+	return ak881x_try_g_fmt(sd, mf);
+}
+
+static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
+			    enum v4l2_mbus_pixelcode *code)
+{
+	if (index)
+		return -EINVAL;
+
+	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
+	return 0;
+}
+
+static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
+{
+	a->bounds.left			= 0;
+	a->bounds.top			= 0;
+	a->bounds.width			= 720;
+	a->bounds.height		= 240 * 2;
+	a->defrect			= a->bounds;
+	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	a->pixelaspect.numerator	= 1;
+	a->pixelaspect.denominator	= 1;
+
+	return 0;
+}
+
+static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
+{
+	struct i2c_client *client = sd->priv;
+	u8 vp1;
+
+	switch (std) {
+	case V4L2_STD_NTSC_M:
+	default:
+		vp1 = 0;
+		break;
+	case V4L2_STD_NTSC_443:
+		vp1 = 3;
+		break;
+	case V4L2_STD_PAL_M:
+		vp1 = 5;
+		break;
+	case V4L2_STD_PAL_60:
+		vp1 = 7;
+		break;
+	case V4L2_STD_PAL_B:
+	case V4L2_STD_PAL_D:
+	case V4L2_STD_PAL_G:
+	case V4L2_STD_PAL_H:
+	case V4L2_STD_PAL_I:
+		vp1 = 0xf;
+	}
+
+	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
+
+	return 0;
+}
+
+static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct i2c_client *client = sd->priv;
+	struct ak881x *ak881x = to_ak881x(client);
+
+	if (enable) {
+		u8 dac;
+		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
+		/* Default: composite output */
+		if (ak881x->pdata->flags & AK881X_COMPONENT)
+			dac = 3;
+		else
+			dac = 4;
+		/* Turn on the DAC(s) */
+		reg_write(client, AK881X_DAC_MODE, dac);
+		dev_dbg(&client->dev, "chip status 0x%x\n",
+			reg_read(client, AK881X_STATUS));
+	} else {
+		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
+		reg_write(client, AK881X_DAC_MODE, 0);
+		dev_dbg(&client->dev, "chip status 0x%x\n",
+			reg_read(client, AK881X_STATUS));
+	}
+
+	return 0;
+}
+
+static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
+	.g_chip_ident	= ak881x_g_chip_ident,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register	= ak881x_g_register,
+	.s_register	= ak881x_s_register,
+#endif
+};
+
+static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
+	.s_mbus_fmt	= ak881x_s_fmt,
+	.g_mbus_fmt	= ak881x_try_g_fmt,
+	.try_mbus_fmt	= ak881x_try_g_fmt,
+	.cropcap	= ak881x_cropcap,
+	.enum_mbus_fmt	= ak881x_enum_fmt,
+	.s_std_output	= ak881x_s_std_output,
+	.s_stream	= ak881x_s_stream,
+};
+
+static struct v4l2_subdev_ops ak881x_subdev_ops = {
+	.core	= &ak881x_subdev_core_ops,
+	.video	= &ak881x_subdev_video_ops,
+};
+
+static int ak881x_probe(struct i2c_client *client,
+			const struct i2c_device_id *did)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct ak881x *ak881x;
+	u8 ifmode, data;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_warn(&adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+		return -EIO;
+	}
+
+	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
+	if (!ak881x)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
+
+	data = reg_read(client, AK881X_DEVICE_ID);
+
+	switch (data) {
+	case 0x13:
+		ak881x->id = V4L2_IDENT_AK8813;
+		break;
+	case 0x14:
+		ak881x->id = V4L2_IDENT_AK8814;
+		break;
+	default:
+		dev_err(&client->dev,
+			"No ak881x chip detected, register read %x\n", data);
+		i2c_set_clientdata(client, NULL);
+		kfree(ak881x);
+		return -ENODEV;
+	}
+
+	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
+	ak881x->pdata = client->dev.platform_data;
+
+	if (ak881x->pdata) {
+		if (ak881x->pdata->flags & AK881X_FIELD)
+			ifmode = 4;
+		else
+			ifmode = 0;
+
+		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
+		case AK881X_IF_MODE_BT656:
+			ifmode |= 1;
+			break;
+		case AK881X_IF_MODE_MASTER:
+			ifmode |= 2;
+			break;
+		case AK881X_IF_MODE_SLAVE:
+		default:
+			break;
+		}
+
+		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
+
+		/*
+		 * "Line Blanking No." seems to be the same as the number of
+		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
+		 * "incidentally" matches ak881x' default
+		 */
+		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
+	}
+
+	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
+		 data, ak881x->revision);
+
+	return 0;
+}
+
+static int ak881x_remove(struct i2c_client *client)
+{
+	struct ak881x *ak881x = to_ak881x(client);
+
+	i2c_set_clientdata(client, NULL);
+	kfree(ak881x);
+
+	return 0;
+}
+
+static const struct i2c_device_id ak881x_id[] = {
+	{ "ak8813", 0 },
+	{ "ak8814", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ak881x_id);
+
+static struct i2c_driver ak881x_i2c_driver = {
+	.driver = {
+		.name = "ak881x",
+	},
+	.probe		= ak881x_probe,
+	.remove		= ak881x_remove,
+	.id_table	= ak881x_id,
+};
+
+static int __init ak881x_module_init(void)
+{
+	return i2c_add_driver(&ak881x_i2c_driver);
+}
+
+static void __exit ak881x_module_exit(void)
+{
+	i2c_del_driver(&ak881x_i2c_driver);
+}
+
+module_init(ak881x_module_init);
+module_exit(ak881x_module_exit);
+
+MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
+MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/ak881x.h b/include/media/ak881x.h
new file mode 100644
index 0000000..b7f2add
--- /dev/null
+++ b/include/media/ak881x.h
@@ -0,0 +1,25 @@
+/*
+ * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
+ *
+ * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * 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.
+ */
+
+#ifndef AK881X_H
+#define AK881X_H
+
+#define AK881X_IF_MODE_MASK	(3 << 0)
+#define AK881X_IF_MODE_BT656	(0 << 0)
+#define AK881X_IF_MODE_MASTER	(1 << 0)
+#define AK881X_IF_MODE_SLAVE	(2 << 0)
+#define AK881X_FIELD		(1 << 2)
+#define AK881X_COMPONENT	(1 << 3)
+
+struct ak881x_pdata {
+	unsigned long flags;
+};
+
+#endif
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index 6cc107d..5d7b742 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -289,6 +289,10 @@ enum {
 
 	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
 	V4L2_IDENT_RJ54N1CB0C = 51980,
+
+	/* AKM AK8813/AK8814 */
+	V4L2_IDENT_AK8813 = 8813,
+	V4L2_IDENT_AK8814 = 8814,
 };
 
 #endif
-- 
1.6.2.4

[PATCH 3/3] sh: add Video Output Unit (VOU) and AK8813 TV-encoder

[Guennadi Liakhovetski]

Add platform bindings, GPIO initialisation and allocation and AK8813 reset code
to ms7724se.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Obviously depends on the previous two VOU and AK881x patches, sorry for 
not marking those with "PATCH x/3" accordingly. Those two patches do not 
depend upon each other and initially I wasn't sure I'd be able to clean up 
this patch sufficiently for submission. Two 10us delays are pretty random, 
maybe they can be optimised out completely. I just tried to reproduced the 
reset procedure from the ak8813 datasheet, and it says nothing about the 
duration of respective stages.

 arch/sh/boards/mach-se/7724/setup.c |   88 ++++++++++++++++++++++++++++++++---
 1 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/arch/sh/boards/mach-se/7724/setup.c b/arch/sh/boards/mach-se/7724/setup.c
index c7dbbec..8c8379b 100644
--- a/arch/sh/boards/mach-se/7724/setup.c
+++ b/arch/sh/boards/mach-se/7724/setup.c
@@ -489,6 +489,52 @@ static struct platform_device sdhi1_cn8_device = {
 	},
 };
 
+#include <media/ak881x.h>
+#include <media/sh_vou.h>
+
+struct ak881x_pdata ak881x_pdata = {
+	.flags = AK881X_IF_MODE_SLAVE,
+};
+
+static struct i2c_board_info ak8813 = {
+	/* With open J18 jumper address is 0x21 */
+	I2C_BOARD_INFO("ak8813", 0x20),
+	.platform_data = &ak881x_pdata,
+};
+
+struct sh_vou_pdata sh_vou_pdata = {
+	.bus_fmt	= SH_VOU_BUS_PAL_8BIT,
+	.flags		= SH_VOU_HSYNC_LOW | SH_VOU_VSYNC_LOW,
+	.board_info	= &ak8813,
+	.i2c_adap	= 0,
+	.module_name	= "ak881x",
+};
+
+static struct resource sh_vou_resources[] = {
+	[0] = {
+		.start  = 0xfe960000,
+		.end    = 0xfe962043,
+		.flags  = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start  = 55,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device vou_device = {
+	.name           = "sh-vou",
+	.id		= -1,
+	.num_resources  = ARRAY_SIZE(sh_vou_resources),
+	.resource       = sh_vou_resources,
+	.dev		= {
+		.platform_data	= &sh_vou_pdata,
+	},
+	.archdata	= {
+		.hwblk_id	= HWBLK_VOU,
+	},
+};
+
 static struct platform_device *ms7724se_devices[] __initdata = {
 	&heartbeat_device,
 	&smc91x_eth_device,
@@ -503,6 +549,7 @@ static struct platform_device *ms7724se_devices[] __initdata = {
 	&fsi_device,
 	&sdhi0_cn7_device,
 	&sdhi1_cn8_device,
+	&vou_device,
 };
 
 /* I2C device */
@@ -587,6 +634,7 @@ static int __init devices_setup(void)
 {
 	u16 sw = __raw_readw(SW4140); /* select camera, monitor */
 	struct clk *fsia_clk;
+	u16 fpga_out;
 
 	/* register board specific self-refresh code */
 	sh_mobile_register_self_refresh(SUSP_SH_STANDBY | SUSP_SH_SF,
@@ -595,13 +643,25 @@ static int __init devices_setup(void)
 					&ms7724se_sdram_leave_start,
 					&ms7724se_sdram_leave_end);
 	/* Reset Release */
-	__raw_writew(__raw_readw(FPGA_OUT) &
-		  ~((1 << 1)  | /* LAN */
-		    (1 << 6)  | /* VIDEO DAC */
-		    (1 << 7)  | /* AK4643 */
-		    (1 << 12) | /* USB0 */
-		    (1 << 14)), /* RMII */
-		  FPGA_OUT);
+	fpga_out = __raw_readw(FPGA_OUT);
+	/* bit4: NTSC_PDN, bit5: NTSC_RESET */
+	fpga_out &= ~((1 << 1)  | /* LAN */
+		      (1 << 4)  | /* AK8813 PDN */
+		      (1 << 5)  | /* AK8813 RESET */
+		      (1 << 6)  | /* VIDEO DAC */
+		      (1 << 7)  | /* AK4643 */
+		      (1 << 12) | /* USB0 */
+		      (1 << 14)); /* RMII */
+	__raw_writew(fpga_out | (1 << 4), FPGA_OUT);
+
+	udelay(10);
+
+	/* AK8813 RESET */
+	__raw_writew(fpga_out | (1 << 5), FPGA_OUT);
+
+	udelay(10);
+
+	__raw_writew(fpga_out, FPGA_OUT);
 
 	/* turn on USB clocks, use external clock */
 	__raw_writew((__raw_readw(PORT_MSELCRB) & ~0xc000) | 0x8000, PORT_MSELCRB);
@@ -837,6 +897,20 @@ static int __init devices_setup(void)
 		lcdc_info.ch[0].flags          = LCDC_FLAGS_DWPOL;
 	}
 
+	/* VOU */
+	gpio_request(GPIO_FN_DV_D15, NULL);
+	gpio_request(GPIO_FN_DV_D14, NULL);
+	gpio_request(GPIO_FN_DV_D13, NULL);
+	gpio_request(GPIO_FN_DV_D12, NULL);
+	gpio_request(GPIO_FN_DV_D11, NULL);
+	gpio_request(GPIO_FN_DV_D10, NULL);
+	gpio_request(GPIO_FN_DV_D9, NULL);
+	gpio_request(GPIO_FN_DV_D8, NULL);
+	gpio_request(GPIO_FN_DV_CLKI, NULL);
+	gpio_request(GPIO_FN_DV_CLK, NULL);
+	gpio_request(GPIO_FN_DV_VSYNC, NULL);
+	gpio_request(GPIO_FN_DV_HSYNC, NULL);
+
 	return platform_add_devices(ms7724se_devices,
 				    ARRAY_SIZE(ms7724se_devices));
 }
-- 
1.6.2.4

[PATCH 3/3] sh: add Video Output Unit (VOU) and AK8813 TV-encoder support to ms7724se

[Guennadi Liakhovetski]

Add platform bindings, GPIO initialisation and allocation and AK8813 reset code
to ms7724se.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Obviously depends on the previous two VOU and AK881x patches, sorry for 
not marking those with "PATCH x/3" accordingly. Those two patches do not 
depend upon each other and initially I wasn't sure I'd be able to clean up 
this patch sufficiently for submission. Two 10us delays are pretty random, 
maybe they can be optimised out completely. I just tried to reproduced the 
reset procedure from the ak8813 datasheet, and it says nothing about the 
duration of respective stages.

 arch/sh/boards/mach-se/7724/setup.c |   88 ++++++++++++++++++++++++++++++++---
 1 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/arch/sh/boards/mach-se/7724/setup.c b/arch/sh/boards/mach-se/7724/setup.c
index c7dbbec..8c8379b 100644
--- a/arch/sh/boards/mach-se/7724/setup.c
+++ b/arch/sh/boards/mach-se/7724/setup.c
@@ -489,6 +489,52 @@ static struct platform_device sdhi1_cn8_device = {
 	},
 };
 
+#include <media/ak881x.h>
+#include <media/sh_vou.h>
+
+struct ak881x_pdata ak881x_pdata = {
+	.flags = AK881X_IF_MODE_SLAVE,
+};
+
+static struct i2c_board_info ak8813 = {
+	/* With open J18 jumper address is 0x21 */
+	I2C_BOARD_INFO("ak8813", 0x20),
+	.platform_data = &ak881x_pdata,
+};
+
+struct sh_vou_pdata sh_vou_pdata = {
+	.bus_fmt	= SH_VOU_BUS_PAL_8BIT,
+	.flags		= SH_VOU_HSYNC_LOW | SH_VOU_VSYNC_LOW,
+	.board_info	= &ak8813,
+	.i2c_adap	= 0,
+	.module_name	= "ak881x",
+};
+
+static struct resource sh_vou_resources[] = {
+	[0] = {
+		.start  = 0xfe960000,
+		.end    = 0xfe962043,
+		.flags  = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start  = 55,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device vou_device = {
+	.name           = "sh-vou",
+	.id		= -1,
+	.num_resources  = ARRAY_SIZE(sh_vou_resources),
+	.resource       = sh_vou_resources,
+	.dev		= {
+		.platform_data	= &sh_vou_pdata,
+	},
+	.archdata	= {
+		.hwblk_id	= HWBLK_VOU,
+	},
+};
+
 static struct platform_device *ms7724se_devices[] __initdata = {
 	&heartbeat_device,
 	&smc91x_eth_device,
@@ -503,6 +549,7 @@ static struct platform_device *ms7724se_devices[] __initdata = {
 	&fsi_device,
 	&sdhi0_cn7_device,
 	&sdhi1_cn8_device,
+	&vou_device,
 };
 
 /* I2C device */
@@ -587,6 +634,7 @@ static int __init devices_setup(void)
 {
 	u16 sw = __raw_readw(SW4140); /* select camera, monitor */
 	struct clk *fsia_clk;
+	u16 fpga_out;
 
 	/* register board specific self-refresh code */
 	sh_mobile_register_self_refresh(SUSP_SH_STANDBY | SUSP_SH_SF,
@@ -595,13 +643,25 @@ static int __init devices_setup(void)
 					&ms7724se_sdram_leave_start,
 					&ms7724se_sdram_leave_end);
 	/* Reset Release */
-	__raw_writew(__raw_readw(FPGA_OUT) &
-		  ~((1 << 1)  | /* LAN */
-		    (1 << 6)  | /* VIDEO DAC */
-		    (1 << 7)  | /* AK4643 */
-		    (1 << 12) | /* USB0 */
-		    (1 << 14)), /* RMII */
-		  FPGA_OUT);
+	fpga_out = __raw_readw(FPGA_OUT);
+	/* bit4: NTSC_PDN, bit5: NTSC_RESET */
+	fpga_out &= ~((1 << 1)  | /* LAN */
+		      (1 << 4)  | /* AK8813 PDN */
+		      (1 << 5)  | /* AK8813 RESET */
+		      (1 << 6)  | /* VIDEO DAC */
+		      (1 << 7)  | /* AK4643 */
+		      (1 << 12) | /* USB0 */
+		      (1 << 14)); /* RMII */
+	__raw_writew(fpga_out | (1 << 4), FPGA_OUT);
+
+	udelay(10);
+
+	/* AK8813 RESET */
+	__raw_writew(fpga_out | (1 << 5), FPGA_OUT);
+
+	udelay(10);
+
+	__raw_writew(fpga_out, FPGA_OUT);
 
 	/* turn on USB clocks, use external clock */
 	__raw_writew((__raw_readw(PORT_MSELCRB) & ~0xc000) | 0x8000, PORT_MSELCRB);
@@ -837,6 +897,20 @@ static int __init devices_setup(void)
 		lcdc_info.ch[0].flags          = LCDC_FLAGS_DWPOL;
 	}
 
+	/* VOU */
+	gpio_request(GPIO_FN_DV_D15, NULL);
+	gpio_request(GPIO_FN_DV_D14, NULL);
+	gpio_request(GPIO_FN_DV_D13, NULL);
+	gpio_request(GPIO_FN_DV_D12, NULL);
+	gpio_request(GPIO_FN_DV_D11, NULL);
+	gpio_request(GPIO_FN_DV_D10, NULL);
+	gpio_request(GPIO_FN_DV_D9, NULL);
+	gpio_request(GPIO_FN_DV_D8, NULL);
+	gpio_request(GPIO_FN_DV_CLKI, NULL);
+	gpio_request(GPIO_FN_DV_CLK, NULL);
+	gpio_request(GPIO_FN_DV_VSYNC, NULL);
+	gpio_request(GPIO_FN_DV_HSYNC, NULL);
+
 	return platform_add_devices(ms7724se_devices,
 				    ARRAY_SIZE(ms7724se_devices));
 }
-- 
1.6.2.4

Re: [PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders from AKM

[Hans Verkuil]

Review notes below...

On Thursday 11 March 2010 11:25:42 Guennadi Liakhovetski wrote:
> AK8814 only differs from AK8813 by included Macrovision Copy Protection
> function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
> encoders.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> as is easy to guess, also tested on the same ms7724se
> 
>  drivers/media/video/Kconfig     |    7 +
>  drivers/media/video/Makefile    |    2 +
>  drivers/media/video/ak881x.c    |  360 +++++++++++++++++++++++++++++++++++++++
>  include/media/ak881x.h          |   25 +++
>  include/media/v4l2-chip-ident.h |    4 +
>  5 files changed, 398 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/ak881x.c
>  create mode 100644 include/media/ak881x.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index be6d016..d029cc5 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -819,6 +819,13 @@ config VIDEO_CAFE_CCIC
>  	  CMOS camera controller.  This is the controller found on first-
>  	  generation OLPC systems.
>  
> +config VIDEO_AK881X
> +	tristate "ak8813/ak8814 support"
> +	depends on I2C
> +	help
> +	  This is a video output driver for AK8813 and AK8814 TV encoders
> +	  from AKM
> +
>  config SOC_CAMERA
>  	tristate "SoC camera support"
>  	depends on VIDEO_V4L2 && HAS_DMA && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 2669d34..6790051 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -149,6 +149,8 @@ obj-$(CONFIG_VIDEO_CX18) += cx18/
>  obj-$(CONFIG_VIDEO_VIVI) += vivi.o
>  obj-$(CONFIG_VIDEO_CX23885) += cx23885/
>  
> +obj-$(CONFIG_VIDEO_AK881X)		+= ak881x.o
> +
>  obj-$(CONFIG_VIDEO_OMAP2)		+= omap2cam.o
>  obj-$(CONFIG_SOC_CAMERA)		+= soc_camera.o soc_mediabus.o
>  obj-$(CONFIG_SOC_CAMERA_PLATFORM)	+= soc_camera_platform.o
> diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
> new file mode 100644
> index 0000000..b91f0f6
> --- /dev/null
> +++ b/drivers/media/video/ak881x.c
> @@ -0,0 +1,360 @@
> +/*
> + * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> + *
> + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * 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.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/ak881x.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-device.h>
> +
> +#define AK881X_INTERFACE_MODE	0
> +#define AK881X_VIDEO_PROCESS1	1
> +#define AK881X_VIDEO_PROCESS2	2
> +#define AK881X_VIDEO_PROCESS3	3
> +#define AK881X_DAC_MODE		5
> +#define AK881X_STATUS		0x24
> +#define AK881X_DEVICE_ID	0x25
> +#define AK881X_DEVICE_REVISION	0x26
> +
> +struct ak881x {
> +	struct v4l2_subdev subdev;
> +	struct ak881x_pdata *pdata;
> +	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
> +	char revision;	/* DEVICE_REVISION content */
> +};
> +
> +static int reg_read(struct i2c_client *client, const u8 reg)
> +{
> +	return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int reg_write(struct i2c_client *client, const u8 reg,
> +		     const u8 data)
> +{
> +	return i2c_smbus_write_byte_data(client, reg, data);
> +}

I suggest making these inline.

I also recommend using struct v4l2_subdev instead of struct i2c_client as
argument. In my experience it makes the code cleaner if the mapping from
subdev to i2c_client is done at the lowest possible level.

> +
> +static int reg_set(struct i2c_client *client, const u8 reg,
> +		   const u8 data, u8 mask)
> +{
> +	int ret = reg_read(client, reg);
> +	if (ret < 0)
> +		return ret;
> +	return reg_write(client, reg, (ret & ~mask) | (data & mask));
> +}
> +
> +static struct ak881x *to_ak881x(const struct i2c_client *client)
> +{
> +	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
> +}

Ditto for this one.

> +
> +static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
> +				struct v4l2_dbg_chip_ident *id)
> +{
> +	struct i2c_client *client = sd->priv;

Don't use sd->priv directly. Use v4l2_get_subdevdata(sd) instead.

> +	struct ak881x *ak881x = to_ak881x(client);
> +
> +	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
> +		return -EINVAL;
> +
> +	if (id->match.addr != client->addr)
> +		return -ENODEV;
> +
> +	id->ident	= ak881x->id;
> +	id->revision	= ak881x->revision;
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int ak881x_g_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = sd->priv;
> +
> +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> +		return -EINVAL;
> +
> +	if (reg->match.addr != client->addr)
> +		return -ENODEV;
> +
> +	reg->val = reg_read(client, reg->reg);
> +
> +	if (reg->val > 0xffff)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = sd->priv;
> +
> +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> +		return -EINVAL;
> +
> +	if (reg->match.addr != client->addr)
> +		return -ENODEV;
> +
> +	if (reg_write(client, reg->reg, reg->val) < 0)
> +		return -EIO;
> +
> +	return 0;
> +}
> +#endif
> +
> +static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
> +			    struct v4l2_mbus_framefmt *mf)

Can you rename this function to ak881x_try_g_mbus_fmt? Same for the other
fmt functions.

> +{
> +	v4l_bound_align_image(&mf->width, 0, 720, 2,
> +			      &mf->height, 0, 480, 1, 0);

480? Doesn't this do 576 as well when PAL is selected?

> +	mf->field	= V4L2_FIELD_INTERLACED;
> +	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
> +	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_fmt(struct v4l2_subdev *sd,
> +			 struct v4l2_mbus_framefmt *mf)
> +{
> +	if (mf->field != V4L2_FIELD_INTERLACED ||
> +	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
> +		return -EINVAL;
> +
> +	return ak881x_try_g_fmt(sd, mf);
> +}
> +
> +static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
> +			    enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
> +	return 0;
> +}
> +
> +static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	a->bounds.left			= 0;
> +	a->bounds.top			= 0;
> +	a->bounds.width			= 720;
> +	a->bounds.height		= 240 * 2;

288 * 2 for PAL?

> +	a->defrect			= a->bounds;
> +	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +	a->pixelaspect.numerator	= 1;
> +	a->pixelaspect.denominator	= 1;
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
> +{
> +	struct i2c_client *client = sd->priv;
> +	u8 vp1;
> +
> +	switch (std) {
> +	case V4L2_STD_NTSC_M:
> +	default:
> +		vp1 = 0;
> +		break;
> +	case V4L2_STD_NTSC_443:
> +		vp1 = 3;
> +		break;
> +	case V4L2_STD_PAL_M:
> +		vp1 = 5;
> +		break;
> +	case V4L2_STD_PAL_60:
> +		vp1 = 7;
> +		break;
> +	case V4L2_STD_PAL_B:
> +	case V4L2_STD_PAL_D:
> +	case V4L2_STD_PAL_G:
> +	case V4L2_STD_PAL_H:
> +	case V4L2_STD_PAL_I:
> +		vp1 = 0xf;
> +	}
> +
> +	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct i2c_client *client = sd->priv;
> +	struct ak881x *ak881x = to_ak881x(client);
> +
> +	if (enable) {
> +		u8 dac;
> +		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
> +		/* Default: composite output */
> +		if (ak881x->pdata->flags & AK881X_COMPONENT)
> +			dac = 3;
> +		else
> +			dac = 4;
> +		/* Turn on the DAC(s) */
> +		reg_write(client, AK881X_DAC_MODE, dac);
> +		dev_dbg(&client->dev, "chip status 0x%x\n",
> +			reg_read(client, AK881X_STATUS));
> +	} else {
> +		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
> +		reg_write(client, AK881X_DAC_MODE, 0);
> +		dev_dbg(&client->dev, "chip status 0x%x\n",
> +			reg_read(client, AK881X_STATUS));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
> +	.g_chip_ident	= ak881x_g_chip_ident,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	.g_register	= ak881x_g_register,
> +	.s_register	= ak881x_s_register,
> +#endif
> +};
> +
> +static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
> +	.s_mbus_fmt	= ak881x_s_fmt,
> +	.g_mbus_fmt	= ak881x_try_g_fmt,
> +	.try_mbus_fmt	= ak881x_try_g_fmt,
> +	.cropcap	= ak881x_cropcap,
> +	.enum_mbus_fmt	= ak881x_enum_fmt,
> +	.s_std_output	= ak881x_s_std_output,
> +	.s_stream	= ak881x_s_stream,
> +};
> +
> +static struct v4l2_subdev_ops ak881x_subdev_ops = {
> +	.core	= &ak881x_subdev_core_ops,
> +	.video	= &ak881x_subdev_video_ops,
> +};
> +
> +static int ak881x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *did)
> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	struct ak881x *ak881x;
> +	u8 ifmode, data;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		dev_warn(&adapter->dev,
> +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> +		return -EIO;
> +	}
> +
> +	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
> +	if (!ak881x)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
> +
> +	data = reg_read(client, AK881X_DEVICE_ID);
> +
> +	switch (data) {
> +	case 0x13:
> +		ak881x->id = V4L2_IDENT_AK8813;
> +		break;
> +	case 0x14:
> +		ak881x->id = V4L2_IDENT_AK8814;
> +		break;
> +	default:
> +		dev_err(&client->dev,
> +			"No ak881x chip detected, register read %x\n", data);
> +		i2c_set_clientdata(client, NULL);

No need to call i2c_set_clientdata here.

> +		kfree(ak881x);
> +		return -ENODEV;
> +	}
> +
> +	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
> +	ak881x->pdata = client->dev.platform_data;
> +
> +	if (ak881x->pdata) {
> +		if (ak881x->pdata->flags & AK881X_FIELD)
> +			ifmode = 4;
> +		else
> +			ifmode = 0;
> +
> +		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
> +		case AK881X_IF_MODE_BT656:
> +			ifmode |= 1;
> +			break;
> +		case AK881X_IF_MODE_MASTER:
> +			ifmode |= 2;
> +			break;
> +		case AK881X_IF_MODE_SLAVE:
> +		default:
> +			break;
> +		}
> +
> +		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
> +
> +		/*
> +		 * "Line Blanking No." seems to be the same as the number of
> +		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
> +		 * "incidentally" matches ak881x' default
> +		 */
> +		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
> +	}
> +
> +	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
> +		 data, ak881x->revision);

Please use this instead:

        v4l2_info(&ak881x->subdev, "chip found @ 0x%02x (%s, revision %x)\n",
                 client->addr << 1, client->adapter->name, ak881x->revision);

This is for consistency with other i2c v4l drivers.

Please also use v4l2_info/warn/err/dbg instead of the dev_ versions (except if
there is no subdev pointer available). Again for consistency with other i2c
drivers (and a more concise prefix as well).


> +
> +	return 0;
> +}
> +
> +static int ak881x_remove(struct i2c_client *client)
> +{
> +	struct ak881x *ak881x = to_ak881x(client);
> +
> +	i2c_set_clientdata(client, NULL);

This is not right. Use this instead:

        v4l2_device_unregister_subdev(sd);

See v4l2-framework.txt why you should do this in remove().

> +	kfree(ak881x);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ak881x_id[] = {
> +	{ "ak8813", 0 },
> +	{ "ak8814", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ak881x_id);
> +
> +static struct i2c_driver ak881x_i2c_driver = {
> +	.driver = {
> +		.name = "ak881x",
> +	},
> +	.probe		= ak881x_probe,
> +	.remove		= ak881x_remove,
> +	.id_table	= ak881x_id,
> +};
> +
> +static int __init ak881x_module_init(void)
> +{
> +	return i2c_add_driver(&ak881x_i2c_driver);
> +}
> +
> +static void __exit ak881x_module_exit(void)
> +{
> +	i2c_del_driver(&ak881x_i2c_driver);
> +}
> +
> +module_init(ak881x_module_init);
> +module_exit(ak881x_module_exit);
> +
> +MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
> +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/media/ak881x.h b/include/media/ak881x.h
> new file mode 100644
> index 0000000..b7f2add
> --- /dev/null
> +++ b/include/media/ak881x.h
> @@ -0,0 +1,25 @@
> +/*
> + * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> + *
> + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * 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.
> + */
> +
> +#ifndef AK881X_H
> +#define AK881X_H
> +
> +#define AK881X_IF_MODE_MASK	(3 << 0)
> +#define AK881X_IF_MODE_BT656	(0 << 0)
> +#define AK881X_IF_MODE_MASTER	(1 << 0)
> +#define AK881X_IF_MODE_SLAVE	(2 << 0)
> +#define AK881X_FIELD		(1 << 2)
> +#define AK881X_COMPONENT	(1 << 3)
> +
> +struct ak881x_pdata {
> +	unsigned long flags;

Why unsigned long? u32 makes more sense. For 64-bit archs unsigned long is
64 bits.

> +};
> +
> +#endif
> diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> index 6cc107d..5d7b742 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -289,6 +289,10 @@ enum {
>  
>  	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
>  	V4L2_IDENT_RJ54N1CB0C = 51980,
> +
> +	/* AKM AK8813/AK8814 */
> +	V4L2_IDENT_AK8813 = 8813,
> +	V4L2_IDENT_AK8814 = 8814,

The IDs in v4l2-chip-ident.h should be kept in increasing numeric order. I see
that several are already placed out of order. I'm going to make a patch for
that and then you can add these new IDs in the right place.

>  };
>  
>  #endif
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders from AKM

[Hans Verkuil]

Review notes below...

On Thursday 11 March 2010 11:25:42 Guennadi Liakhovetski wrote:
> AK8814 only differs from AK8813 by included Macrovision Copy Protection
> function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
> encoders.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> as is easy to guess, also tested on the same ms7724se
> 
>  drivers/media/video/Kconfig     |    7 +
>  drivers/media/video/Makefile    |    2 +
>  drivers/media/video/ak881x.c    |  360 +++++++++++++++++++++++++++++++++++++++
>  include/media/ak881x.h          |   25 +++
>  include/media/v4l2-chip-ident.h |    4 +
>  5 files changed, 398 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/ak881x.c
>  create mode 100644 include/media/ak881x.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index be6d016..d029cc5 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -819,6 +819,13 @@ config VIDEO_CAFE_CCIC
>  	  CMOS camera controller.  This is the controller found on first-
>  	  generation OLPC systems.
>  
> +config VIDEO_AK881X
> +	tristate "ak8813/ak8814 support"
> +	depends on I2C
> +	help
> +	  This is a video output driver for AK8813 and AK8814 TV encoders
> +	  from AKM
> +
>  config SOC_CAMERA
>  	tristate "SoC camera support"
>  	depends on VIDEO_V4L2 && HAS_DMA && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 2669d34..6790051 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -149,6 +149,8 @@ obj-$(CONFIG_VIDEO_CX18) += cx18/
>  obj-$(CONFIG_VIDEO_VIVI) += vivi.o
>  obj-$(CONFIG_VIDEO_CX23885) += cx23885/
>  
> +obj-$(CONFIG_VIDEO_AK881X)		+= ak881x.o
> +
>  obj-$(CONFIG_VIDEO_OMAP2)		+= omap2cam.o
>  obj-$(CONFIG_SOC_CAMERA)		+= soc_camera.o soc_mediabus.o
>  obj-$(CONFIG_SOC_CAMERA_PLATFORM)	+= soc_camera_platform.o
> diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
> new file mode 100644
> index 0000000..b91f0f6
> --- /dev/null
> +++ b/drivers/media/video/ak881x.c
> @@ -0,0 +1,360 @@
> +/*
> + * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> + *
> + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * 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.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/ak881x.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-device.h>
> +
> +#define AK881X_INTERFACE_MODE	0
> +#define AK881X_VIDEO_PROCESS1	1
> +#define AK881X_VIDEO_PROCESS2	2
> +#define AK881X_VIDEO_PROCESS3	3
> +#define AK881X_DAC_MODE		5
> +#define AK881X_STATUS		0x24
> +#define AK881X_DEVICE_ID	0x25
> +#define AK881X_DEVICE_REVISION	0x26
> +
> +struct ak881x {
> +	struct v4l2_subdev subdev;
> +	struct ak881x_pdata *pdata;
> +	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
> +	char revision;	/* DEVICE_REVISION content */
> +};
> +
> +static int reg_read(struct i2c_client *client, const u8 reg)
> +{
> +	return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int reg_write(struct i2c_client *client, const u8 reg,
> +		     const u8 data)
> +{
> +	return i2c_smbus_write_byte_data(client, reg, data);
> +}

I suggest making these inline.

I also recommend using struct v4l2_subdev instead of struct i2c_client as
argument. In my experience it makes the code cleaner if the mapping from
subdev to i2c_client is done at the lowest possible level.

> +
> +static int reg_set(struct i2c_client *client, const u8 reg,
> +		   const u8 data, u8 mask)
> +{
> +	int ret = reg_read(client, reg);
> +	if (ret < 0)
> +		return ret;
> +	return reg_write(client, reg, (ret & ~mask) | (data & mask));
> +}
> +
> +static struct ak881x *to_ak881x(const struct i2c_client *client)
> +{
> +	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
> +}

Ditto for this one.

> +
> +static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
> +				struct v4l2_dbg_chip_ident *id)
> +{
> +	struct i2c_client *client = sd->priv;

Don't use sd->priv directly. Use v4l2_get_subdevdata(sd) instead.

> +	struct ak881x *ak881x = to_ak881x(client);
> +
> +	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
> +		return -EINVAL;
> +
> +	if (id->match.addr != client->addr)
> +		return -ENODEV;
> +
> +	id->ident	= ak881x->id;
> +	id->revision	= ak881x->revision;
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int ak881x_g_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = sd->priv;
> +
> +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> +		return -EINVAL;
> +
> +	if (reg->match.addr != client->addr)
> +		return -ENODEV;
> +
> +	reg->val = reg_read(client, reg->reg);
> +
> +	if (reg->val > 0xffff)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = sd->priv;
> +
> +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> +		return -EINVAL;
> +
> +	if (reg->match.addr != client->addr)
> +		return -ENODEV;
> +
> +	if (reg_write(client, reg->reg, reg->val) < 0)
> +		return -EIO;
> +
> +	return 0;
> +}
> +#endif
> +
> +static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
> +			    struct v4l2_mbus_framefmt *mf)

Can you rename this function to ak881x_try_g_mbus_fmt? Same for the other
fmt functions.

> +{
> +	v4l_bound_align_image(&mf->width, 0, 720, 2,
> +			      &mf->height, 0, 480, 1, 0);

480? Doesn't this do 576 as well when PAL is selected?

> +	mf->field	= V4L2_FIELD_INTERLACED;
> +	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
> +	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_fmt(struct v4l2_subdev *sd,
> +			 struct v4l2_mbus_framefmt *mf)
> +{
> +	if (mf->field != V4L2_FIELD_INTERLACED ||
> +	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
> +		return -EINVAL;
> +
> +	return ak881x_try_g_fmt(sd, mf);
> +}
> +
> +static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
> +			    enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
> +	return 0;
> +}
> +
> +static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	a->bounds.left			= 0;
> +	a->bounds.top			= 0;
> +	a->bounds.width			= 720;
> +	a->bounds.height		= 240 * 2;

288 * 2 for PAL?

> +	a->defrect			= a->bounds;
> +	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +	a->pixelaspect.numerator	= 1;
> +	a->pixelaspect.denominator	= 1;
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
> +{
> +	struct i2c_client *client = sd->priv;
> +	u8 vp1;
> +
> +	switch (std) {
> +	case V4L2_STD_NTSC_M:
> +	default:
> +		vp1 = 0;
> +		break;
> +	case V4L2_STD_NTSC_443:
> +		vp1 = 3;
> +		break;
> +	case V4L2_STD_PAL_M:
> +		vp1 = 5;
> +		break;
> +	case V4L2_STD_PAL_60:
> +		vp1 = 7;
> +		break;
> +	case V4L2_STD_PAL_B:
> +	case V4L2_STD_PAL_D:
> +	case V4L2_STD_PAL_G:
> +	case V4L2_STD_PAL_H:
> +	case V4L2_STD_PAL_I:
> +		vp1 = 0xf;
> +	}
> +
> +	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
> +
> +	return 0;
> +}
> +
> +static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct i2c_client *client = sd->priv;
> +	struct ak881x *ak881x = to_ak881x(client);
> +
> +	if (enable) {
> +		u8 dac;
> +		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
> +		/* Default: composite output */
> +		if (ak881x->pdata->flags & AK881X_COMPONENT)
> +			dac = 3;
> +		else
> +			dac = 4;
> +		/* Turn on the DAC(s) */
> +		reg_write(client, AK881X_DAC_MODE, dac);
> +		dev_dbg(&client->dev, "chip status 0x%x\n",
> +			reg_read(client, AK881X_STATUS));
> +	} else {
> +		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
> +		reg_write(client, AK881X_DAC_MODE, 0);
> +		dev_dbg(&client->dev, "chip status 0x%x\n",
> +			reg_read(client, AK881X_STATUS));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
> +	.g_chip_ident	= ak881x_g_chip_ident,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	.g_register	= ak881x_g_register,
> +	.s_register	= ak881x_s_register,
> +#endif
> +};
> +
> +static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
> +	.s_mbus_fmt	= ak881x_s_fmt,
> +	.g_mbus_fmt	= ak881x_try_g_fmt,
> +	.try_mbus_fmt	= ak881x_try_g_fmt,
> +	.cropcap	= ak881x_cropcap,
> +	.enum_mbus_fmt	= ak881x_enum_fmt,
> +	.s_std_output	= ak881x_s_std_output,
> +	.s_stream	= ak881x_s_stream,
> +};
> +
> +static struct v4l2_subdev_ops ak881x_subdev_ops = {
> +	.core	= &ak881x_subdev_core_ops,
> +	.video	= &ak881x_subdev_video_ops,
> +};
> +
> +static int ak881x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *did)
> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	struct ak881x *ak881x;
> +	u8 ifmode, data;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		dev_warn(&adapter->dev,
> +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> +		return -EIO;
> +	}
> +
> +	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
> +	if (!ak881x)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
> +
> +	data = reg_read(client, AK881X_DEVICE_ID);
> +
> +	switch (data) {
> +	case 0x13:
> +		ak881x->id = V4L2_IDENT_AK8813;
> +		break;
> +	case 0x14:
> +		ak881x->id = V4L2_IDENT_AK8814;
> +		break;
> +	default:
> +		dev_err(&client->dev,
> +			"No ak881x chip detected, register read %x\n", data);
> +		i2c_set_clientdata(client, NULL);

No need to call i2c_set_clientdata here.

> +		kfree(ak881x);
> +		return -ENODEV;
> +	}
> +
> +	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
> +	ak881x->pdata = client->dev.platform_data;
> +
> +	if (ak881x->pdata) {
> +		if (ak881x->pdata->flags & AK881X_FIELD)
> +			ifmode = 4;
> +		else
> +			ifmode = 0;
> +
> +		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
> +		case AK881X_IF_MODE_BT656:
> +			ifmode |= 1;
> +			break;
> +		case AK881X_IF_MODE_MASTER:
> +			ifmode |= 2;
> +			break;
> +		case AK881X_IF_MODE_SLAVE:
> +		default:
> +			break;
> +		}
> +
> +		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
> +
> +		/*
> +		 * "Line Blanking No." seems to be the same as the number of
> +		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
> +		 * "incidentally" matches ak881x' default
> +		 */
> +		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
> +	}
> +
> +	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
> +		 data, ak881x->revision);

Please use this instead:

        v4l2_info(&ak881x->subdev, "chip found @ 0x%02x (%s, revision %x)\n",
                 client->addr << 1, client->adapter->name, ak881x->revision);

This is for consistency with other i2c v4l drivers.

Please also use v4l2_info/warn/err/dbg instead of the dev_ versions (except if
there is no subdev pointer available). Again for consistency with other i2c
drivers (and a more concise prefix as well).


> +
> +	return 0;
> +}
> +
> +static int ak881x_remove(struct i2c_client *client)
> +{
> +	struct ak881x *ak881x = to_ak881x(client);
> +
> +	i2c_set_clientdata(client, NULL);

This is not right. Use this instead:

        v4l2_device_unregister_subdev(sd);

See v4l2-framework.txt why you should do this in remove().

> +	kfree(ak881x);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ak881x_id[] = {
> +	{ "ak8813", 0 },
> +	{ "ak8814", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ak881x_id);
> +
> +static struct i2c_driver ak881x_i2c_driver = {
> +	.driver = {
> +		.name = "ak881x",
> +	},
> +	.probe		= ak881x_probe,
> +	.remove		= ak881x_remove,
> +	.id_table	= ak881x_id,
> +};
> +
> +static int __init ak881x_module_init(void)
> +{
> +	return i2c_add_driver(&ak881x_i2c_driver);
> +}
> +
> +static void __exit ak881x_module_exit(void)
> +{
> +	i2c_del_driver(&ak881x_i2c_driver);
> +}
> +
> +module_init(ak881x_module_init);
> +module_exit(ak881x_module_exit);
> +
> +MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
> +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/media/ak881x.h b/include/media/ak881x.h
> new file mode 100644
> index 0000000..b7f2add
> --- /dev/null
> +++ b/include/media/ak881x.h
> @@ -0,0 +1,25 @@
> +/*
> + * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> + *
> + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * 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.
> + */
> +
> +#ifndef AK881X_H
> +#define AK881X_H
> +
> +#define AK881X_IF_MODE_MASK	(3 << 0)
> +#define AK881X_IF_MODE_BT656	(0 << 0)
> +#define AK881X_IF_MODE_MASTER	(1 << 0)
> +#define AK881X_IF_MODE_SLAVE	(2 << 0)
> +#define AK881X_FIELD		(1 << 2)
> +#define AK881X_COMPONENT	(1 << 3)
> +
> +struct ak881x_pdata {
> +	unsigned long flags;

Why unsigned long? u32 makes more sense. For 64-bit archs unsigned long is
64 bits.

> +};
> +
> +#endif
> diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> index 6cc107d..5d7b742 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -289,6 +289,10 @@ enum {
>  
>  	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
>  	V4L2_IDENT_RJ54N1CB0C = 51980,
> +
> +	/* AKM AK8813/AK8814 */
> +	V4L2_IDENT_AK8813 = 8813,
> +	V4L2_IDENT_AK8814 = 8814,

The IDs in v4l2-chip-ident.h should be kept in increasing numeric order. I see
that several are already placed out of order. I'm going to make a patch for
that and then you can add these new IDs in the right place.

>  };
>  
>  #endif
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders

[Guennadi Liakhovetski]

On Sun, 14 Mar 2010, Hans Verkuil wrote:

> Review notes below...
> 
> On Thursday 11 March 2010 11:25:42 Guennadi Liakhovetski wrote:
> > AK8814 only differs from AK8813 by included Macrovision Copy Protection
> > function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
> > encoders.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---

[snip]

> > diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
> > new file mode 100644
> > index 0000000..b91f0f6
> > --- /dev/null
> > +++ b/drivers/media/video/ak881x.c
> > @@ -0,0 +1,360 @@
> > +/*
> > + * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > + *
> > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/videodev2.h>
> > +
> > +#include <media/ak881x.h>
> > +#include <media/v4l2-chip-ident.h>
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-device.h>
> > +
> > +#define AK881X_INTERFACE_MODE	0
> > +#define AK881X_VIDEO_PROCESS1	1
> > +#define AK881X_VIDEO_PROCESS2	2
> > +#define AK881X_VIDEO_PROCESS3	3
> > +#define AK881X_DAC_MODE		5
> > +#define AK881X_STATUS		0x24
> > +#define AK881X_DEVICE_ID	0x25
> > +#define AK881X_DEVICE_REVISION	0x26
> > +
> > +struct ak881x {
> > +	struct v4l2_subdev subdev;
> > +	struct ak881x_pdata *pdata;
> > +	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
> > +	char revision;	/* DEVICE_REVISION content */
> > +};
> > +
> > +static int reg_read(struct i2c_client *client, const u8 reg)
> > +{
> > +	return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +static int reg_write(struct i2c_client *client, const u8 reg,
> > +		     const u8 data)
> > +{
> > +	return i2c_smbus_write_byte_data(client, reg, data);
> > +}
> 
> I suggest making these inline.

Disagree. It has been advised on the LKML to _not_ use inline in .c files 
- the compiler decides itself, and it does trivially inline these.

> I also recommend using struct v4l2_subdev instead of struct i2c_client as
> argument. In my experience it makes the code cleaner if the mapping from
> subdev to i2c_client is done at the lowest possible level.

May I disagree with this one too?;) Just for a mere reason, that in this 
specific case, register-access routines should not need anything except the 
i2c-client - by desiign.

> > +
> > +static int reg_set(struct i2c_client *client, const u8 reg,
> > +		   const u8 data, u8 mask)
> > +{
> > +	int ret = reg_read(client, reg);
> > +	if (ret < 0)
> > +		return ret;
> > +	return reg_write(client, reg, (ret & ~mask) | (data & mask));
> > +}
> > +
> > +static struct ak881x *to_ak881x(const struct i2c_client *client)
> > +{
> > +	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
> > +}
> 
> Ditto for this one.
> 
> > +
> > +static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
> > +				struct v4l2_dbg_chip_ident *id)
> > +{
> > +	struct i2c_client *client = sd->priv;
> 
> Don't use sd->priv directly. Use v4l2_get_subdevdata(sd) instead.

Ok.

> > +	struct ak881x *ak881x = to_ak881x(client);
> > +
> > +	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
> > +		return -EINVAL;
> > +
> > +	if (id->match.addr != client->addr)
> > +		return -ENODEV;
> > +
> > +	id->ident	= ak881x->id;
> > +	id->revision	= ak881x->revision;
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +static int ak881x_g_register(struct v4l2_subdev *sd,
> > +			      struct v4l2_dbg_register *reg)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +
> > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > +		return -EINVAL;
> > +
> > +	if (reg->match.addr != client->addr)
> > +		return -ENODEV;
> > +
> > +	reg->val = reg_read(client, reg->reg);
> > +
> > +	if (reg->val > 0xffff)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_register(struct v4l2_subdev *sd,
> > +			      struct v4l2_dbg_register *reg)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +
> > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > +		return -EINVAL;
> > +
> > +	if (reg->match.addr != client->addr)
> > +		return -ENODEV;
> > +
> > +	if (reg_write(client, reg->reg, reg->val) < 0)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
> > +			    struct v4l2_mbus_framefmt *mf)
> 
> Can you rename this function to ak881x_try_g_mbus_fmt? Same for the other
> fmt functions.

Well, we (including you) wanted to eventually convert all subdev drivers 
to mediabus, and then to rename *_mbus_fmt to just *_fmt. That's why I 
kept these names without "mbus" also in all soc-camera drivers, as I was 
converting them to mediabus. So, I would prefer to keep this here too.

> > +{
> > +	v4l_bound_align_image(&mf->width, 0, 720, 2,
> > +			      &mf->height, 0, 480, 1, 0);
> 
> 480? Doesn't this do 576 as well when PAL is selected?

Right, it, probably, can.

> > +	mf->field	= V4L2_FIELD_INTERLACED;
> > +	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > +	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_fmt(struct v4l2_subdev *sd,
> > +			 struct v4l2_mbus_framefmt *mf)
> > +{
> > +	if (mf->field != V4L2_FIELD_INTERLACED ||
> > +	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
> > +		return -EINVAL;
> > +
> > +	return ak881x_try_g_fmt(sd, mf);
> > +}
> > +
> > +static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
> > +			    enum v4l2_mbus_pixelcode *code)
> > +{
> > +	if (index)
> > +		return -EINVAL;
> > +
> > +	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > +	return 0;
> > +}
> > +
> > +static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> > +{
> > +	a->bounds.left			= 0;
> > +	a->bounds.top			= 0;
> > +	a->bounds.width			= 720;
> > +	a->bounds.height		= 240 * 2;
> 
> 288 * 2 for PAL?

Yup.

> > +	a->defrect			= a->bounds;
> > +	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > +	a->pixelaspect.numerator	= 1;
> > +	a->pixelaspect.denominator	= 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +	u8 vp1;
> > +
> > +	switch (std) {
> > +	case V4L2_STD_NTSC_M:
> > +	default:
> > +		vp1 = 0;
> > +		break;
> > +	case V4L2_STD_NTSC_443:
> > +		vp1 = 3;
> > +		break;
> > +	case V4L2_STD_PAL_M:
> > +		vp1 = 5;
> > +		break;
> > +	case V4L2_STD_PAL_60:
> > +		vp1 = 7;
> > +		break;
> > +	case V4L2_STD_PAL_B:
> > +	case V4L2_STD_PAL_D:
> > +	case V4L2_STD_PAL_G:
> > +	case V4L2_STD_PAL_H:
> > +	case V4L2_STD_PAL_I:
> > +		vp1 = 0xf;
> > +	}
> > +
> > +	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +	struct ak881x *ak881x = to_ak881x(client);
> > +
> > +	if (enable) {
> > +		u8 dac;
> > +		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
> > +		/* Default: composite output */
> > +		if (ak881x->pdata->flags & AK881X_COMPONENT)
> > +			dac = 3;
> > +		else
> > +			dac = 4;
> > +		/* Turn on the DAC(s) */
> > +		reg_write(client, AK881X_DAC_MODE, dac);
> > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > +			reg_read(client, AK881X_STATUS));
> > +	} else {
> > +		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
> > +		reg_write(client, AK881X_DAC_MODE, 0);
> > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > +			reg_read(client, AK881X_STATUS));
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
> > +	.g_chip_ident	= ak881x_g_chip_ident,
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +	.g_register	= ak881x_g_register,
> > +	.s_register	= ak881x_s_register,
> > +#endif
> > +};
> > +
> > +static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
> > +	.s_mbus_fmt	= ak881x_s_fmt,
> > +	.g_mbus_fmt	= ak881x_try_g_fmt,
> > +	.try_mbus_fmt	= ak881x_try_g_fmt,
> > +	.cropcap	= ak881x_cropcap,
> > +	.enum_mbus_fmt	= ak881x_enum_fmt,
> > +	.s_std_output	= ak881x_s_std_output,
> > +	.s_stream	= ak881x_s_stream,
> > +};
> > +
> > +static struct v4l2_subdev_ops ak881x_subdev_ops = {
> > +	.core	= &ak881x_subdev_core_ops,
> > +	.video	= &ak881x_subdev_video_ops,
> > +};
> > +
> > +static int ak881x_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *did)
> > +{
> > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > +	struct ak881x *ak881x;
> > +	u8 ifmode, data;
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		dev_warn(&adapter->dev,
> > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > +		return -EIO;
> > +	}
> > +
> > +	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
> > +	if (!ak881x)
> > +		return -ENOMEM;
> > +
> > +	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
> > +
> > +	data = reg_read(client, AK881X_DEVICE_ID);
> > +
> > +	switch (data) {
> > +	case 0x13:
> > +		ak881x->id = V4L2_IDENT_AK8813;
> > +		break;
> > +	case 0x14:
> > +		ak881x->id = V4L2_IDENT_AK8814;
> > +		break;
> > +	default:
> > +		dev_err(&client->dev,
> > +			"No ak881x chip detected, register read %x\n", data);
> > +		i2c_set_clientdata(client, NULL);
> 
> No need to call i2c_set_clientdata here.

Ok.

> > +		kfree(ak881x);
> > +		return -ENODEV;
> > +	}
> > +
> > +	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
> > +	ak881x->pdata = client->dev.platform_data;
> > +
> > +	if (ak881x->pdata) {
> > +		if (ak881x->pdata->flags & AK881X_FIELD)
> > +			ifmode = 4;
> > +		else
> > +			ifmode = 0;
> > +
> > +		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
> > +		case AK881X_IF_MODE_BT656:
> > +			ifmode |= 1;
> > +			break;
> > +		case AK881X_IF_MODE_MASTER:
> > +			ifmode |= 2;
> > +			break;
> > +		case AK881X_IF_MODE_SLAVE:
> > +		default:
> > +			break;
> > +		}
> > +
> > +		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
> > +
> > +		/*
> > +		 * "Line Blanking No." seems to be the same as the number of
> > +		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
> > +		 * "incidentally" matches ak881x' default
> > +		 */
> > +		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
> > +	}
> > +
> > +	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
> > +		 data, ak881x->revision);
> 
> Please use this instead:
> 
>         v4l2_info(&ak881x->subdev, "chip found @ 0x%02x (%s, revision %x)\n",
>                  client->addr << 1, client->adapter->name, ak881x->revision);
> 
> This is for consistency with other i2c v4l drivers.
> 
> Please also use v4l2_info/warn/err/dbg instead of the dev_ versions (except if
> there is no subdev pointer available). Again for consistency with other i2c
> drivers (and a more concise prefix as well).

Ooh, why do we have to be special?... Firstly, it is a Linux convention to 
specify I2C device addresses without the read / write bit. Granted it has 
introduced enough confusion, but don't we make it even worse with this?... 
Secondly, I didn't like these macros as I first saw them, and I still 
don't like them for a very simple reason: do all driver subsystems now 
have to introduce their own *_printk versions? usb_info()? pci_info()? 
ata_info()? net_info()?... Don't we have dev_* exactly for this purpose - 
to be used by all drivers, regardless of the subsystem? Can we maybe 
rethink this approach, shall we?

> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_remove(struct i2c_client *client)
> > +{
> > +	struct ak881x *ak881x = to_ak881x(client);
> > +
> > +	i2c_set_clientdata(client, NULL);
> 
> This is not right. Use this instead:
> 
>         v4l2_device_unregister_subdev(sd);
> 
> See v4l2-framework.txt why you should do this in remove().

Ok.

> > +	kfree(ak881x);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id ak881x_id[] = {
> > +	{ "ak8813", 0 },
> > +	{ "ak8814", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ak881x_id);
> > +
> > +static struct i2c_driver ak881x_i2c_driver = {
> > +	.driver = {
> > +		.name = "ak881x",
> > +	},
> > +	.probe		= ak881x_probe,
> > +	.remove		= ak881x_remove,
> > +	.id_table	= ak881x_id,
> > +};
> > +
> > +static int __init ak881x_module_init(void)
> > +{
> > +	return i2c_add_driver(&ak881x_i2c_driver);
> > +}
> > +
> > +static void __exit ak881x_module_exit(void)
> > +{
> > +	i2c_del_driver(&ak881x_i2c_driver);
> > +}
> > +
> > +module_init(ak881x_module_init);
> > +module_exit(ak881x_module_exit);
> > +
> > +MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
> > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/media/ak881x.h b/include/media/ak881x.h
> > new file mode 100644
> > index 0000000..b7f2add
> > --- /dev/null
> > +++ b/include/media/ak881x.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > + *
> > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef AK881X_H
> > +#define AK881X_H
> > +
> > +#define AK881X_IF_MODE_MASK	(3 << 0)
> > +#define AK881X_IF_MODE_BT656	(0 << 0)
> > +#define AK881X_IF_MODE_MASTER	(1 << 0)
> > +#define AK881X_IF_MODE_SLAVE	(2 << 0)
> > +#define AK881X_FIELD		(1 << 2)
> > +#define AK881X_COMPONENT	(1 << 3)
> > +
> > +struct ak881x_pdata {
> > +	unsigned long flags;
> 
> Why unsigned long? u32 makes more sense. For 64-bit archs unsigned long is
> 64 bits.

Disagree. I'm trying to follow the rule to only use fixed-bitsize types 
where needed, e.g., where values, they describe, correspond to some (fixed 
width) hardware registers, or are a part of an ABI. In all other cases I 
prefer to use C-native types. So, if you really want to save those 32 bits 
per ak881x device on hypothetical 64-bit systems where it can ever by used 
- we can make it unsigned int, otherwise, using a long for flags has 
become more or less a tradition, I think.

> > +};
> > +
> > +#endif
> > diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> > index 6cc107d..5d7b742 100644
> > --- a/include/media/v4l2-chip-ident.h
> > +++ b/include/media/v4l2-chip-ident.h
> > @@ -289,6 +289,10 @@ enum {
> >  
> >  	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
> >  	V4L2_IDENT_RJ54N1CB0C = 51980,
> > +
> > +	/* AKM AK8813/AK8814 */
> > +	V4L2_IDENT_AK8813 = 8813,
> > +	V4L2_IDENT_AK8814 = 8814,
> 
> The IDs in v4l2-chip-ident.h should be kept in increasing numeric order. I see
> that several are already placed out of order. I'm going to make a patch for
> that and then you can add these new IDs in the right place.

Yep, will do.

> >  };
> >  
> >  #endif
> > 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders from AKM

[Guennadi Liakhovetski]

On Sun, 14 Mar 2010, Hans Verkuil wrote:

> Review notes below...
> 
> On Thursday 11 March 2010 11:25:42 Guennadi Liakhovetski wrote:
> > AK8814 only differs from AK8813 by included Macrovision Copy Protection
> > function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
> > encoders.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---

[snip]

> > diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
> > new file mode 100644
> > index 0000000..b91f0f6
> > --- /dev/null
> > +++ b/drivers/media/video/ak881x.c
> > @@ -0,0 +1,360 @@
> > +/*
> > + * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > + *
> > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/videodev2.h>
> > +
> > +#include <media/ak881x.h>
> > +#include <media/v4l2-chip-ident.h>
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-device.h>
> > +
> > +#define AK881X_INTERFACE_MODE	0
> > +#define AK881X_VIDEO_PROCESS1	1
> > +#define AK881X_VIDEO_PROCESS2	2
> > +#define AK881X_VIDEO_PROCESS3	3
> > +#define AK881X_DAC_MODE		5
> > +#define AK881X_STATUS		0x24
> > +#define AK881X_DEVICE_ID	0x25
> > +#define AK881X_DEVICE_REVISION	0x26
> > +
> > +struct ak881x {
> > +	struct v4l2_subdev subdev;
> > +	struct ak881x_pdata *pdata;
> > +	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
> > +	char revision;	/* DEVICE_REVISION content */
> > +};
> > +
> > +static int reg_read(struct i2c_client *client, const u8 reg)
> > +{
> > +	return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +static int reg_write(struct i2c_client *client, const u8 reg,
> > +		     const u8 data)
> > +{
> > +	return i2c_smbus_write_byte_data(client, reg, data);
> > +}
> 
> I suggest making these inline.

Disagree. It has been advised on the LKML to _not_ use inline in .c files 
- the compiler decides itself, and it does trivially inline these.

> I also recommend using struct v4l2_subdev instead of struct i2c_client as
> argument. In my experience it makes the code cleaner if the mapping from
> subdev to i2c_client is done at the lowest possible level.

May I disagree with this one too?;) Just for a mere reason, that in this 
specific case, register-access routines should not need anything except the 
i2c-client - by desiign.

> > +
> > +static int reg_set(struct i2c_client *client, const u8 reg,
> > +		   const u8 data, u8 mask)
> > +{
> > +	int ret = reg_read(client, reg);
> > +	if (ret < 0)
> > +		return ret;
> > +	return reg_write(client, reg, (ret & ~mask) | (data & mask));
> > +}
> > +
> > +static struct ak881x *to_ak881x(const struct i2c_client *client)
> > +{
> > +	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
> > +}
> 
> Ditto for this one.
> 
> > +
> > +static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
> > +				struct v4l2_dbg_chip_ident *id)
> > +{
> > +	struct i2c_client *client = sd->priv;
> 
> Don't use sd->priv directly. Use v4l2_get_subdevdata(sd) instead.

Ok.

> > +	struct ak881x *ak881x = to_ak881x(client);
> > +
> > +	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
> > +		return -EINVAL;
> > +
> > +	if (id->match.addr != client->addr)
> > +		return -ENODEV;
> > +
> > +	id->ident	= ak881x->id;
> > +	id->revision	= ak881x->revision;
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +static int ak881x_g_register(struct v4l2_subdev *sd,
> > +			      struct v4l2_dbg_register *reg)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +
> > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > +		return -EINVAL;
> > +
> > +	if (reg->match.addr != client->addr)
> > +		return -ENODEV;
> > +
> > +	reg->val = reg_read(client, reg->reg);
> > +
> > +	if (reg->val > 0xffff)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_register(struct v4l2_subdev *sd,
> > +			      struct v4l2_dbg_register *reg)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +
> > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > +		return -EINVAL;
> > +
> > +	if (reg->match.addr != client->addr)
> > +		return -ENODEV;
> > +
> > +	if (reg_write(client, reg->reg, reg->val) < 0)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
> > +			    struct v4l2_mbus_framefmt *mf)
> 
> Can you rename this function to ak881x_try_g_mbus_fmt? Same for the other
> fmt functions.

Well, we (including you) wanted to eventually convert all subdev drivers 
to mediabus, and then to rename *_mbus_fmt to just *_fmt. That's why I 
kept these names without "mbus" also in all soc-camera drivers, as I was 
converting them to mediabus. So, I would prefer to keep this here too.

> > +{
> > +	v4l_bound_align_image(&mf->width, 0, 720, 2,
> > +			      &mf->height, 0, 480, 1, 0);
> 
> 480? Doesn't this do 576 as well when PAL is selected?

Right, it, probably, can.

> > +	mf->field	= V4L2_FIELD_INTERLACED;
> > +	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > +	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_fmt(struct v4l2_subdev *sd,
> > +			 struct v4l2_mbus_framefmt *mf)
> > +{
> > +	if (mf->field != V4L2_FIELD_INTERLACED ||
> > +	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
> > +		return -EINVAL;
> > +
> > +	return ak881x_try_g_fmt(sd, mf);
> > +}
> > +
> > +static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
> > +			    enum v4l2_mbus_pixelcode *code)
> > +{
> > +	if (index)
> > +		return -EINVAL;
> > +
> > +	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > +	return 0;
> > +}
> > +
> > +static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> > +{
> > +	a->bounds.left			= 0;
> > +	a->bounds.top			= 0;
> > +	a->bounds.width			= 720;
> > +	a->bounds.height		= 240 * 2;
> 
> 288 * 2 for PAL?

Yup.

> > +	a->defrect			= a->bounds;
> > +	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > +	a->pixelaspect.numerator	= 1;
> > +	a->pixelaspect.denominator	= 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +	u8 vp1;
> > +
> > +	switch (std) {
> > +	case V4L2_STD_NTSC_M:
> > +	default:
> > +		vp1 = 0;
> > +		break;
> > +	case V4L2_STD_NTSC_443:
> > +		vp1 = 3;
> > +		break;
> > +	case V4L2_STD_PAL_M:
> > +		vp1 = 5;
> > +		break;
> > +	case V4L2_STD_PAL_60:
> > +		vp1 = 7;
> > +		break;
> > +	case V4L2_STD_PAL_B:
> > +	case V4L2_STD_PAL_D:
> > +	case V4L2_STD_PAL_G:
> > +	case V4L2_STD_PAL_H:
> > +	case V4L2_STD_PAL_I:
> > +		vp1 = 0xf;
> > +	}
> > +
> > +	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +	struct ak881x *ak881x = to_ak881x(client);
> > +
> > +	if (enable) {
> > +		u8 dac;
> > +		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
> > +		/* Default: composite output */
> > +		if (ak881x->pdata->flags & AK881X_COMPONENT)
> > +			dac = 3;
> > +		else
> > +			dac = 4;
> > +		/* Turn on the DAC(s) */
> > +		reg_write(client, AK881X_DAC_MODE, dac);
> > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > +			reg_read(client, AK881X_STATUS));
> > +	} else {
> > +		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
> > +		reg_write(client, AK881X_DAC_MODE, 0);
> > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > +			reg_read(client, AK881X_STATUS));
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
> > +	.g_chip_ident	= ak881x_g_chip_ident,
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +	.g_register	= ak881x_g_register,
> > +	.s_register	= ak881x_s_register,
> > +#endif
> > +};
> > +
> > +static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
> > +	.s_mbus_fmt	= ak881x_s_fmt,
> > +	.g_mbus_fmt	= ak881x_try_g_fmt,
> > +	.try_mbus_fmt	= ak881x_try_g_fmt,
> > +	.cropcap	= ak881x_cropcap,
> > +	.enum_mbus_fmt	= ak881x_enum_fmt,
> > +	.s_std_output	= ak881x_s_std_output,
> > +	.s_stream	= ak881x_s_stream,
> > +};
> > +
> > +static struct v4l2_subdev_ops ak881x_subdev_ops = {
> > +	.core	= &ak881x_subdev_core_ops,
> > +	.video	= &ak881x_subdev_video_ops,
> > +};
> > +
> > +static int ak881x_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *did)
> > +{
> > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > +	struct ak881x *ak881x;
> > +	u8 ifmode, data;
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		dev_warn(&adapter->dev,
> > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > +		return -EIO;
> > +	}
> > +
> > +	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
> > +	if (!ak881x)
> > +		return -ENOMEM;
> > +
> > +	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
> > +
> > +	data = reg_read(client, AK881X_DEVICE_ID);
> > +
> > +	switch (data) {
> > +	case 0x13:
> > +		ak881x->id = V4L2_IDENT_AK8813;
> > +		break;
> > +	case 0x14:
> > +		ak881x->id = V4L2_IDENT_AK8814;
> > +		break;
> > +	default:
> > +		dev_err(&client->dev,
> > +			"No ak881x chip detected, register read %x\n", data);
> > +		i2c_set_clientdata(client, NULL);
> 
> No need to call i2c_set_clientdata here.

Ok.

> > +		kfree(ak881x);
> > +		return -ENODEV;
> > +	}
> > +
> > +	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
> > +	ak881x->pdata = client->dev.platform_data;
> > +
> > +	if (ak881x->pdata) {
> > +		if (ak881x->pdata->flags & AK881X_FIELD)
> > +			ifmode = 4;
> > +		else
> > +			ifmode = 0;
> > +
> > +		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
> > +		case AK881X_IF_MODE_BT656:
> > +			ifmode |= 1;
> > +			break;
> > +		case AK881X_IF_MODE_MASTER:
> > +			ifmode |= 2;
> > +			break;
> > +		case AK881X_IF_MODE_SLAVE:
> > +		default:
> > +			break;
> > +		}
> > +
> > +		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
> > +
> > +		/*
> > +		 * "Line Blanking No." seems to be the same as the number of
> > +		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
> > +		 * "incidentally" matches ak881x' default
> > +		 */
> > +		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
> > +	}
> > +
> > +	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
> > +		 data, ak881x->revision);
> 
> Please use this instead:
> 
>         v4l2_info(&ak881x->subdev, "chip found @ 0x%02x (%s, revision %x)\n",
>                  client->addr << 1, client->adapter->name, ak881x->revision);
> 
> This is for consistency with other i2c v4l drivers.
> 
> Please also use v4l2_info/warn/err/dbg instead of the dev_ versions (except if
> there is no subdev pointer available). Again for consistency with other i2c
> drivers (and a more concise prefix as well).

Ooh, why do we have to be special?... Firstly, it is a Linux convention to 
specify I2C device addresses without the read / write bit. Granted it has 
introduced enough confusion, but don't we make it even worse with this?... 
Secondly, I didn't like these macros as I first saw them, and I still 
don't like them for a very simple reason: do all driver subsystems now 
have to introduce their own *_printk versions? usb_info()? pci_info()? 
ata_info()? net_info()?... Don't we have dev_* exactly for this purpose - 
to be used by all drivers, regardless of the subsystem? Can we maybe 
rethink this approach, shall we?

> > +
> > +	return 0;
> > +}
> > +
> > +static int ak881x_remove(struct i2c_client *client)
> > +{
> > +	struct ak881x *ak881x = to_ak881x(client);
> > +
> > +	i2c_set_clientdata(client, NULL);
> 
> This is not right. Use this instead:
> 
>         v4l2_device_unregister_subdev(sd);
> 
> See v4l2-framework.txt why you should do this in remove().

Ok.

> > +	kfree(ak881x);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id ak881x_id[] = {
> > +	{ "ak8813", 0 },
> > +	{ "ak8814", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ak881x_id);
> > +
> > +static struct i2c_driver ak881x_i2c_driver = {
> > +	.driver = {
> > +		.name = "ak881x",
> > +	},
> > +	.probe		= ak881x_probe,
> > +	.remove		= ak881x_remove,
> > +	.id_table	= ak881x_id,
> > +};
> > +
> > +static int __init ak881x_module_init(void)
> > +{
> > +	return i2c_add_driver(&ak881x_i2c_driver);
> > +}
> > +
> > +static void __exit ak881x_module_exit(void)
> > +{
> > +	i2c_del_driver(&ak881x_i2c_driver);
> > +}
> > +
> > +module_init(ak881x_module_init);
> > +module_exit(ak881x_module_exit);
> > +
> > +MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
> > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/media/ak881x.h b/include/media/ak881x.h
> > new file mode 100644
> > index 0000000..b7f2add
> > --- /dev/null
> > +++ b/include/media/ak881x.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > + *
> > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef AK881X_H
> > +#define AK881X_H
> > +
> > +#define AK881X_IF_MODE_MASK	(3 << 0)
> > +#define AK881X_IF_MODE_BT656	(0 << 0)
> > +#define AK881X_IF_MODE_MASTER	(1 << 0)
> > +#define AK881X_IF_MODE_SLAVE	(2 << 0)
> > +#define AK881X_FIELD		(1 << 2)
> > +#define AK881X_COMPONENT	(1 << 3)
> > +
> > +struct ak881x_pdata {
> > +	unsigned long flags;
> 
> Why unsigned long? u32 makes more sense. For 64-bit archs unsigned long is
> 64 bits.

Disagree. I'm trying to follow the rule to only use fixed-bitsize types 
where needed, e.g., where values, they describe, correspond to some (fixed 
width) hardware registers, or are a part of an ABI. In all other cases I 
prefer to use C-native types. So, if you really want to save those 32 bits 
per ak881x device on hypothetical 64-bit systems where it can ever by used 
- we can make it unsigned int, otherwise, using a long for flags has 
become more or less a tradition, I think.

> > +};
> > +
> > +#endif
> > diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> > index 6cc107d..5d7b742 100644
> > --- a/include/media/v4l2-chip-ident.h
> > +++ b/include/media/v4l2-chip-ident.h
> > @@ -289,6 +289,10 @@ enum {
> >  
> >  	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
> >  	V4L2_IDENT_RJ54N1CB0C = 51980,
> > +
> > +	/* AKM AK8813/AK8814 */
> > +	V4L2_IDENT_AK8813 = 8813,
> > +	V4L2_IDENT_AK8814 = 8814,
> 
> The IDs in v4l2-chip-ident.h should be kept in increasing numeric order. I see
> that several are already placed out of order. I'm going to make a patch for
> that and then you can add these new IDs in the right place.

Yep, will do.

> >  };
> >  
> >  #endif
> > 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders from AKM

[Hans Verkuil]

On Monday 15 March 2010 09:56:35 Guennadi Liakhovetski wrote:
> On Sun, 14 Mar 2010, Hans Verkuil wrote:
> 
> > Review notes below...
> > 
> > On Thursday 11 March 2010 11:25:42 Guennadi Liakhovetski wrote:
> > > AK8814 only differs from AK8813 by included Macrovision Copy Protection
> > > function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
> > > encoders.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> 
> [snip]
> 
> > > diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
> > > new file mode 100644
> > > index 0000000..b91f0f6
> > > --- /dev/null
> > > +++ b/drivers/media/video/ak881x.c
> > > @@ -0,0 +1,360 @@
> > > +/*
> > > + * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > > + *
> > > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/videodev2.h>
> > > +
> > > +#include <media/ak881x.h>
> > > +#include <media/v4l2-chip-ident.h>
> > > +#include <media/v4l2-common.h>
> > > +#include <media/v4l2-device.h>
> > > +
> > > +#define AK881X_INTERFACE_MODE	0
> > > +#define AK881X_VIDEO_PROCESS1	1
> > > +#define AK881X_VIDEO_PROCESS2	2
> > > +#define AK881X_VIDEO_PROCESS3	3
> > > +#define AK881X_DAC_MODE		5
> > > +#define AK881X_STATUS		0x24
> > > +#define AK881X_DEVICE_ID	0x25
> > > +#define AK881X_DEVICE_REVISION	0x26
> > > +
> > > +struct ak881x {
> > > +	struct v4l2_subdev subdev;
> > > +	struct ak881x_pdata *pdata;
> > > +	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
> > > +	char revision;	/* DEVICE_REVISION content */
> > > +};
> > > +
> > > +static int reg_read(struct i2c_client *client, const u8 reg)
> > > +{
> > > +	return i2c_smbus_read_byte_data(client, reg);
> > > +}
> > > +
> > > +static int reg_write(struct i2c_client *client, const u8 reg,
> > > +		     const u8 data)
> > > +{
> > > +	return i2c_smbus_write_byte_data(client, reg, data);
> > > +}
> > 
> > I suggest making these inline.
> 
> Disagree. It has been advised on the LKML to _not_ use inline in .c files 
> - the compiler decides itself, and it does trivially inline these.

The latest CodingStyle document still contains this paragraph:

"A reasonable rule of thumb is to not put inline at functions that have more
than 3 lines of code in them. An exception to this rule are the cases where
a parameter is known to be a compiletime constant, and as a result of this
constantness you *know* the compiler will be able to optimize most of your
function away at compile time. For a good example of this later case, see
the kmalloc() inline function."

So that's what I'm following.

> 
> > I also recommend using struct v4l2_subdev instead of struct i2c_client as
> > argument. In my experience it makes the code cleaner if the mapping from
> > subdev to i2c_client is done at the lowest possible level.
> 
> May I disagree with this one too?;) Just for a mere reason, that in this 
> specific case, register-access routines should not need anything except the 
> i2c-client - by desiign.

It was just a suggestion. I prefer to get the i2c_client pointer only there
where it is actually needed, rather then all over the source. It's not a
blocking issue for me, just the voice of experience :-)

> 
> > > +
> > > +static int reg_set(struct i2c_client *client, const u8 reg,
> > > +		   const u8 data, u8 mask)
> > > +{
> > > +	int ret = reg_read(client, reg);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	return reg_write(client, reg, (ret & ~mask) | (data & mask));
> > > +}
> > > +
> > > +static struct ak881x *to_ak881x(const struct i2c_client *client)
> > > +{
> > > +	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
> > > +}
> > 
> > Ditto for this one.
> > 
> > > +
> > > +static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
> > > +				struct v4l2_dbg_chip_ident *id)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > 
> > Don't use sd->priv directly. Use v4l2_get_subdevdata(sd) instead.
> 
> Ok.
> 
> > > +	struct ak881x *ak881x = to_ak881x(client);
> > > +
> > > +	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
> > > +		return -EINVAL;
> > > +
> > > +	if (id->match.addr != client->addr)
> > > +		return -ENODEV;
> > > +
> > > +	id->ident	= ak881x->id;
> > > +	id->revision	= ak881x->revision;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > > +static int ak881x_g_register(struct v4l2_subdev *sd,
> > > +			      struct v4l2_dbg_register *reg)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +
> > > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > > +		return -EINVAL;
> > > +
> > > +	if (reg->match.addr != client->addr)
> > > +		return -ENODEV;
> > > +
> > > +	reg->val = reg_read(client, reg->reg);
> > > +
> > > +	if (reg->val > 0xffff)
> > > +		return -EIO;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_register(struct v4l2_subdev *sd,
> > > +			      struct v4l2_dbg_register *reg)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +
> > > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > > +		return -EINVAL;
> > > +
> > > +	if (reg->match.addr != client->addr)
> > > +		return -ENODEV;
> > > +
> > > +	if (reg_write(client, reg->reg, reg->val) < 0)
> > > +		return -EIO;
> > > +
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > > +static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
> > > +			    struct v4l2_mbus_framefmt *mf)
> > 
> > Can you rename this function to ak881x_try_g_mbus_fmt? Same for the other
> > fmt functions.
> 
> Well, we (including you) wanted to eventually convert all subdev drivers 
> to mediabus, 

True. I've actually started to work on that.

> and then to rename *_mbus_fmt to just *_fmt.

If I ever said that (I might have, I can't remember), then I have definitely
changed my mind on that. It's about the mediabus so that fact should be part
of the name.

> That's why I 
> kept these names without "mbus" also in all soc-camera drivers, as I was 
> converting them to mediabus. So, I would prefer to keep this here too.
> 
> > > +{
> > > +	v4l_bound_align_image(&mf->width, 0, 720, 2,
> > > +			      &mf->height, 0, 480, 1, 0);
> > 
> > 480? Doesn't this do 576 as well when PAL is selected?
> 
> Right, it, probably, can.
> 
> > > +	mf->field	= V4L2_FIELD_INTERLACED;
> > > +	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > > +	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_fmt(struct v4l2_subdev *sd,
> > > +			 struct v4l2_mbus_framefmt *mf)
> > > +{
> > > +	if (mf->field != V4L2_FIELD_INTERLACED ||
> > > +	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
> > > +		return -EINVAL;
> > > +
> > > +	return ak881x_try_g_fmt(sd, mf);
> > > +}
> > > +
> > > +static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
> > > +			    enum v4l2_mbus_pixelcode *code)
> > > +{
> > > +	if (index)
> > > +		return -EINVAL;
> > > +
> > > +	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> > > +{
> > > +	a->bounds.left			= 0;
> > > +	a->bounds.top			= 0;
> > > +	a->bounds.width			= 720;
> > > +	a->bounds.height		= 240 * 2;
> > 
> > 288 * 2 for PAL?
> 
> Yup.
> 
> > > +	a->defrect			= a->bounds;
> > > +	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > +	a->pixelaspect.numerator	= 1;
> > > +	a->pixelaspect.denominator	= 1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +	u8 vp1;
> > > +
> > > +	switch (std) {
> > > +	case V4L2_STD_NTSC_M:
> > > +	default:
> > > +		vp1 = 0;
> > > +		break;
> > > +	case V4L2_STD_NTSC_443:
> > > +		vp1 = 3;
> > > +		break;
> > > +	case V4L2_STD_PAL_M:
> > > +		vp1 = 5;
> > > +		break;
> > > +	case V4L2_STD_PAL_60:
> > > +		vp1 = 7;
> > > +		break;
> > > +	case V4L2_STD_PAL_B:
> > > +	case V4L2_STD_PAL_D:
> > > +	case V4L2_STD_PAL_G:
> > > +	case V4L2_STD_PAL_H:
> > > +	case V4L2_STD_PAL_I:
> > > +		vp1 = 0xf;
> > > +	}
> > > +
> > > +	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +	struct ak881x *ak881x = to_ak881x(client);
> > > +
> > > +	if (enable) {
> > > +		u8 dac;
> > > +		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
> > > +		/* Default: composite output */
> > > +		if (ak881x->pdata->flags & AK881X_COMPONENT)
> > > +			dac = 3;
> > > +		else
> > > +			dac = 4;
> > > +		/* Turn on the DAC(s) */
> > > +		reg_write(client, AK881X_DAC_MODE, dac);
> > > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > > +			reg_read(client, AK881X_STATUS));
> > > +	} else {
> > > +		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
> > > +		reg_write(client, AK881X_DAC_MODE, 0);
> > > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > > +			reg_read(client, AK881X_STATUS));
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
> > > +	.g_chip_ident	= ak881x_g_chip_ident,
> > > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > > +	.g_register	= ak881x_g_register,
> > > +	.s_register	= ak881x_s_register,
> > > +#endif
> > > +};
> > > +
> > > +static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
> > > +	.s_mbus_fmt	= ak881x_s_fmt,
> > > +	.g_mbus_fmt	= ak881x_try_g_fmt,
> > > +	.try_mbus_fmt	= ak881x_try_g_fmt,
> > > +	.cropcap	= ak881x_cropcap,
> > > +	.enum_mbus_fmt	= ak881x_enum_fmt,
> > > +	.s_std_output	= ak881x_s_std_output,
> > > +	.s_stream	= ak881x_s_stream,
> > > +};
> > > +
> > > +static struct v4l2_subdev_ops ak881x_subdev_ops = {
> > > +	.core	= &ak881x_subdev_core_ops,
> > > +	.video	= &ak881x_subdev_video_ops,
> > > +};
> > > +
> > > +static int ak881x_probe(struct i2c_client *client,
> > > +			const struct i2c_device_id *did)
> > > +{
> > > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > > +	struct ak881x *ak881x;
> > > +	u8 ifmode, data;
> > > +
> > > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > > +		dev_warn(&adapter->dev,
> > > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
> > > +	if (!ak881x)
> > > +		return -ENOMEM;
> > > +
> > > +	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
> > > +
> > > +	data = reg_read(client, AK881X_DEVICE_ID);
> > > +
> > > +	switch (data) {
> > > +	case 0x13:
> > > +		ak881x->id = V4L2_IDENT_AK8813;
> > > +		break;
> > > +	case 0x14:
> > > +		ak881x->id = V4L2_IDENT_AK8814;
> > > +		break;
> > > +	default:
> > > +		dev_err(&client->dev,
> > > +			"No ak881x chip detected, register read %x\n", data);
> > > +		i2c_set_clientdata(client, NULL);
> > 
> > No need to call i2c_set_clientdata here.
> 
> Ok.
> 
> > > +		kfree(ak881x);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
> > > +	ak881x->pdata = client->dev.platform_data;
> > > +
> > > +	if (ak881x->pdata) {
> > > +		if (ak881x->pdata->flags & AK881X_FIELD)
> > > +			ifmode = 4;
> > > +		else
> > > +			ifmode = 0;
> > > +
> > > +		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
> > > +		case AK881X_IF_MODE_BT656:
> > > +			ifmode |= 1;
> > > +			break;
> > > +		case AK881X_IF_MODE_MASTER:
> > > +			ifmode |= 2;
> > > +			break;
> > > +		case AK881X_IF_MODE_SLAVE:
> > > +		default:
> > > +			break;
> > > +		}
> > > +
> > > +		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
> > > +
> > > +		/*
> > > +		 * "Line Blanking No." seems to be the same as the number of
> > > +		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
> > > +		 * "incidentally" matches ak881x' default
> > > +		 */
> > > +		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
> > > +	}
> > > +
> > > +	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
> > > +		 data, ak881x->revision);
> > 
> > Please use this instead:
> > 
> >         v4l2_info(&ak881x->subdev, "chip found @ 0x%02x (%s, revision %x)\n",
> >                  client->addr << 1, client->adapter->name, ak881x->revision);
> > 
> > This is for consistency with other i2c v4l drivers.
> > 
> > Please also use v4l2_info/warn/err/dbg instead of the dev_ versions (except if
> > there is no subdev pointer available). Again for consistency with other i2c
> > drivers (and a more concise prefix as well).
> 
> Ooh, why do we have to be special?... Firstly, it is a Linux convention to 
> specify I2C device addresses without the read / write bit. Granted it has 
> introduced enough confusion, but don't we make it even worse with this?...

Good point, I'll look at this.

> Secondly, I didn't like these macros as I first saw them, and I still 
> don't like them for a very simple reason: do all driver subsystems now 
> have to introduce their own *_printk versions? usb_info()? pci_info()? 
> ata_info()? net_info()?... Don't we have dev_* exactly for this purpose - 
> to be used by all drivers, regardless of the subsystem? Can we maybe 
> rethink this approach, shall we?

I will get back to this later. For what it is worth, I'm not too pleased
with the current situation either.

I *do* want to keep the format of the line that logs a newly found i2c chip
consistent over all v4l i2c drivers. Often video hardware has a constellation
of i2c devices and it should be easy to see from the log which are loaded.

Perhaps I should make a dedicated v4l2_subdev function for this instead,
but that seems a bit overkill.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_remove(struct i2c_client *client)
> > > +{
> > > +	struct ak881x *ak881x = to_ak881x(client);
> > > +
> > > +	i2c_set_clientdata(client, NULL);
> > 
> > This is not right. Use this instead:
> > 
> >         v4l2_device_unregister_subdev(sd);
> > 
> > See v4l2-framework.txt why you should do this in remove().
> 
> Ok.
> 
> > > +	kfree(ak881x);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id ak881x_id[] = {
> > > +	{ "ak8813", 0 },
> > > +	{ "ak8814", 0 },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ak881x_id);
> > > +
> > > +static struct i2c_driver ak881x_i2c_driver = {
> > > +	.driver = {
> > > +		.name = "ak881x",
> > > +	},
> > > +	.probe		= ak881x_probe,
> > > +	.remove		= ak881x_remove,
> > > +	.id_table	= ak881x_id,
> > > +};
> > > +
> > > +static int __init ak881x_module_init(void)
> > > +{
> > > +	return i2c_add_driver(&ak881x_i2c_driver);
> > > +}
> > > +
> > > +static void __exit ak881x_module_exit(void)
> > > +{
> > > +	i2c_del_driver(&ak881x_i2c_driver);
> > > +}
> > > +
> > > +module_init(ak881x_module_init);
> > > +module_exit(ak881x_module_exit);
> > > +
> > > +MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
> > > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/media/ak881x.h b/include/media/ak881x.h
> > > new file mode 100644
> > > index 0000000..b7f2add
> > > --- /dev/null
> > > +++ b/include/media/ak881x.h
> > > @@ -0,0 +1,25 @@
> > > +/*
> > > + * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > > + *
> > > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#ifndef AK881X_H
> > > +#define AK881X_H
> > > +
> > > +#define AK881X_IF_MODE_MASK	(3 << 0)
> > > +#define AK881X_IF_MODE_BT656	(0 << 0)
> > > +#define AK881X_IF_MODE_MASTER	(1 << 0)
> > > +#define AK881X_IF_MODE_SLAVE	(2 << 0)
> > > +#define AK881X_FIELD		(1 << 2)
> > > +#define AK881X_COMPONENT	(1 << 3)
> > > +
> > > +struct ak881x_pdata {
> > > +	unsigned long flags;
> > 
> > Why unsigned long? u32 makes more sense. For 64-bit archs unsigned long is
> > 64 bits.
> 
> Disagree. I'm trying to follow the rule to only use fixed-bitsize types 
> where needed, e.g., where values, they describe, correspond to some (fixed 
> width) hardware registers, or are a part of an ABI. In all other cases I 
> prefer to use C-native types. So, if you really want to save those 32 bits 
> per ak881x device on hypothetical 64-bit systems where it can ever by used 
> - we can make it unsigned int, otherwise, using a long for flags has 
> become more or less a tradition, I think.

True. unsigned long seems to be used often for flags, although it's a mystery
to me what the reasoning is behind that. It just seems a waste of space to me.

> 
> > > +};
> > > +
> > > +#endif
> > > diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> > > index 6cc107d..5d7b742 100644
> > > --- a/include/media/v4l2-chip-ident.h
> > > +++ b/include/media/v4l2-chip-ident.h
> > > @@ -289,6 +289,10 @@ enum {
> > >  
> > >  	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
> > >  	V4L2_IDENT_RJ54N1CB0C = 51980,
> > > +
> > > +	/* AKM AK8813/AK8814 */
> > > +	V4L2_IDENT_AK8813 = 8813,
> > > +	V4L2_IDENT_AK8814 = 8814,
> > 
> > The IDs in v4l2-chip-ident.h should be kept in increasing numeric order. I see
> > that several are already placed out of order. I'm going to make a patch for
> > that and then you can add these new IDs in the right place.

My patch fixing this has already been merged.

Regards,

	Hans

> 
> Yep, will do.
> 
> > >  };
> > >  
> > >  #endif
> > > 
> 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] V4L: v4l2-subdev driver for AK8813 and AK8814 TV-encoders from AKM

[Hans Verkuil]

On Monday 15 March 2010 09:56:35 Guennadi Liakhovetski wrote:
> On Sun, 14 Mar 2010, Hans Verkuil wrote:
> 
> > Review notes below...
> > 
> > On Thursday 11 March 2010 11:25:42 Guennadi Liakhovetski wrote:
> > > AK8814 only differs from AK8813 by included Macrovision Copy Protection
> > > function. This patch adds a driver for AK8813 and AK8814 I2C PAL/NTSC TV
> > > encoders.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> 
> [snip]
> 
> > > diff --git a/drivers/media/video/ak881x.c b/drivers/media/video/ak881x.c
> > > new file mode 100644
> > > index 0000000..b91f0f6
> > > --- /dev/null
> > > +++ b/drivers/media/video/ak881x.c
> > > @@ -0,0 +1,360 @@
> > > +/*
> > > + * Driver for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > > + *
> > > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/videodev2.h>
> > > +
> > > +#include <media/ak881x.h>
> > > +#include <media/v4l2-chip-ident.h>
> > > +#include <media/v4l2-common.h>
> > > +#include <media/v4l2-device.h>
> > > +
> > > +#define AK881X_INTERFACE_MODE	0
> > > +#define AK881X_VIDEO_PROCESS1	1
> > > +#define AK881X_VIDEO_PROCESS2	2
> > > +#define AK881X_VIDEO_PROCESS3	3
> > > +#define AK881X_DAC_MODE		5
> > > +#define AK881X_STATUS		0x24
> > > +#define AK881X_DEVICE_ID	0x25
> > > +#define AK881X_DEVICE_REVISION	0x26
> > > +
> > > +struct ak881x {
> > > +	struct v4l2_subdev subdev;
> > > +	struct ak881x_pdata *pdata;
> > > +	int id;	/* DEVICE_ID code V4L2_IDENT_AK881X code from v4l2-chip-ident.h */
> > > +	char revision;	/* DEVICE_REVISION content */
> > > +};
> > > +
> > > +static int reg_read(struct i2c_client *client, const u8 reg)
> > > +{
> > > +	return i2c_smbus_read_byte_data(client, reg);
> > > +}
> > > +
> > > +static int reg_write(struct i2c_client *client, const u8 reg,
> > > +		     const u8 data)
> > > +{
> > > +	return i2c_smbus_write_byte_data(client, reg, data);
> > > +}
> > 
> > I suggest making these inline.
> 
> Disagree. It has been advised on the LKML to _not_ use inline in .c files 
> - the compiler decides itself, and it does trivially inline these.

The latest CodingStyle document still contains this paragraph:

"A reasonable rule of thumb is to not put inline at functions that have more
than 3 lines of code in them. An exception to this rule are the cases where
a parameter is known to be a compiletime constant, and as a result of this
constantness you *know* the compiler will be able to optimize most of your
function away at compile time. For a good example of this later case, see
the kmalloc() inline function."

So that's what I'm following.

> 
> > I also recommend using struct v4l2_subdev instead of struct i2c_client as
> > argument. In my experience it makes the code cleaner if the mapping from
> > subdev to i2c_client is done at the lowest possible level.
> 
> May I disagree with this one too?;) Just for a mere reason, that in this 
> specific case, register-access routines should not need anything except the 
> i2c-client - by desiign.

It was just a suggestion. I prefer to get the i2c_client pointer only there
where it is actually needed, rather then all over the source. It's not a
blocking issue for me, just the voice of experience :-)

> 
> > > +
> > > +static int reg_set(struct i2c_client *client, const u8 reg,
> > > +		   const u8 data, u8 mask)
> > > +{
> > > +	int ret = reg_read(client, reg);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	return reg_write(client, reg, (ret & ~mask) | (data & mask));
> > > +}
> > > +
> > > +static struct ak881x *to_ak881x(const struct i2c_client *client)
> > > +{
> > > +	return container_of(i2c_get_clientdata(client), struct ak881x, subdev);
> > > +}
> > 
> > Ditto for this one.
> > 
> > > +
> > > +static int ak881x_g_chip_ident(struct v4l2_subdev *sd,
> > > +				struct v4l2_dbg_chip_ident *id)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > 
> > Don't use sd->priv directly. Use v4l2_get_subdevdata(sd) instead.
> 
> Ok.
> 
> > > +	struct ak881x *ak881x = to_ak881x(client);
> > > +
> > > +	if (id->match.type != V4L2_CHIP_MATCH_I2C_ADDR)
> > > +		return -EINVAL;
> > > +
> > > +	if (id->match.addr != client->addr)
> > > +		return -ENODEV;
> > > +
> > > +	id->ident	= ak881x->id;
> > > +	id->revision	= ak881x->revision;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > > +static int ak881x_g_register(struct v4l2_subdev *sd,
> > > +			      struct v4l2_dbg_register *reg)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +
> > > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > > +		return -EINVAL;
> > > +
> > > +	if (reg->match.addr != client->addr)
> > > +		return -ENODEV;
> > > +
> > > +	reg->val = reg_read(client, reg->reg);
> > > +
> > > +	if (reg->val > 0xffff)
> > > +		return -EIO;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_register(struct v4l2_subdev *sd,
> > > +			      struct v4l2_dbg_register *reg)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +
> > > +	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0x26)
> > > +		return -EINVAL;
> > > +
> > > +	if (reg->match.addr != client->addr)
> > > +		return -ENODEV;
> > > +
> > > +	if (reg_write(client, reg->reg, reg->val) < 0)
> > > +		return -EIO;
> > > +
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > > +static int ak881x_try_g_fmt(struct v4l2_subdev *sd,
> > > +			    struct v4l2_mbus_framefmt *mf)
> > 
> > Can you rename this function to ak881x_try_g_mbus_fmt? Same for the other
> > fmt functions.
> 
> Well, we (including you) wanted to eventually convert all subdev drivers 
> to mediabus, 

True. I've actually started to work on that.

> and then to rename *_mbus_fmt to just *_fmt.

If I ever said that (I might have, I can't remember), then I have definitely
changed my mind on that. It's about the mediabus so that fact should be part
of the name.

> That's why I 
> kept these names without "mbus" also in all soc-camera drivers, as I was 
> converting them to mediabus. So, I would prefer to keep this here too.
> 
> > > +{
> > > +	v4l_bound_align_image(&mf->width, 0, 720, 2,
> > > +			      &mf->height, 0, 480, 1, 0);
> > 
> > 480? Doesn't this do 576 as well when PAL is selected?
> 
> Right, it, probably, can.
> 
> > > +	mf->field	= V4L2_FIELD_INTERLACED;
> > > +	mf->code	= V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > > +	mf->colorspace	= V4L2_COLORSPACE_SMPTE170M;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_fmt(struct v4l2_subdev *sd,
> > > +			 struct v4l2_mbus_framefmt *mf)
> > > +{
> > > +	if (mf->field != V4L2_FIELD_INTERLACED ||
> > > +	    mf->code != V4L2_MBUS_FMT_YUYV8_2X8_LE)
> > > +		return -EINVAL;
> > > +
> > > +	return ak881x_try_g_fmt(sd, mf);
> > > +}
> > > +
> > > +static int ak881x_enum_fmt(struct v4l2_subdev *sd, int index,
> > > +			    enum v4l2_mbus_pixelcode *code)
> > > +{
> > > +	if (index)
> > > +		return -EINVAL;
> > > +
> > > +	*code = V4L2_MBUS_FMT_YUYV8_2X8_LE;
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> > > +{
> > > +	a->bounds.left			= 0;
> > > +	a->bounds.top			= 0;
> > > +	a->bounds.width			= 720;
> > > +	a->bounds.height		= 240 * 2;
> > 
> > 288 * 2 for PAL?
> 
> Yup.
> 
> > > +	a->defrect			= a->bounds;
> > > +	a->type				= V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > +	a->pixelaspect.numerator	= 1;
> > > +	a->pixelaspect.denominator	= 1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_std_output(struct v4l2_subdev *sd, v4l2_std_id std)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +	u8 vp1;
> > > +
> > > +	switch (std) {
> > > +	case V4L2_STD_NTSC_M:
> > > +	default:
> > > +		vp1 = 0;
> > > +		break;
> > > +	case V4L2_STD_NTSC_443:
> > > +		vp1 = 3;
> > > +		break;
> > > +	case V4L2_STD_PAL_M:
> > > +		vp1 = 5;
> > > +		break;
> > > +	case V4L2_STD_PAL_60:
> > > +		vp1 = 7;
> > > +		break;
> > > +	case V4L2_STD_PAL_B:
> > > +	case V4L2_STD_PAL_D:
> > > +	case V4L2_STD_PAL_G:
> > > +	case V4L2_STD_PAL_H:
> > > +	case V4L2_STD_PAL_I:
> > > +		vp1 = 0xf;
> > > +	}
> > > +
> > > +	reg_set(client, AK881X_VIDEO_PROCESS1, vp1, 0xf);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_s_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > +	struct i2c_client *client = sd->priv;
> > > +	struct ak881x *ak881x = to_ak881x(client);
> > > +
> > > +	if (enable) {
> > > +		u8 dac;
> > > +		/* For colour-bar testing set bit 6 of AK881X_VIDEO_PROCESS1 */
> > > +		/* Default: composite output */
> > > +		if (ak881x->pdata->flags & AK881X_COMPONENT)
> > > +			dac = 3;
> > > +		else
> > > +			dac = 4;
> > > +		/* Turn on the DAC(s) */
> > > +		reg_write(client, AK881X_DAC_MODE, dac);
> > > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > > +			reg_read(client, AK881X_STATUS));
> > > +	} else {
> > > +		/* ...and clear bit 6 of AK881X_VIDEO_PROCESS1 here */
> > > +		reg_write(client, AK881X_DAC_MODE, 0);
> > > +		dev_dbg(&client->dev, "chip status 0x%x\n",
> > > +			reg_read(client, AK881X_STATUS));
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct v4l2_subdev_core_ops ak881x_subdev_core_ops = {
> > > +	.g_chip_ident	= ak881x_g_chip_ident,
> > > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > > +	.g_register	= ak881x_g_register,
> > > +	.s_register	= ak881x_s_register,
> > > +#endif
> > > +};
> > > +
> > > +static struct v4l2_subdev_video_ops ak881x_subdev_video_ops = {
> > > +	.s_mbus_fmt	= ak881x_s_fmt,
> > > +	.g_mbus_fmt	= ak881x_try_g_fmt,
> > > +	.try_mbus_fmt	= ak881x_try_g_fmt,
> > > +	.cropcap	= ak881x_cropcap,
> > > +	.enum_mbus_fmt	= ak881x_enum_fmt,
> > > +	.s_std_output	= ak881x_s_std_output,
> > > +	.s_stream	= ak881x_s_stream,
> > > +};
> > > +
> > > +static struct v4l2_subdev_ops ak881x_subdev_ops = {
> > > +	.core	= &ak881x_subdev_core_ops,
> > > +	.video	= &ak881x_subdev_video_ops,
> > > +};
> > > +
> > > +static int ak881x_probe(struct i2c_client *client,
> > > +			const struct i2c_device_id *did)
> > > +{
> > > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > > +	struct ak881x *ak881x;
> > > +	u8 ifmode, data;
> > > +
> > > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > > +		dev_warn(&adapter->dev,
> > > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	ak881x = kzalloc(sizeof(struct ak881x), GFP_KERNEL);
> > > +	if (!ak881x)
> > > +		return -ENOMEM;
> > > +
> > > +	v4l2_i2c_subdev_init(&ak881x->subdev, client, &ak881x_subdev_ops);
> > > +
> > > +	data = reg_read(client, AK881X_DEVICE_ID);
> > > +
> > > +	switch (data) {
> > > +	case 0x13:
> > > +		ak881x->id = V4L2_IDENT_AK8813;
> > > +		break;
> > > +	case 0x14:
> > > +		ak881x->id = V4L2_IDENT_AK8814;
> > > +		break;
> > > +	default:
> > > +		dev_err(&client->dev,
> > > +			"No ak881x chip detected, register read %x\n", data);
> > > +		i2c_set_clientdata(client, NULL);
> > 
> > No need to call i2c_set_clientdata here.
> 
> Ok.
> 
> > > +		kfree(ak881x);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ak881x->revision = reg_read(client, AK881X_DEVICE_REVISION);
> > > +	ak881x->pdata = client->dev.platform_data;
> > > +
> > > +	if (ak881x->pdata) {
> > > +		if (ak881x->pdata->flags & AK881X_FIELD)
> > > +			ifmode = 4;
> > > +		else
> > > +			ifmode = 0;
> > > +
> > > +		switch (ak881x->pdata->flags & AK881X_IF_MODE_MASK) {
> > > +		case AK881X_IF_MODE_BT656:
> > > +			ifmode |= 1;
> > > +			break;
> > > +		case AK881X_IF_MODE_MASTER:
> > > +			ifmode |= 2;
> > > +			break;
> > > +		case AK881X_IF_MODE_SLAVE:
> > > +		default:
> > > +			break;
> > > +		}
> > > +
> > > +		dev_dbg(&client->dev, "IF mode %x\n", ifmode);
> > > +
> > > +		/*
> > > +		 * "Line Blanking No." seems to be the same as the number of
> > > +		 * "black" lines on, e.g., SuperH VOU, whose default value of 20
> > > +		 * "incidentally" matches ak881x' default
> > > +		 */
> > > +		reg_write(client, AK881X_INTERFACE_MODE, ifmode | (20 << 3));
> > > +	}
> > > +
> > > +	dev_info(&client->dev, "Detected an ak881x chip ID %x, revision %x\n",
> > > +		 data, ak881x->revision);
> > 
> > Please use this instead:
> > 
> >         v4l2_info(&ak881x->subdev, "chip found @ 0x%02x (%s, revision %x)\n",
> >                  client->addr << 1, client->adapter->name, ak881x->revision);
> > 
> > This is for consistency with other i2c v4l drivers.
> > 
> > Please also use v4l2_info/warn/err/dbg instead of the dev_ versions (except if
> > there is no subdev pointer available). Again for consistency with other i2c
> > drivers (and a more concise prefix as well).
> 
> Ooh, why do we have to be special?... Firstly, it is a Linux convention to 
> specify I2C device addresses without the read / write bit. Granted it has 
> introduced enough confusion, but don't we make it even worse with this?...

Good point, I'll look at this.

> Secondly, I didn't like these macros as I first saw them, and I still 
> don't like them for a very simple reason: do all driver subsystems now 
> have to introduce their own *_printk versions? usb_info()? pci_info()? 
> ata_info()? net_info()?... Don't we have dev_* exactly for this purpose - 
> to be used by all drivers, regardless of the subsystem? Can we maybe 
> rethink this approach, shall we?

I will get back to this later. For what it is worth, I'm not too pleased
with the current situation either.

I *do* want to keep the format of the line that logs a newly found i2c chip
consistent over all v4l i2c drivers. Often video hardware has a constellation
of i2c devices and it should be easy to see from the log which are loaded.

Perhaps I should make a dedicated v4l2_subdev function for this instead,
but that seems a bit overkill.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ak881x_remove(struct i2c_client *client)
> > > +{
> > > +	struct ak881x *ak881x = to_ak881x(client);
> > > +
> > > +	i2c_set_clientdata(client, NULL);
> > 
> > This is not right. Use this instead:
> > 
> >         v4l2_device_unregister_subdev(sd);
> > 
> > See v4l2-framework.txt why you should do this in remove().
> 
> Ok.
> 
> > > +	kfree(ak881x);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id ak881x_id[] = {
> > > +	{ "ak8813", 0 },
> > > +	{ "ak8814", 0 },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ak881x_id);
> > > +
> > > +static struct i2c_driver ak881x_i2c_driver = {
> > > +	.driver = {
> > > +		.name = "ak881x",
> > > +	},
> > > +	.probe		= ak881x_probe,
> > > +	.remove		= ak881x_remove,
> > > +	.id_table	= ak881x_id,
> > > +};
> > > +
> > > +static int __init ak881x_module_init(void)
> > > +{
> > > +	return i2c_add_driver(&ak881x_i2c_driver);
> > > +}
> > > +
> > > +static void __exit ak881x_module_exit(void)
> > > +{
> > > +	i2c_del_driver(&ak881x_i2c_driver);
> > > +}
> > > +
> > > +module_init(ak881x_module_init);
> > > +module_exit(ak881x_module_exit);
> > > +
> > > +MODULE_DESCRIPTION("TV-output driver for ak8813/ak8814");
> > > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/media/ak881x.h b/include/media/ak881x.h
> > > new file mode 100644
> > > index 0000000..b7f2add
> > > --- /dev/null
> > > +++ b/include/media/ak881x.h
> > > @@ -0,0 +1,25 @@
> > > +/*
> > > + * Header for AK8813 / AK8814 TV-ecoders from Asahi Kasei Microsystems Co., Ltd. (AKM)
> > > + *
> > > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#ifndef AK881X_H
> > > +#define AK881X_H
> > > +
> > > +#define AK881X_IF_MODE_MASK	(3 << 0)
> > > +#define AK881X_IF_MODE_BT656	(0 << 0)
> > > +#define AK881X_IF_MODE_MASTER	(1 << 0)
> > > +#define AK881X_IF_MODE_SLAVE	(2 << 0)
> > > +#define AK881X_FIELD		(1 << 2)
> > > +#define AK881X_COMPONENT	(1 << 3)
> > > +
> > > +struct ak881x_pdata {
> > > +	unsigned long flags;
> > 
> > Why unsigned long? u32 makes more sense. For 64-bit archs unsigned long is
> > 64 bits.
> 
> Disagree. I'm trying to follow the rule to only use fixed-bitsize types 
> where needed, e.g., where values, they describe, correspond to some (fixed 
> width) hardware registers, or are a part of an ABI. In all other cases I 
> prefer to use C-native types. So, if you really want to save those 32 bits 
> per ak881x device on hypothetical 64-bit systems where it can ever by used 
> - we can make it unsigned int, otherwise, using a long for flags has 
> become more or less a tradition, I think.

True. unsigned long seems to be used often for flags, although it's a mystery
to me what the reasoning is behind that. It just seems a waste of space to me.

> 
> > > +};
> > > +
> > > +#endif
> > > diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> > > index 6cc107d..5d7b742 100644
> > > --- a/include/media/v4l2-chip-ident.h
> > > +++ b/include/media/v4l2-chip-ident.h
> > > @@ -289,6 +289,10 @@ enum {
> > >  
> > >  	/* Sharp RJ54N1CB0C, 0xCB0C = 51980 */
> > >  	V4L2_IDENT_RJ54N1CB0C = 51980,
> > > +
> > > +	/* AKM AK8813/AK8814 */
> > > +	V4L2_IDENT_AK8813 = 8813,
> > > +	V4L2_IDENT_AK8814 = 8814,
> > 
> > The IDs in v4l2-chip-ident.h should be kept in increasing numeric order. I see
> > that several are already placed out of order. I'm going to make a patch for
> > that and then you can add these new IDs in the right place.

My patch fixing this has already been merged.

Regards,

	Hans

> 
> Yep, will do.
> 
> > >  };
> > >  
> > >  #endif
> > > 
> 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 3/3] sh: add Video Output Unit (VOU) and AK8813 TV-encoder support to ms7724se

[Paul Mundt]

On Thu, Mar 11, 2010 at 02:45:00PM +0100, Guennadi Liakhovetski wrote:
> Add platform bindings, GPIO initialisation and allocation and AK8813 reset code
> to ms7724se.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Obviously depends on the previous two VOU and AK881x patches, sorry for 
> not marking those with "PATCH x/3" accordingly. Those two patches do not 
> depend upon each other and initially I wasn't sure I'd be able to clean up 
> this patch sufficiently for submission. Two 10us delays are pretty random, 
> maybe they can be optimised out completely. I just tried to reproduced the 
> reset procedure from the ak8813 datasheet, and it says nothing about the 
> duration of respective stages.
> 
>  arch/sh/boards/mach-se/7724/setup.c |   88 ++++++++++++++++++++++++++++++++---
>  1 files changed, 81 insertions(+), 7 deletions(-)
> 
Now that the other two patches are upstream, I've applied this.

Re: [PATCH 3/3] sh: add Video Output Unit (VOU) and AK8813 TV-encoder support to ms7724se

[Paul Mundt]

On Thu, Mar 11, 2010 at 02:45:00PM +0100, Guennadi Liakhovetski wrote:
> Add platform bindings, GPIO initialisation and allocation and AK8813 reset code
> to ms7724se.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Obviously depends on the previous two VOU and AK881x patches, sorry for 
> not marking those with "PATCH x/3" accordingly. Those two patches do not 
> depend upon each other and initially I wasn't sure I'd be able to clean up 
> this patch sufficiently for submission. Two 10us delays are pretty random, 
> maybe they can be optimised out completely. I just tried to reproduced the 
> reset procedure from the ak8813 datasheet, and it says nothing about the 
> duration of respective stages.
> 
>  arch/sh/boards/mach-se/7724/setup.c |   88 ++++++++++++++++++++++++++++++++---
>  1 files changed, 81 insertions(+), 7 deletions(-)
> 
Now that the other two patches are upstream, I've applied this.