Re: [PATCH 01/10] staging: media: go7007: Some additional code for TW2804 driver functionality

Re: [PATCH 01/10] staging: media: go7007: Some additional code for TW2804 driver functionality

[Hans Verkuil]

Hi Volokh!

Thanks for working on this!

I have some quick review notes below.

On Wed August 22 2012 12:45:10 Volokh Konstantin wrote:
> - using new v4l2 framework controls
> - function for reading volatile controls via i2c bus
> - separate V4L2_CID_ ctrls into each V4L2 calls
> 
> Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
> ---
>  drivers/staging/media/go7007/wis-tw2804.c |  248 +++++++++++++++++++++++++++++
>  1 files changed, 248 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
> index 9134f03..05851d3 100644
> --- a/drivers/staging/media/go7007/wis-tw2804.c
> +++ b/drivers/staging/media/go7007/wis-tw2804.c
> @@ -21,10 +21,18 @@
>  #include <linux/videodev2.h>
>  #include <linux/ioctl.h>
>  #include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-ctrls.h>
>  
>  #include "wis-i2c.h"
>  
>  struct wis_tw2804 {
> +	struct v4l2_subdev sd;
> +	struct v4l2_ctrl_handler hdl;
> +	u8 channel:2;
> +	u8 input:1;
>  	int channel;
>  	int norm;
>  	int brightness;
> @@ -116,9 +124,246 @@ static int write_regs(struct i2c_client *client, u8 *regs, int channel)
>  		if (i2c_smbus_write_byte_data(client,
>  				regs[i] | (channel << 6), regs[i + 1]) < 0)
>  			return -1;
> +static s32 read_reg(struct i2c_client *client, u8 reg, u8 channel)
> +{
> +	return i2c_smbus_read_byte_data(client, (reg) | (channel << 6));
> +}
> +
> +inline struct wis_tw2804 *to_state(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct wis_tw2804, sd);
> +}
> +
> +inline struct wis_tw2804 *to_state_from_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	return container_of(ctrl->handler, struct wis_tw2804, hdl);
> +}
> +
> +static int tw2804_log_status(struct v4l2_subdev *sd)
> +{
> +	struct wis_tw2804 *state = to_state(sd);
> +	v4l2_info(sd, "Standard: %s\n",
> +			state->norm == V4L2_STD_NTSC ? "NTSC" :
> +			state->norm == V4L2_STD_PAL ? "PAL" : "unknown");
> +	v4l2_info(sd, "Channel: %d\n", state->channel);
> +	v4l2_info(sd, "Input: %d\n", state->input);
> +	v4l2_ctrl_handler_log_status(&state->hdl, sd->name);
> +	return 0;
> +}
> +
> +static s32 get_ctrl_addr(int ctrl)
> +{
> +	switch (ctrl) {
> +	case V4L2_CID_BRIGHTNESS:
> +		return 0x12;
> +	case V4L2_CID_CONTRAST:
> +		return 0x11;
> +	case V4L2_CID_SATURATION:
> +		return 0x10;
> +	case V4L2_CID_HUE:
> +		return 0x0f;
> +	case V4L2_CID_AUTOGAIN:
> +		return 0x02;
> +	case V4L2_CID_COLOR_KILLER:
> +		return 0x14;
> +	case V4L2_CID_GAIN:
> +		return 0x3c;
> +	case V4L2_CID_CHROMA_GAIN:
> +		return 0x3d;
> +	case V4L2_CID_RED_BALANCE:
> +		return 0x3f;
> +	case V4L2_CID_BLUE_BALANCE:
> +		return 0x3e;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int tw2804_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = &to_state_from_ctrl(ctrl)->sd;
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	s32 addr = get_ctrl_addr(ctrl->id);
> +
> +	if (addr == -EINVAL)
> +		return -EINVAL;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_GAIN:
> +	case V4L2_CID_CHROMA_GAIN:
> +	case V4L2_CID_RED_BALANCE:
> +	case V4L2_CID_BLUE_BALANCE:
> +		ctrl->cur.val = read_reg(client, addr, 0);

That should be ctrl->val, not ctrl->cur.val.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int tw2804_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct wis_tw2804 *state = to_state_from_ctrl(ctrl);
> +	struct v4l2_subdev *sd = &state->sd;
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	s32 reg = ctrl->val;
> +	s32 addr = get_ctrl_addr(ctrl->id);
> +
> +	if (addr == -EINVAL)
> +		return -EINVAL;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_AUTOGAIN:
> +		reg = read_reg(client, addr, state->channel);
> +		if (reg > 0) {
> +			if (ctrl->val == 0)
> +				reg &= ~(1<<7);
> +			else
> +				reg |= 1<<7;
> +		} else
> +			return reg;
> +		break;
> +	case V4L2_CID_COLOR_KILLER:
> +		reg = read_reg(client, addr, state->channel);
> +		if (reg > 0)
> +			reg = (reg & ~(0x03)) | (ctrl->val == 0 ? 0x02 : 0x03);
> +		else
> +			return reg;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	reg = reg > 255 ? 255 : (reg < 0 ? 0 : reg);
> +	reg = write_reg(client, addr, (u8)reg,
> +			ctrl->id == V4L2_CID_GAIN ||
> +			ctrl->id == V4L2_CID_CHROMA_GAIN ||
> +			ctrl->id == V4L2_CID_RED_BALANCE ||
> +			ctrl->id == V4L2_CID_BLUE_BALANCE ? 0 : state->channel);
> +
> +	if (reg < 0) {
> +		v4l2_err(sd, "Can`t set_ctrl value:id=%d;value=%d\n", ctrl->id,
> +								    ctrl->val);
> +		return reg;
> +	}
> +	return 0;
> +}
> +
> +static int tw2804_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
> +{
> +	struct wis_tw2804 *dec = to_state(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	u8 regs[] = {
> +		0x01, norm&V4L2_STD_NTSC ? 0xc4 : 0x84,

I recommend creating a bool variable storing the result of norm & V4L2_STD_NTSC
rather than repeating it.

Also, do you really mean 'norm & V4L2_STD_NTSC', or do you mean
'norm & V4L2_STD_525_60'?

My guess is the latter.

> +		0x09, norm&V4L2_STD_NTSC ? 0x07 : 0x04,
> +		0x0a, norm&V4L2_STD_NTSC ? 0xf0 : 0x20,
> +		0x0b, norm&V4L2_STD_NTSC ? 0x07 : 0x04,
> +		0x0c, norm&V4L2_STD_NTSC ? 0xf0 : 0x20,
> +		0x0d, norm&V4L2_STD_NTSC ? 0x40 : 0x4a,
> +		0x16, norm&V4L2_STD_NTSC ? 0x00 : 0x40,
> +		0x17, norm&V4L2_STD_NTSC ? 0x00 : 0x40,
> +		0x20, norm&V4L2_STD_NTSC ? 0x07 : 0x0f,
> +		0x21, norm&V4L2_STD_NTSC ? 0x07 : 0x0f,
> +		0xff, 0xff,
> +	};
> +	write_regs(client, regs, dec->channel);
> +	dec->norm = norm;
> +	return 0;
> +}
> +
> +static int tw2804_g_chip_ident(struct v4l2_subdev *sd,
> +				struct v4l2_dbg_chip_ident *chip)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	return v4l2_chip_ident_i2c_client(client, chip,
> +					V4L2_IDENT_TW2804, 0x0e);
> +}
> +
> +static const struct v4l2_ctrl_ops tw2804_ctrl_ops = {
> +	.g_volatile_ctrl = tw2804_g_volatile_ctrl,
> +	.s_ctrl = tw2804_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_core_ops tw2804_core_ops = {
> +	.log_status = tw2804_log_status,
> +	.g_chip_ident = tw2804_g_chip_ident,
> +	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
> +	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
> +	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
> +	.g_ctrl = v4l2_subdev_g_ctrl,
> +	.s_ctrl = v4l2_subdev_s_ctrl,
> +	.queryctrl = v4l2_subdev_queryctrl,
> +	.querymenu = v4l2_subdev_querymenu,
> +	.s_std = tw2804_s_std,
> +};
> +
> +static int tw2804_s_video_routing(struct v4l2_subdev *sd, u32 input, u32 output,
> +	u32 config)
> +{
> +	struct wis_tw2804 *dec = to_state(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	s32 reg = 0;
> +
> +	if (0 > input || input > 1)
> +		return -EINVAL;
> +
> +	if (input == dec->input)
> +		return 0;
> +
> +	reg = read_reg(client, 0x22, dec->channel);
> +
> +	if (reg >= 0) {
> +		if (input == 0)
> +			reg &= ~(1<<2);
> +		else
> +			reg |= 1<<2;
> +		reg = write_reg(client, 0x22, (u8)reg, dec->channel);
> +	}
> +
> +	if (reg >= 0)
> +		dec->input = input;
> +	else
> +		return reg;
>  	return 0;
>  }
>  
> +static int tw2804_s_mbus_fmt(struct v4l2_subdev *sd,
> +	struct v4l2_mbus_framefmt *fmt)
> +{
> +	/*TODO need select between 3fmt:
> +	 * bt_656,
> +	 * bt_601_8bit,
> +	 * bt_656_dual,
> +	 */
> +	return 0;
> +}
> +
> +static int tw2804_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct wis_tw2804 *dec = to_state(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	u32 reg = read_reg(client, 0x78, 0);
> +
> +	if (enable == 1)
> +		write_reg(client, 0x78, reg & ~(1<<dec->channel), 0);
> +	else
> +		write_reg(client, 0x78, reg | (1<<dec->channel), 0);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops tw2804_video_ops = {
> +	.s_routing = tw2804_s_video_routing,
> +	.s_mbus_fmt = tw2804_s_mbus_fmt,
> +	.s_stream = tw2804_s_stream,
> +};
> +
> +static const struct v4l2_subdev_ops tw2804_ops = {
> +	.core = &tw2804_core_ops,
> +	.video = &tw2804_video_ops,
> +};
> +
>  static int wis_tw2804_command(struct i2c_client *client,
>  				unsigned int cmd, void *arg)
>  {
> @@ -355,3 +600,6 @@ module_init(wis_tw2804_init);
>  module_exit(wis_tw2804_cleanup);
>  
>  MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TW2804/TW2802 V4L2 i2c driver");
> +MODULE_AUTHOR("Volokh Konstantin <volokh84@gmail.com>");
> +MODULE_AUTHOR("Micronas USA Inc");
> 

Regards,

	Hans

Re: [PATCH 04/10] staging: media: go7007: Add IDENT for TW2802/2804

[Hans Verkuil]

On Wed August 22 2012 12:45:13 Volokh Konstantin wrote:
> Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
> ---
>  include/media/v4l2-chip-ident.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> index 7395c81..5395495 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -113,6 +113,10 @@ enum {
>  	/* module vp27smpx: just ident 2700 */
>  	V4L2_IDENT_VP27SMPX = 2700,
>  
> +	/* module wis-tw2804: 2802/2804 */
> +	V4L2_IDENT_TW2802 = 2802,
> +	V4L2_IDENT_TW2804 = 2804,
> +
>  	/* module vpx3220: reserved range: 3210-3229 */
>  	V4L2_IDENT_VPX3214C = 3214,
>  	V4L2_IDENT_VPX3216B = 3216,
> 

There is no need to add this, unless the g/s_register or g_chip_ident ops
are also implemented.

In this case I'd just drop this patch.

Regards,

	Hans

[PATCH 01/10] staging: media: go7007: Some additional code for TW2804 driver functionality

[Volokh Konstantin]

- using new v4l2 framework controls
- function for reading volatile controls via i2c bus
- separate V4L2_CID_ ctrls into each V4L2 calls

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/wis-tw2804.c |  248 +++++++++++++++++++++++++++++
 1 files changed, 248 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
index 9134f03..05851d3 100644
--- a/drivers/staging/media/go7007/wis-tw2804.c
+++ b/drivers/staging/media/go7007/wis-tw2804.c
@@ -21,10 +21,18 @@
 #include <linux/videodev2.h>
 #include <linux/ioctl.h>
 #include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
 
 #include "wis-i2c.h"
 
 struct wis_tw2804 {
+	struct v4l2_subdev sd;
+	struct v4l2_ctrl_handler hdl;
+	u8 channel:2;
+	u8 input:1;
 	int channel;
 	int norm;
 	int brightness;
@@ -116,9 +124,246 @@ static int write_regs(struct i2c_client *client, u8 *regs, int channel)
 		if (i2c_smbus_write_byte_data(client,
 				regs[i] | (channel << 6), regs[i + 1]) < 0)
 			return -1;
+static s32 read_reg(struct i2c_client *client, u8 reg, u8 channel)
+{
+	return i2c_smbus_read_byte_data(client, (reg) | (channel << 6));
+}
+
+inline struct wis_tw2804 *to_state(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct wis_tw2804, sd);
+}
+
+inline struct wis_tw2804 *to_state_from_ctrl(struct v4l2_ctrl *ctrl)
+{
+	return container_of(ctrl->handler, struct wis_tw2804, hdl);
+}
+
+static int tw2804_log_status(struct v4l2_subdev *sd)
+{
+	struct wis_tw2804 *state = to_state(sd);
+	v4l2_info(sd, "Standard: %s\n",
+			state->norm == V4L2_STD_NTSC ? "NTSC" :
+			state->norm == V4L2_STD_PAL ? "PAL" : "unknown");
+	v4l2_info(sd, "Channel: %d\n", state->channel);
+	v4l2_info(sd, "Input: %d\n", state->input);
+	v4l2_ctrl_handler_log_status(&state->hdl, sd->name);
+	return 0;
+}
+
+static s32 get_ctrl_addr(int ctrl)
+{
+	switch (ctrl) {
+	case V4L2_CID_BRIGHTNESS:
+		return 0x12;
+	case V4L2_CID_CONTRAST:
+		return 0x11;
+	case V4L2_CID_SATURATION:
+		return 0x10;
+	case V4L2_CID_HUE:
+		return 0x0f;
+	case V4L2_CID_AUTOGAIN:
+		return 0x02;
+	case V4L2_CID_COLOR_KILLER:
+		return 0x14;
+	case V4L2_CID_GAIN:
+		return 0x3c;
+	case V4L2_CID_CHROMA_GAIN:
+		return 0x3d;
+	case V4L2_CID_RED_BALANCE:
+		return 0x3f;
+	case V4L2_CID_BLUE_BALANCE:
+		return 0x3e;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tw2804_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_subdev *sd = &to_state_from_ctrl(ctrl)->sd;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	s32 addr = get_ctrl_addr(ctrl->id);
+
+	if (addr == -EINVAL)
+		return -EINVAL;
+
+	switch (ctrl->id) {
+	case V4L2_CID_GAIN:
+	case V4L2_CID_CHROMA_GAIN:
+	case V4L2_CID_RED_BALANCE:
+	case V4L2_CID_BLUE_BALANCE:
+		ctrl->cur.val = read_reg(client, addr, 0);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int tw2804_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct wis_tw2804 *state = to_state_from_ctrl(ctrl);
+	struct v4l2_subdev *sd = &state->sd;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	s32 reg = ctrl->val;
+	s32 addr = get_ctrl_addr(ctrl->id);
+
+	if (addr == -EINVAL)
+		return -EINVAL;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUTOGAIN:
+		reg = read_reg(client, addr, state->channel);
+		if (reg > 0) {
+			if (ctrl->val == 0)
+				reg &= ~(1<<7);
+			else
+				reg |= 1<<7;
+		} else
+			return reg;
+		break;
+	case V4L2_CID_COLOR_KILLER:
+		reg = read_reg(client, addr, state->channel);
+		if (reg > 0)
+			reg = (reg & ~(0x03)) | (ctrl->val == 0 ? 0x02 : 0x03);
+		else
+			return reg;
+		break;
+	default:
+		break;
+	}
+
+	reg = reg > 255 ? 255 : (reg < 0 ? 0 : reg);
+	reg = write_reg(client, addr, (u8)reg,
+			ctrl->id == V4L2_CID_GAIN ||
+			ctrl->id == V4L2_CID_CHROMA_GAIN ||
+			ctrl->id == V4L2_CID_RED_BALANCE ||
+			ctrl->id == V4L2_CID_BLUE_BALANCE ? 0 : state->channel);
+
+	if (reg < 0) {
+		v4l2_err(sd, "Can`t set_ctrl value:id=%d;value=%d\n", ctrl->id,
+								    ctrl->val);
+		return reg;
+	}
+	return 0;
+}
+
+static int tw2804_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
+{
+	struct wis_tw2804 *dec = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	u8 regs[] = {
+		0x01, norm&V4L2_STD_NTSC ? 0xc4 : 0x84,
+		0x09, norm&V4L2_STD_NTSC ? 0x07 : 0x04,
+		0x0a, norm&V4L2_STD_NTSC ? 0xf0 : 0x20,
+		0x0b, norm&V4L2_STD_NTSC ? 0x07 : 0x04,
+		0x0c, norm&V4L2_STD_NTSC ? 0xf0 : 0x20,
+		0x0d, norm&V4L2_STD_NTSC ? 0x40 : 0x4a,
+		0x16, norm&V4L2_STD_NTSC ? 0x00 : 0x40,
+		0x17, norm&V4L2_STD_NTSC ? 0x00 : 0x40,
+		0x20, norm&V4L2_STD_NTSC ? 0x07 : 0x0f,
+		0x21, norm&V4L2_STD_NTSC ? 0x07 : 0x0f,
+		0xff, 0xff,
+	};
+	write_regs(client, regs, dec->channel);
+	dec->norm = norm;
+	return 0;
+}
+
+static int tw2804_g_chip_ident(struct v4l2_subdev *sd,
+				struct v4l2_dbg_chip_ident *chip)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	return v4l2_chip_ident_i2c_client(client, chip,
+					V4L2_IDENT_TW2804, 0x0e);
+}
+
+static const struct v4l2_ctrl_ops tw2804_ctrl_ops = {
+	.g_volatile_ctrl = tw2804_g_volatile_ctrl,
+	.s_ctrl = tw2804_s_ctrl,
+};
+
+static const struct v4l2_subdev_core_ops tw2804_core_ops = {
+	.log_status = tw2804_log_status,
+	.g_chip_ident = tw2804_g_chip_ident,
+	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
+	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
+	.g_ctrl = v4l2_subdev_g_ctrl,
+	.s_ctrl = v4l2_subdev_s_ctrl,
+	.queryctrl = v4l2_subdev_queryctrl,
+	.querymenu = v4l2_subdev_querymenu,
+	.s_std = tw2804_s_std,
+};
+
+static int tw2804_s_video_routing(struct v4l2_subdev *sd, u32 input, u32 output,
+	u32 config)
+{
+	struct wis_tw2804 *dec = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	s32 reg = 0;
+
+	if (0 > input || input > 1)
+		return -EINVAL;
+
+	if (input == dec->input)
+		return 0;
+
+	reg = read_reg(client, 0x22, dec->channel);
+
+	if (reg >= 0) {
+		if (input == 0)
+			reg &= ~(1<<2);
+		else
+			reg |= 1<<2;
+		reg = write_reg(client, 0x22, (u8)reg, dec->channel);
+	}
+
+	if (reg >= 0)
+		dec->input = input;
+	else
+		return reg;
 	return 0;
 }
 
+static int tw2804_s_mbus_fmt(struct v4l2_subdev *sd,
+	struct v4l2_mbus_framefmt *fmt)
+{
+	/*TODO need select between 3fmt:
+	 * bt_656,
+	 * bt_601_8bit,
+	 * bt_656_dual,
+	 */
+	return 0;
+}
+
+static int tw2804_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct wis_tw2804 *dec = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	u32 reg = read_reg(client, 0x78, 0);
+
+	if (enable == 1)
+		write_reg(client, 0x78, reg & ~(1<<dec->channel), 0);
+	else
+		write_reg(client, 0x78, reg | (1<<dec->channel), 0);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_video_ops tw2804_video_ops = {
+	.s_routing = tw2804_s_video_routing,
+	.s_mbus_fmt = tw2804_s_mbus_fmt,
+	.s_stream = tw2804_s_stream,
+};
+
+static const struct v4l2_subdev_ops tw2804_ops = {
+	.core = &tw2804_core_ops,
+	.video = &tw2804_video_ops,
+};
+
 static int wis_tw2804_command(struct i2c_client *client,
 				unsigned int cmd, void *arg)
 {
@@ -355,3 +600,6 @@ module_init(wis_tw2804_init);
 module_exit(wis_tw2804_cleanup);
 
 MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TW2804/TW2802 V4L2 i2c driver");
+MODULE_AUTHOR("Volokh Konstantin <volokh84@gmail.com>");
+MODULE_AUTHOR("Micronas USA Inc");
-- 
1.7.7.6

[PATCH 02/10] staging: media: go7007: TW2804 driver fix

[Volokh Konstantin]

- correct cast private data from i2c_get_clientdata()
- initialization priv data
- destroy priv data
- using V4L2 controls framework

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/wis-tw2804.c |   72 +++++++++++++++++++++++------
 1 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
index 05851d3..13f0f63 100644
--- a/drivers/staging/media/go7007/wis-tw2804.c
+++ b/drivers/staging/media/go7007/wis-tw2804.c
@@ -367,10 +367,12 @@ static const struct v4l2_subdev_ops tw2804_ops = {
 static int wis_tw2804_command(struct i2c_client *client,
 				unsigned int cmd, void *arg)
 {
-	struct wis_tw2804 *dec = i2c_get_clientdata(client);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct wis_tw2804 *dec = to_state(sd);
+	int *input;
 
 	if (cmd == DECODER_SET_CHANNEL) {
-		int *input = arg;
+		input = arg;
 
 		if (*input < 0 || *input > 3) {
 			printk(KERN_ERR "wis-tw2804: channel %d is not "
@@ -539,22 +541,59 @@ static int wis_tw2804_probe(struct i2c_client *client,
 			    const struct i2c_device_id *id)
 {
 	struct i2c_adapter *adapter = client->adapter;
-	struct wis_tw2804 *dec;
+	struct wis_tw2804 *state;
+	struct v4l2_subdev *sd;
+	struct v4l2_ctrl *ctrl = NULL;
+	int err;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
 
-	dec = kmalloc(sizeof(struct wis_tw2804), GFP_KERNEL);
-	if (dec == NULL)
-		return -ENOMEM;
+	state = kzalloc(sizeof(struct wis_tw2804), GFP_KERNEL);
 
-	dec->channel = -1;
-	dec->norm = V4L2_STD_NTSC;
-	dec->brightness = 128;
-	dec->contrast = 128;
-	dec->saturation = 128;
-	dec->hue = 128;
-	i2c_set_clientdata(client, dec);
+	if (state == NULL)
+		return -ENOMEM;
+	sd = &state->sd;
+	v4l2_i2c_subdev_init(sd, client, &tw2804_ops);
+	state->channel = -1;
+	state->norm = V4L2_STD_NTSC;
+
+	v4l2_ctrl_handler_init(&state->hdl, 10);
+	v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
+	v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_CONTRAST, 0, 255, 1, 128);
+	v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_SATURATION, 0, 255, 1, 128);
+	v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_HUE, 0, 255, 1, 128);
+	v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_COLOR_KILLER, 0, 1, 1, 0);
+	ctrl = v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_GAIN, 0, 255, 1, 128);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	ctrl = v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_CHROMA_GAIN, 0, 255, 1, 128);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	ctrl = v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_BLUE_BALANCE, 0, 255, 1, 122);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	ctrl = v4l2_ctrl_new_std(&state->hdl, &tw2804_ctrl_ops,
+					V4L2_CID_RED_BALANCE, 0, 255, 1, 122);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	sd->ctrl_handler = &state->hdl;
+	err = state->hdl.error;
+	if (err) {
+		v4l2_ctrl_handler_free(&state->hdl);
+		kfree(state);
+		return err;
+	}
 
 	printk(KERN_DEBUG "wis-tw2804: creating TW2804 at address %d on %s\n",
 		client->addr, adapter->name);
@@ -564,9 +603,12 @@ static int wis_tw2804_probe(struct i2c_client *client,
 
 static int wis_tw2804_remove(struct i2c_client *client)
 {
-	struct wis_tw2804 *dec = i2c_get_clientdata(client);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct wis_tw2804 *state = to_state(sd);
 
-	kfree(dec);
+	v4l2_device_unregister_subdev(sd);
+	v4l2_ctrl_handler_free(&state->hdl);
+	kfree(state);
 	return 0;
 }
 
-- 
1.7.7.6

[PATCH 03/10] staging: media: go7007: Non main code changing

[Volokh Konstantin]

types cast approved, max channels==4 (ADC), i2c_smbus_write_
  return s32 type

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/wis-tw2804.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
index 13f0f63..0b49342 100644
--- a/drivers/staging/media/go7007/wis-tw2804.c
+++ b/drivers/staging/media/go7007/wis-tw2804.c
@@ -111,19 +111,22 @@ static u8 channel_registers[] = {
 	0xff, 0xff, /* Terminator (reg 0xff does not exist) */
 };
 
-static int write_reg(struct i2c_client *client, u8 reg, u8 value, int channel)
+static s32 write_reg(struct i2c_client *client, u8 reg, u8 value, u8 channel)
 {
 	return i2c_smbus_write_byte_data(client, reg | (channel << 6), value);
 }
 
-static int write_regs(struct i2c_client *client, u8 *regs, int channel)
+static int write_regs(struct i2c_client *client, u8 *regs, u8 channel)
 {
 	int i;
 
 	for (i = 0; regs[i] != 0xff; i += 2)
 		if (i2c_smbus_write_byte_data(client,
 				regs[i] | (channel << 6), regs[i + 1]) < 0)
-			return -1;
+			return -EINVAL;
+	return 0;
+}
+
 static s32 read_reg(struct i2c_client *client, u8 reg, u8 channel)
 {
 	return i2c_smbus_read_byte_data(client, (reg) | (channel << 6));
-- 
1.7.7.6

[PATCH 04/10] staging: media: go7007: Add IDENT for TW2802/2804

[Volokh Konstantin]

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 include/media/v4l2-chip-ident.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index 7395c81..5395495 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -113,6 +113,10 @@ enum {
 	/* module vp27smpx: just ident 2700 */
 	V4L2_IDENT_VP27SMPX = 2700,
 
+	/* module wis-tw2804: 2802/2804 */
+	V4L2_IDENT_TW2802 = 2802,
+	V4L2_IDENT_TW2804 = 2804,
+
 	/* module vpx3220: reserved range: 3210-3229 */
 	V4L2_IDENT_VPX3214C = 3214,
 	V4L2_IDENT_VPX3216B = 3216,
-- 
1.7.7.6

[PATCH 05/10] staging: media: go7007: Cleanup unused old code

[Volokh Konstantin]

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/wis-tw2804.c |  138 -----------------------------
 1 files changed, 0 insertions(+), 138 deletions(-)

diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
index 0b49342..7bd3058 100644
--- a/drivers/staging/media/go7007/wis-tw2804.c
+++ b/drivers/staging/media/go7007/wis-tw2804.c
@@ -33,12 +33,7 @@ struct wis_tw2804 {
 	struct v4l2_ctrl_handler hdl;
 	u8 channel:2;
 	u8 input:1;
-	int channel;
 	int norm;
-	int brightness;
-	int contrast;
-	int saturation;
-	int hue;
 };
 
 static u8 global_registers[] = {
@@ -404,139 +399,6 @@ static int wis_tw2804_command(struct i2c_client *client,
 				"channel number is set\n", cmd);
 		return 0;
 	}
-
-	switch (cmd) {
-	case VIDIOC_S_STD:
-	{
-		v4l2_std_id *input = arg;
-		u8 regs[] = {
-			0x01, *input & V4L2_STD_NTSC ? 0xc4 : 0x84,
-			0x09, *input & V4L2_STD_NTSC ? 0x07 : 0x04,
-			0x0a, *input & V4L2_STD_NTSC ? 0xf0 : 0x20,
-			0x0b, *input & V4L2_STD_NTSC ? 0x07 : 0x04,
-			0x0c, *input & V4L2_STD_NTSC ? 0xf0 : 0x20,
-			0x0d, *input & V4L2_STD_NTSC ? 0x40 : 0x4a,
-			0x16, *input & V4L2_STD_NTSC ? 0x00 : 0x40,
-			0x17, *input & V4L2_STD_NTSC ? 0x00 : 0x40,
-			0x20, *input & V4L2_STD_NTSC ? 0x07 : 0x0f,
-			0x21, *input & V4L2_STD_NTSC ? 0x07 : 0x0f,
-			0xff,	0xff,
-		};
-		write_regs(client, regs, dec->channel);
-		dec->norm = *input;
-		break;
-	}
-	case VIDIOC_QUERYCTRL:
-	{
-		struct v4l2_queryctrl *ctrl = arg;
-
-		switch (ctrl->id) {
-		case V4L2_CID_BRIGHTNESS:
-			ctrl->type = V4L2_CTRL_TYPE_INTEGER;
-			strncpy(ctrl->name, "Brightness", sizeof(ctrl->name));
-			ctrl->minimum = 0;
-			ctrl->maximum = 255;
-			ctrl->step = 1;
-			ctrl->default_value = 128;
-			ctrl->flags = 0;
-			break;
-		case V4L2_CID_CONTRAST:
-			ctrl->type = V4L2_CTRL_TYPE_INTEGER;
-			strncpy(ctrl->name, "Contrast", sizeof(ctrl->name));
-			ctrl->minimum = 0;
-			ctrl->maximum = 255;
-			ctrl->step = 1;
-			ctrl->default_value = 128;
-			ctrl->flags = 0;
-			break;
-		case V4L2_CID_SATURATION:
-			ctrl->type = V4L2_CTRL_TYPE_INTEGER;
-			strncpy(ctrl->name, "Saturation", sizeof(ctrl->name));
-			ctrl->minimum = 0;
-			ctrl->maximum = 255;
-			ctrl->step = 1;
-			ctrl->default_value = 128;
-			ctrl->flags = 0;
-			break;
-		case V4L2_CID_HUE:
-			ctrl->type = V4L2_CTRL_TYPE_INTEGER;
-			strncpy(ctrl->name, "Hue", sizeof(ctrl->name));
-			ctrl->minimum = 0;
-			ctrl->maximum = 255;
-			ctrl->step = 1;
-			ctrl->default_value = 128;
-			ctrl->flags = 0;
-			break;
-		}
-		break;
-	}
-	case VIDIOC_S_CTRL:
-	{
-		struct v4l2_control *ctrl = arg;
-
-		switch (ctrl->id) {
-		case V4L2_CID_BRIGHTNESS:
-			if (ctrl->value > 255)
-				dec->brightness = 255;
-			else if (ctrl->value < 0)
-				dec->brightness = 0;
-			else
-				dec->brightness = ctrl->value;
-			write_reg(client, 0x12, dec->brightness, dec->channel);
-			break;
-		case V4L2_CID_CONTRAST:
-			if (ctrl->value > 255)
-				dec->contrast = 255;
-			else if (ctrl->value < 0)
-				dec->contrast = 0;
-			else
-				dec->contrast = ctrl->value;
-			write_reg(client, 0x11, dec->contrast, dec->channel);
-			break;
-		case V4L2_CID_SATURATION:
-			if (ctrl->value > 255)
-				dec->saturation = 255;
-			else if (ctrl->value < 0)
-				dec->saturation = 0;
-			else
-				dec->saturation = ctrl->value;
-			write_reg(client, 0x10, dec->saturation, dec->channel);
-			break;
-		case V4L2_CID_HUE:
-			if (ctrl->value > 255)
-				dec->hue = 255;
-			else if (ctrl->value < 0)
-				dec->hue = 0;
-			else
-				dec->hue = ctrl->value;
-			write_reg(client, 0x0f, dec->hue, dec->channel);
-			break;
-		}
-		break;
-	}
-	case VIDIOC_G_CTRL:
-	{
-		struct v4l2_control *ctrl = arg;
-
-		switch (ctrl->id) {
-		case V4L2_CID_BRIGHTNESS:
-			ctrl->value = dec->brightness;
-			break;
-		case V4L2_CID_CONTRAST:
-			ctrl->value = dec->contrast;
-			break;
-		case V4L2_CID_SATURATION:
-			ctrl->value = dec->saturation;
-			break;
-		case V4L2_CID_HUE:
-			ctrl->value = dec->hue;
-			break;
-		}
-		break;
-	}
-	default:
-		break;
-	}
 	return 0;
 }
 
-- 
1.7.7.6

[PATCH 06/10] staging: media: go7007: tw2804 ADC power control

[Volokh Konstantin]

switch off all ADC (max 4) with first init,
we control it then start/stop grabbing frame

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/wis-tw2804.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
index 7bd3058..34cf6b6 100644
--- a/drivers/staging/media/go7007/wis-tw2804.c
+++ b/drivers/staging/media/go7007/wis-tw2804.c
@@ -44,6 +44,7 @@ static u8 global_registers[] = {
 	0x3d, 0x80,
 	0x3e, 0x82,
 	0x3f, 0x82,
+	0x78, 0x0f,
 	0xff, 0xff, /* Terminator (reg 0xff does not exist) */
 };
 
-- 
1.7.7.6

[PATCH 07/10] staging: media: go7007: README TODO

[Volokh Konstantin]

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/README |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/media/go7007/README b/drivers/staging/media/go7007/README
index aeba132..237f45d 100644
--- a/drivers/staging/media/go7007/README
+++ b/drivers/staging/media/go7007/README
@@ -5,6 +5,32 @@ Todo:
 	  and added to the build.
 	- testing?
 	- handle churn in v4l layer.
+	- Some features for wis-tw2804 subdev control (comb filter,coring,IF comp,peak,more over...)
+	- Cropping&Scaling on tw2804
+	- Motion detector on tw2804, spatial&temporal sensitivity & masks control,velocity
+	- Control Output Format on tw2804
+	- go7007-v4l2.c need rewrite with new v4l2 style without nonstandard IO controls (set detector & bitrate)
+
+13/05/2012 3.4.0-rc+:
+Changes:
+	- Convert to V4L2 control framework
+
+05/05/2012 3.4.0-rc+:
+Changes:
+	- When go7007 reset device, i2c was not working (need rewrite GPIO5)
+	- As wis2804 has i2c_addr=0x00/*really*/, so need to set I2C_CLIENT_TEN flag for validity
+	- Some main nonzero initialization, rewrites with kzalloc instead kmalloc
+	- STATUS_SHUTDOWN was placed in incorrect place, so if firmware wasn`t loaded, we
+		failed v4l2_device_unregister with kernel panic (OOPS)
+	- Some new v4l2 style features as call_all(...s_stream...) for using subdev calls
+	- wis-tw2804.ko module code was incompatible with 3.4.x branch in initialization v4l2_subdev parts.
+		now i2c_get_clientdata(...) contains v4l2_subdev struct instead non standard wis_tw2804 struct
+
+Adds:
+	- Switch between 2 composite video inputs on channel: VIN[1,2,3,4]A and VIN[1,2,3,4]B
+	- Additional chipset wis2804 controls with: gain,auto gain,inputs[0,1],color kill,chroma gain,gain balances,
+		for all 4 channels (from tw2804.pdf)
+	- Power control for each 4 ADC up when s_stream(...,1), down otherwise in wis-tw2804 module
 
 Please send patches to Greg Kroah-Hartman <greg@linuxfoundation.org> and Cc: Ross
 Cohen <rcohen@snurgle.org> as well.
-- 
1.7.7.6

[PATCH 08/10] staging: media: go7007: some additional functionality for sub.

[Volokh Konstantin]

zero cleaning allocation for fix.
unregistering bug fixing.

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/go7007-driver.c |    2 +-
 drivers/staging/media/go7007/go7007-usb.c    |    2 +-
 drivers/staging/media/go7007/go7007-v4l2.c   |    7 +++++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/go7007/go7007-driver.c b/drivers/staging/media/go7007/go7007-driver.c
index ece2dd1..a66e339 100644
--- a/drivers/staging/media/go7007/go7007-driver.c
+++ b/drivers/staging/media/go7007/go7007-driver.c
@@ -571,7 +571,7 @@ struct go7007 *go7007_alloc(struct go7007_board_info *board, struct device *dev)
 	struct go7007 *go;
 	int i;
 
-	go = kmalloc(sizeof(struct go7007), GFP_KERNEL);
+	go = kzalloc(sizeof(struct go7007), GFP_KERNEL);
 	if (go == NULL)
 		return NULL;
 	go->dev = dev;
diff --git a/drivers/staging/media/go7007/go7007-usb.c b/drivers/staging/media/go7007/go7007-usb.c
index 5443e25..a6cad15 100644
--- a/drivers/staging/media/go7007/go7007-usb.c
+++ b/drivers/staging/media/go7007/go7007-usb.c
@@ -1245,7 +1245,6 @@ static void go7007_usb_disconnect(struct usb_interface *intf)
 	struct urb *vurb, *aurb;
 	int i;
 
-	go->status = STATUS_SHUTDOWN;
 	usb_kill_urb(usb->intr_urb);
 
 	/* Free USB-related structs */
@@ -1269,6 +1268,7 @@ static void go7007_usb_disconnect(struct usb_interface *intf)
 	kfree(go->hpi_context);
 
 	go7007_remove(go);
+	go->status = STATUS_SHUTDOWN;
 }
 
 static struct usb_driver go7007_usb_driver = {
diff --git a/drivers/staging/media/go7007/go7007-v4l2.c b/drivers/staging/media/go7007/go7007-v4l2.c
index c184ad3..b8f2eb6 100644
--- a/drivers/staging/media/go7007/go7007-v4l2.c
+++ b/drivers/staging/media/go7007/go7007-v4l2.c
@@ -98,7 +98,7 @@ static int go7007_open(struct file *file)
 
 	if (go->status != STATUS_ONLINE)
 		return -EBUSY;
-	gofh = kmalloc(sizeof(struct go7007_file), GFP_KERNEL);
+	gofh = kzalloc(sizeof(struct go7007_file), GFP_KERNEL);
 	if (gofh == NULL)
 		return -ENOMEM;
 	++go->ref_count;
@@ -953,6 +953,7 @@ static int vidioc_streamon(struct file *file, void *priv,
 	}
 	mutex_unlock(&go->hw_lock);
 	mutex_unlock(&gofh->lock);
+	call_all(&go->v4l2_dev, video, s_stream, 1);
 
 	return retval;
 }
@@ -968,6 +969,7 @@ static int vidioc_streamoff(struct file *file, void *priv,
 	mutex_lock(&gofh->lock);
 	go7007_streamoff(go);
 	mutex_unlock(&gofh->lock);
+	call_all(&go->v4l2_dev, video, s_stream, 0);
 
 	return 0;
 }
@@ -1832,5 +1834,6 @@ void go7007_v4l2_remove(struct go7007 *go)
 	mutex_unlock(&go->hw_lock);
 	if (go->video_dev)
 		video_unregister_device(go->video_dev);
-	v4l2_device_unregister(&go->v4l2_dev);
+	if (go->status != STATUS_SHUTDOWN)
+		v4l2_device_unregister(&go->v4l2_dev);
 }
-- 
1.7.7.6

[PATCH 09/10] staging: media: go7007: Adlink_MPG24 board initialization fix.

[Volokh Konstantin]

setting via gpio i2c bus support at every time then init encoder.

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/go7007-driver.c |    5 +++++
 drivers/staging/media/go7007/go7007-usb.c    |    3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/go7007/go7007-driver.c b/drivers/staging/media/go7007/go7007-driver.c
index a66e339..c0ea312 100644
--- a/drivers/staging/media/go7007/go7007-driver.c
+++ b/drivers/staging/media/go7007/go7007-driver.c
@@ -173,6 +173,11 @@ static int go7007_init_encoder(struct go7007 *go)
 		go7007_write_addr(go, 0x3c82, 0x0001);
 		go7007_write_addr(go, 0x3c80, 0x00fe);
 	}
+	if (go->board_id == GO7007_BOARDID_ADLINK_MPG24) {
+		/* set GPIO5 to be an output, currently low */
+		go7007_write_addr(go, 0x3c82, 0x0000);
+		go7007_write_addr(go, 0x3c80, 0x00df);
+	}
 	return 0;
 }
 
diff --git a/drivers/staging/media/go7007/go7007-usb.c b/drivers/staging/media/go7007/go7007-usb.c
index a6cad15..9dbf5ec 100644
--- a/drivers/staging/media/go7007/go7007-usb.c
+++ b/drivers/staging/media/go7007/go7007-usb.c
@@ -1110,9 +1110,6 @@ static int go7007_usb_probe(struct usb_interface *intf,
 			} else {
 				u16 channel;
 
-				/* set GPIO5 to be an output, currently low */
-				go7007_write_addr(go, 0x3c82, 0x0000);
-				go7007_write_addr(go, 0x3c80, 0x00df);
 				/* read channel number from GPIO[1:0] */
 				go7007_read_addr(go, 0x3c81, &channel);
 				channel &= 0x3;
-- 
1.7.7.6

[PATCH 10/10] staging: media: go7007: some i2c initialization

[Volokh Konstantin]

i2c initialization via struct item
as tw2804 have 0x00 i2c address, so need to use I2C_CLIENT_TEN
  flag for validity

Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
---
 drivers/staging/media/go7007/go7007-driver.c |   20 ++++++++++++--------
 drivers/staging/media/go7007/go7007-priv.h   |    2 +-
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/go7007/go7007-driver.c b/drivers/staging/media/go7007/go7007-driver.c
index c0ea312..2dff9b5 100644
--- a/drivers/staging/media/go7007/go7007-driver.c
+++ b/drivers/staging/media/go7007/go7007-driver.c
@@ -197,17 +197,23 @@ int go7007_reset_encoder(struct go7007 *go)
 /*
  * Attempt to instantiate an I2C client by ID, probably loading a module.
  */
-static int init_i2c_module(struct i2c_adapter *adapter, const char *type,
-			   int addr)
+static int init_i2c_module(struct i2c_adapter *adapter, const struct go_i2c *const i2c)
 {
 	struct go7007 *go = i2c_get_adapdata(adapter);
 	struct v4l2_device *v4l2_dev = &go->v4l2_dev;
+	struct i2c_board_info info;
 
-	if (v4l2_i2c_new_subdev(v4l2_dev, adapter, type, addr, NULL))
+	memset(&info, 0, sizeof(info));
+	strlcpy(info.type, i2c->type, sizeof(info.type));
+	info.addr = i2c->addr;
+
+	if (i2c->id == I2C_DRIVERID_WIS_TW2804)
+		info.flags |= I2C_CLIENT_TEN;
+	if (v4l2_i2c_new_subdev_board(v4l2_dev, adapter, &info, NULL))
 		return 0;
 
-	printk(KERN_INFO "go7007: probing for module i2c:%s failed\n", type);
-	return -1;
+	printk(KERN_INFO "go7007: probing for module i2c:%s failed\n", i2c->type);
+	return -EINVAL;
 }
 
 /*
@@ -243,9 +249,7 @@ int go7007_register_encoder(struct go7007 *go)
 	}
 	if (go->i2c_adapter_online) {
 		for (i = 0; i < go->board_info->num_i2c_devs; ++i)
-			init_i2c_module(&go->i2c_adapter,
-					go->board_info->i2c_devs[i].type,
-					go->board_info->i2c_devs[i].addr);
+			init_i2c_module(&go->i2c_adapter, &go->board_info->i2c_devs[i]);
 		if (go->board_id == GO7007_BOARDID_ADLINK_MPG24)
 			i2c_clients_command(&go->i2c_adapter,
 				DECODER_SET_CHANNEL, &go->channel_number);
diff --git a/drivers/staging/media/go7007/go7007-priv.h b/drivers/staging/media/go7007/go7007-priv.h
index b58c394..b7b939a 100644
--- a/drivers/staging/media/go7007/go7007-priv.h
+++ b/drivers/staging/media/go7007/go7007-priv.h
@@ -88,7 +88,7 @@ struct go7007_board_info {
 	int audio_bclk_div;
 	int audio_main_div;
 	int num_i2c_devs;
-	struct {
+	struct go_i2c {
 		const char *type;
 		int id;
 		int addr;
-- 
1.7.7.6

Re: [PATCH 01/10] staging: media: go7007: Some additional code for TW2804 driver functionality

[Dan Carpenter]

This patch got corrupted somewhere so it doesn't apply.  There are
some minor style issues as well.

On Wed, Aug 22, 2012 at 02:45:10PM +0400, Volokh Konstantin wrote:
> - using new v4l2 framework controls
> - function for reading volatile controls via i2c bus
> - separate V4L2_CID_ ctrls into each V4L2 calls
> 
> Signed-off-by: Volokh Konstantin <volokh84@gmail.com>
> ---
>  drivers/staging/media/go7007/wis-tw2804.c |  248 +++++++++++++++++++++++++++++
>  1 files changed, 248 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/media/go7007/wis-tw2804.c b/drivers/staging/media/go7007/wis-tw2804.c
> index 9134f03..05851d3 100644
> --- a/drivers/staging/media/go7007/wis-tw2804.c
> +++ b/drivers/staging/media/go7007/wis-tw2804.c
> @@ -21,10 +21,18 @@
>  #include <linux/videodev2.h>
>  #include <linux/ioctl.h>
>  #include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-ctrls.h>
>  
>  #include "wis-i2c.h"
>  
>  struct wis_tw2804 {
> +	struct v4l2_subdev sd;
> +	struct v4l2_ctrl_handler hdl;
> +	u8 channel:2;
> +	u8 input:1;
>  	int channel;
>  	int norm;
>  	int brightness;
> @@ -116,9 +124,246 @@ static int write_regs(struct i2c_client *client, u8 *regs, int channel)
>  		if (i2c_smbus_write_byte_data(client,
>  				regs[i] | (channel << 6), regs[i + 1]) < 0)
>  			return -1;
> +static s32 read_reg(struct i2c_client *client, u8 reg, u8 channel)
> +{

Look at the five lines above.  This patch introduces a new function
in the middle of an existing function.

> +	return i2c_smbus_read_byte_data(client, (reg) | (channel << 6));
> +}
> +
> +inline struct wis_tw2804 *to_state(struct v4l2_subdev *sd)

Should this be static?  Otherwise the name is very generic.

> +{
> +	return container_of(sd, struct wis_tw2804, sd);
> +}
> +
> +inline struct wis_tw2804 *to_state_from_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	return container_of(ctrl->handler, struct wis_tw2804, hdl);
> +}
> +
> +static int tw2804_log_status(struct v4l2_subdev *sd)
> +{
> +	struct wis_tw2804 *state = to_state(sd);

Blank line here.

> +	v4l2_info(sd, "Standard: %s\n",
> +			state->norm == V4L2_STD_NTSC ? "NTSC" :
> +			state->norm == V4L2_STD_PAL ? "PAL" : "unknown");
> +	v4l2_info(sd, "Channel: %d\n", state->channel);
> +	v4l2_info(sd, "Input: %d\n", state->input);
> +	v4l2_ctrl_handler_log_status(&state->hdl, sd->name);
> +	return 0;
> +}
> +
> +static s32 get_ctrl_addr(int ctrl)

Just use int instead of s32.  s32 and int are the same but s32 is
only needed in special circumstances.  It is for when the hardware
or protocol specifies a signed 32 bit.  You use it throughout
instead of using ints.

> +{
> +	switch (ctrl) {
> +	case V4L2_CID_BRIGHTNESS:
> +		return 0x12;
> +	case V4L2_CID_CONTRAST:
> +		return 0x11;
> +	case V4L2_CID_SATURATION:
> +		return 0x10;
> +	case V4L2_CID_HUE:
> +		return 0x0f;
> +	case V4L2_CID_AUTOGAIN:
> +		return 0x02;
> +	case V4L2_CID_COLOR_KILLER:
> +		return 0x14;
> +	case V4L2_CID_GAIN:
> +		return 0x3c;
> +	case V4L2_CID_CHROMA_GAIN:
> +		return 0x3d;
> +	case V4L2_CID_RED_BALANCE:
> +		return 0x3f;
> +	case V4L2_CID_BLUE_BALANCE:
> +		return 0x3e;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int tw2804_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = &to_state_from_ctrl(ctrl)->sd;
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	s32 addr = get_ctrl_addr(ctrl->id);
> +
> +	if (addr == -EINVAL)
> +		return -EINVAL;

This should be:
	if (addr < 0)
		return addr;

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_GAIN:
> +	case V4L2_CID_CHROMA_GAIN:
> +	case V4L2_CID_RED_BALANCE:
> +	case V4L2_CID_BLUE_BALANCE:
> +		ctrl->cur.val = read_reg(client, addr, 0);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int tw2804_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct wis_tw2804 *state = to_state_from_ctrl(ctrl);
> +	struct v4l2_subdev *sd = &state->sd;
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	s32 reg = ctrl->val;
> +	s32 addr = get_ctrl_addr(ctrl->id);
> +
> +	if (addr == -EINVAL)
> +		return -EINVAL;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_AUTOGAIN:
> +		reg = read_reg(client, addr, state->channel);
> +		if (reg > 0) {
> +			if (ctrl->val == 0)
> +				reg &= ~(1<<7);
> +			else
> +				reg |= 1<<7;

Spaces around the operations:
				reg &= ~(1 << 7);
			else
				reg |= 1 << 7;

> +		} else
> +			return reg;
> +		break;
> +	case V4L2_CID_COLOR_KILLER:
> +		reg = read_reg(client, addr, state->channel);
> +		if (reg > 0)
> +			reg = (reg & ~(0x03)) | (ctrl->val == 0 ? 0x02 : 0x03);
> +		else
> +			return reg;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	reg = reg > 255 ? 255 : (reg < 0 ? 0 : reg);
> +	reg = write_reg(client, addr, (u8)reg,
                                      ^^^^
Not needed.



> +			ctrl->id == V4L2_CID_GAIN ||
> +			ctrl->id == V4L2_CID_CHROMA_GAIN ||
> +			ctrl->id == V4L2_CID_RED_BALANCE ||
> +			ctrl->id == V4L2_CID_BLUE_BALANCE ? 0 : state->channel);
> +
> +	if (reg < 0) {
> +		v4l2_err(sd, "Can`t set_ctrl value:id=%d;value=%d\n", ctrl->id,
> +								    ctrl->val);
> +		return reg;
> +	}
> +	return 0;
> +}
> +
> +static int tw2804_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
> +{
> +	struct wis_tw2804 *dec = to_state(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +

No blank line here.

> +	u8 regs[] = {
> +		0x01, norm&V4L2_STD_NTSC ? 0xc4 : 0x84,
> +		0x09, norm&V4L2_STD_NTSC ? 0x07 : 0x04,
> +		0x0a, norm&V4L2_STD_NTSC ? 0xf0 : 0x20,
> +		0x0b, norm&V4L2_STD_NTSC ? 0x07 : 0x04,
> +		0x0c, norm&V4L2_STD_NTSC ? 0xf0 : 0x20,
> +		0x0d, norm&V4L2_STD_NTSC ? 0x40 : 0x4a,
> +		0x16, norm&V4L2_STD_NTSC ? 0x00 : 0x40,
> +		0x17, norm&V4L2_STD_NTSC ? 0x00 : 0x40,
> +		0x20, norm&V4L2_STD_NTSC ? 0x07 : 0x0f,
> +		0x21, norm&V4L2_STD_NTSC ? 0x07 : 0x0f,

This is hard to read without proper white space:
		0x01, norm & V4L2_STD_NTSC ? 0xc4 : 0x84,
		0x09, norm & V4L2_STD_NTSC ? 0x07 : 0x04,

> +		0xff, 0xff,
> +	};

Put a blank line here.

> +	write_regs(client, regs, dec->channel);
> +	dec->norm = norm;
> +	return 0;
> +}
> +
> +static int tw2804_g_chip_ident(struct v4l2_subdev *sd,
> +				struct v4l2_dbg_chip_ident *chip)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	return v4l2_chip_ident_i2c_client(client, chip,
> +					V4L2_IDENT_TW2804, 0x0e);
> +}
> +
> +static const struct v4l2_ctrl_ops tw2804_ctrl_ops = {
> +	.g_volatile_ctrl = tw2804_g_volatile_ctrl,
> +	.s_ctrl = tw2804_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_core_ops tw2804_core_ops = {
> +	.log_status = tw2804_log_status,
> +	.g_chip_ident = tw2804_g_chip_ident,
> +	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
> +	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
> +	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
> +	.g_ctrl = v4l2_subdev_g_ctrl,
> +	.s_ctrl = v4l2_subdev_s_ctrl,
> +	.queryctrl = v4l2_subdev_queryctrl,
> +	.querymenu = v4l2_subdev_querymenu,
> +	.s_std = tw2804_s_std,
> +};
> +
> +static int tw2804_s_video_routing(struct v4l2_subdev *sd, u32 input, u32 output,
> +	u32 config)
> +{
> +	struct wis_tw2804 *dec = to_state(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	s32 reg = 0;

The initialization is not needed here.

> +
> +	if (0 > input || input > 1)
> +		return -EINVAL;
> +
> +	if (input == dec->input)
> +		return 0;
> +
> +	reg = read_reg(client, 0x22, dec->channel);

Move the error handling next to the function call.  It makes the
code simpler and you can pull everything in an indent level.

	reg = read_reg(client, 0x22, dec->channel);
	if (reg < 0)
		return reg;

	if (input == 0)
		reg &= ~(1 << 2);
	else
		reg |= 1 << 2;

	reg = write_reg(client, 0x22, reg, dec->channel);
	if (reg < 0)
		return reg;

	dec->input = input;
	return 0;

regards,
dan carpenter

> +
> +	if (reg >= 0) {
> +		if (input == 0)
> +			reg &= ~(1<<2);
> +		else
> +			reg |= 1<<2;
> +		reg = write_reg(client, 0x22, (u8)reg, dec->channel);
> +	}
> +
> +	if (reg >= 0)
> +		dec->input = input;
> +	else
> +		return reg;
>  	return 0;
>  }
>  
> +static int tw2804_s_mbus_fmt(struct v4l2_subdev *sd,
> +	struct v4l2_mbus_framefmt *fmt)
> +{
> +	/*TODO need select between 3fmt:
> +	 * bt_656,
> +	 * bt_601_8bit,
> +	 * bt_656_dual,
> +	 */
> +	return 0;
> +}
> +
> +static int tw2804_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct wis_tw2804 *dec = to_state(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	u32 reg = read_reg(client, 0x78, 0);
> +
> +	if (enable == 1)
> +		write_reg(client, 0x78, reg & ~(1<<dec->channel), 0);
> +	else
> +		write_reg(client, 0x78, reg | (1<<dec->channel), 0);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops tw2804_video_ops = {
> +	.s_routing = tw2804_s_video_routing,
> +	.s_mbus_fmt = tw2804_s_mbus_fmt,
> +	.s_stream = tw2804_s_stream,
> +};
> +
> +static const struct v4l2_subdev_ops tw2804_ops = {
> +	.core = &tw2804_core_ops,
> +	.video = &tw2804_video_ops,
> +};
> +
>  static int wis_tw2804_command(struct i2c_client *client,
>  				unsigned int cmd, void *arg)
>  {
> @@ -355,3 +600,6 @@ module_init(wis_tw2804_init);
>  module_exit(wis_tw2804_cleanup);
>  
>  MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TW2804/TW2802 V4L2 i2c driver");
> +MODULE_AUTHOR("Volokh Konstantin <volokh84@gmail.com>");
> +MODULE_AUTHOR("Micronas USA Inc");
> -- 
> 1.7.7.6
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel