Re: [PATCH v2] media: ths7353: add support for ths7353 video amplifier

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

 



Hi Prabhakar,

Here is my review. There are a few things that need to be fixed before I
can Ack it.

On Tue 5 February 2013 20:20:21 Prabhakar Lad wrote:
> From: Lad, Prabhakar <prabhakar.lad@xxxxxx>
> 
> The patch adds support for THS7353 video amplifier.
> The the THS7353 amplifier is very much similar to the
> existing THS7303 video amplifier driver.
> This patch appropriately makes changes to the existing
> ths7303 driver and adds support for the THS7353.
> This patch also adds V4L2_IDENT_THS7353 for the THS7353
> chip and appropriate changes to Kconfig file for building.
> 
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> Signed-off-by: Martin Bugge <marbugge@xxxxxxxxx>
> Cc: Chaithrika U S <chaithrika@xxxxxx>
> ---
>  Changes for v2:
>  1: Merged the driver in existing ths7303 driver.
>  2: Merged the patch which adds the chip indent in same patch.
>  
>  drivers/media/i2c/Kconfig       |    4 +-
>  drivers/media/i2c/ths7303.c     |  334 +++++++++++++++++++++++++++++++++------
>  include/media/ths7303.h         |   35 ++++
>  include/media/v4l2-chip-ident.h |    3 +
>  4 files changed, 327 insertions(+), 49 deletions(-)
>  create mode 100644 include/media/ths7303.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 1e4b2d0..2e02916 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -563,9 +563,9 @@ comment "Miscelaneous helper chips"
>  
>  config VIDEO_THS7303
>  	tristate "THS7303 Video Amplifier"
> -	depends on I2C
> +	depends on VIDEO_V4L2 && I2C
>  	help
> -	  Support for TI THS7303 video amplifier
> +	  Support for TI THS7303/53 video amplifier
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ths7303.
> diff --git a/drivers/media/i2c/ths7303.c b/drivers/media/i2c/ths7303.c
> index e747524..e7f8c59 100644
> --- a/drivers/media/i2c/ths7303.c
> +++ b/drivers/media/i2c/ths7303.c
> @@ -1,7 +1,15 @@
>  /*
> - * ths7303- THS7303 Video Amplifier driver
> + * ths7303/53- THS7303/53 Video Amplifier driver
>   *
>   * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
> + * Copyright 2013 Cisco Systems, Inc. and/or its affiliates.
> + *
> + * Author: Chaithrika U S <chaithrika@xxxxxx>
> + *
> + * Contributors:
> + *     Lad, Prabhakar <prabhakar.lad@xxxxxx>
> + *     Hans Verkuil <hans.verkuil@xxxxxxxxx>
> + *     Martin Bugge <marbugge@xxxxxxxxx>
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License as
> @@ -13,25 +21,27 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <linux/kernel.h>
> -#include <linux/init.h>
> -#include <linux/ctype.h>
> -#include <linux/slab.h>
>  #include <linux/i2c.h>
> -#include <linux/device.h>
> -#include <linux/delay.h>
>  #include <linux/module.h>
> -#include <linux/uaccess.h>
> -#include <linux/videodev2.h>
> +#include <linux/slab.h>
>  
> -#include <media/v4l2-device.h>
> -#include <media/v4l2-subdev.h>
> +#include <media/ths7303.h>
>  #include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-device.h>
>  
>  #define THS7303_CHANNEL_1	1
>  #define THS7303_CHANNEL_2	2
>  #define THS7303_CHANNEL_3	3
>  
> +struct ths7303_state {
> +	struct v4l2_subdev sd;
> +	struct ths7303_platform_data pdata;
> +	struct v4l2_dv_timings dv_timings;
> +	int std_id;
> +	int stream_on;
> +	int driver_data;
> +};
> +
>  enum ths7303_filter_mode {
>  	THS7303_FILTER_MODE_480I_576I,
>  	THS7303_FILTER_MODE_480P_576P,
> @@ -48,64 +58,97 @@ static int debug;
>  module_param(debug, int, 0644);
>  MODULE_PARM_DESC(debug, "Debug level 0-1");
>  
> +static inline struct ths7303_state *to_state(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct ths7303_state, sd);
> +}
> +
> +static int ths7303_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int ths7303_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = i2c_smbus_write_byte_data(client, reg, val);
> +		if (ret == 0)
> +			return 0;
> +	}
> +	return ret;
> +}
> +
>  /* following function is used to set ths7303 */
>  int ths7303_setval(struct v4l2_subdev *sd, enum ths7303_filter_mode mode)
>  {
> -	u8 input_bias_chroma = 3;
> -	u8 input_bias_luma = 3;
> -	int disable = 0;
> -	int err = 0;
> -	u8 val = 0;
> -	u8 temp;
> -
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ths7303_state *state = to_state(sd);
> +	struct ths7303_platform_data *pdata = &state->pdata;
> +	u8 val, sel = 0;
> +	int err, disable = 0;
>  
>  	if (!client)
>  		return -EINVAL;
>  
> +	if (!state->stream_on) {
> +		ths7303_write(sd, THS7303_CHANNEL_1,
> +			      (ths7303_read(sd, THS7303_CHANNEL_1) & 0xf8) |
> +			      0x00);
> +		ths7303_write(sd, THS7303_CHANNEL_2,
> +			      (ths7303_read(sd, THS7303_CHANNEL_2) & 0xf8) |
> +			      0x00);
> +		ths7303_write(sd, THS7303_CHANNEL_3,
> +			      (ths7303_read(sd, THS7303_CHANNEL_3) & 0xf8) |
> +			      0x00);
> +		return 0;
> +	}
> +
>  	switch (mode) {
>  	case THS7303_FILTER_MODE_1080P:
> -		val = (3 << 6);
> -		val |= (3 << 3);
> +		sel = 0x3;	/*1080p and SXGA/UXGA */
>  		break;
>  	case THS7303_FILTER_MODE_720P_1080I:
> -		val = (2 << 6);
> -		val |= (2 << 3);
> +		sel = 0x2;	/*720p, 1080i and SVGA/XGA */
>  		break;
>  	case THS7303_FILTER_MODE_480P_576P:
> -		val = (1 << 6);
> -		val |= (1 << 3);
> +		sel = 0x1;	/* EDTV 480p/576p and VGA */
>  		break;
>  	case THS7303_FILTER_MODE_480I_576I:
> +		sel = 0x0;	/* SDTV, S-Video, 480i/576i */
>  		break;
> -	case THS7303_FILTER_MODE_DISABLE:
> -		pr_info("mode disabled\n");
> -		/* disable all channels */
> -		disable = 1;
>  	default:
>  		/* disable all channels */
>  		disable = 1;
>  	}
> -	/* Setup channel 2 - Luma - Green */
> -	temp = val;
> +
> +	val = (sel << 6) | (sel << 3);
>  	if (!disable)
> -		val |= input_bias_luma;
> -	err = i2c_smbus_write_byte_data(client, THS7303_CHANNEL_2, val);
> +		val |= (pdata->ch_1 & 0x27);
> +	err = ths7303_write(sd, 0x01, val);
>  	if (err)
>  		goto out;
>  
> -	/* setup two chroma channels */
> +	val = (sel << 6) | (sel << 3);
>  	if (!disable)
> -		temp |= input_bias_chroma;
> -
> -	err = i2c_smbus_write_byte_data(client, THS7303_CHANNEL_1, temp);
> +		val |= (pdata->ch_2 & 0x27);
> +	err = ths7303_write(sd, 0x02, val);
>  	if (err)
>  		goto out;
>  
> -	err = i2c_smbus_write_byte_data(client, THS7303_CHANNEL_3, temp);
> +	val = (sel << 6) | (sel << 3);
> +	if (!disable)
> +		val |= (pdata->ch_3 & 0x27);
> +	err = ths7303_write(sd, 0x03, val);
>  	if (err)
>  		goto out;
> -	return err;
> +
> +	return 0;
>  out:
>  	pr_info("write byte data failed\n");
>  	return err;
> @@ -113,16 +156,47 @@ out:
>  
>  static int ths7303_s_std_output(struct v4l2_subdev *sd, v4l2_std_id norm)
>  {
> -	if (norm & (V4L2_STD_ALL & ~V4L2_STD_SECAM))
> +	struct ths7303_state *state = to_state(sd);
> +
> +	if (norm & (V4L2_STD_ALL & ~V4L2_STD_SECAM)) {
> +		state->std_id = 1;
> +		return ths7303_setval(sd, THS7303_FILTER_MODE_480I_576I);
> +	}
> +
> +	return ths7303_setval(sd, THS7303_FILTER_MODE_DISABLE);
> +}
> +
> +static int ths7303_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct ths7303_state *state = to_state(sd);
> +	u32 height = state->dv_timings.bt.height;
> +	int interlaced = state->dv_timings.bt.interlaced;

Instead of storing the whole dv_timings struct, just store the relevant
fields.

> +	int res;
> +
> +	state->stream_on = enable;
> +
> +	if (state->std_id)
>  		return ths7303_setval(sd, THS7303_FILTER_MODE_480I_576I);
> +
> +	if (height == 1080 && !interlaced)

You can't use the height as such. It is the pixelclock that determines which
mode to use. What if I select a 1600x1200 format? That should still work!
Cisco definitely needs this functionality.

The code I mailed you earlier does this selection based on the pixelclock.

> +		res = ths7303_setval(sd, THS7303_FILTER_MODE_1080P);
> +	else if ((height == 720 && !interlaced) ||
> +			(height == 1080 && interlaced))
> +		res = ths7303_setval(sd, THS7303_FILTER_MODE_720P_1080I);
> +	else if ((height == 480 || height == 576) && !interlaced)
> +		res = ths7303_setval(sd, THS7303_FILTER_MODE_480P_576P);
>  	else
> -		return ths7303_setval(sd, THS7303_FILTER_MODE_DISABLE);
> +		/* disable all channels */
> +		res = ths7303_setval(sd, THS7303_FILTER_MODE_DISABLE);
> +
> +	return res;
>  }
>  
>  /* for setting filter for HD output */
>  static int ths7303_s_dv_timings(struct v4l2_subdev *sd,
>  			       struct v4l2_dv_timings *dv_timings)
>  {
> +	struct ths7303_state *state = to_state(sd);
>  	u32 height = dv_timings->bt.height;
>  	int interlaced = dv_timings->bt.interlaced;
>  	int res = 0;
> @@ -138,6 +212,11 @@ static int ths7303_s_dv_timings(struct v4l2_subdev *sd,
>  		/* disable all channels */
>  		res = ths7303_setval(sd, THS7303_FILTER_MODE_DISABLE);
>  
> +	if (!res) {
> +		state->dv_timings = *dv_timings;
> +		state->std_id = 0;
> +	}
> +
>  	return res;
>  }
>  
> @@ -145,17 +224,160 @@ static int ths7303_g_chip_ident(struct v4l2_subdev *sd,
>  				struct v4l2_dbg_chip_ident *chip)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	u32 indent = V4L2_IDENT_THS7303;
> +	struct ths7303_state *state = to_state(sd);
>  
> -	return v4l2_chip_ident_i2c_client(client, chip, V4L2_IDENT_THS7303, 0);
> +	if (state->driver_data)
> +		indent = V4L2_IDENT_THS7353;
> +
> +	return v4l2_chip_ident_i2c_client(client, chip, indent, 0);
>  }
>  
>  static const struct v4l2_subdev_video_ops ths7303_video_ops = {
> +	.s_stream	= ths7303_s_stream,
>  	.s_std_output	= ths7303_s_std_output,
> -	.s_dv_timings    = ths7303_s_dv_timings,
> +	.s_dv_timings   = ths7303_s_dv_timings,
> +};
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +
> +static int ths7303_g_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	if (!v4l2_chip_match_i2c_client(client, &reg->match))
> +		return -EINVAL;
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	reg->size = 1;
> +	reg->val = ths7303_read(sd, reg->reg);
> +	return 0;
> +}
> +
> +static int ths7303_s_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	if (!v4l2_chip_match_i2c_client(client, &reg->match))
> +		return -EINVAL;
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	ths7303_write(sd, reg->reg, reg->val);
> +	return 0;
> +}
> +#endif
> +
> +struct channel_register_bit_decoder {
> +	uint8_t stc_lpf_sel;
> +	uint8_t in_mux_sel;
> +	uint8_t lpf_freq_sel;
> +	uint8_t in_bias_sel_dis_cont;
> +};
> +
> +static const char *stc_lpf_sel_txt[4] = {

Use static const char * const (I think checkpatch complains about that
these days). Also do this for the static const arrays below.

> +	"500-kHz Filter",
> +	"2.5-MHz Filter",
> +	"5-MHz Filter",
> +	"5-MHz Filter",
>  };
>  
> +static const char *in_mux_sel_txt[2] = {
> +	"Input A Select",
> +	"Input B Select",
> +};
> +
> +static const char *lpf_freq_sel_txt[4] = {
> +	"9-MHz LPF",
> +	"16-MHz LPF",
> +	"35-MHz LPF",
> +	"Bypass LPF",
> +};
> +
> +static const char *in_bias_sel_dis_cont_txt[8] = {
> +	"Disable Channel",
> +	"Mute Function - No Output",
> +	"DC Bias Select",
> +	"DC Bias + 250 mV Offset Select",
> +	"AC Bias Select",
> +	"Sync Tip Clamp with low bias",
> +	"Sync Tip Clamp with mid bias",
> +	"Sync Tip Clamp with high bias",
> +};
> +
> +static void
> +parse_channel_register(struct channel_register_bit_decoder *ch_reg, u8 val)
> +{
> +	ch_reg->stc_lpf_sel = (val >> 6) & 0x3;
> +	ch_reg->in_mux_sel = (val >> 5) & 0x1;
> +	ch_reg->lpf_freq_sel = (val >> 3) & 0x3;
> +	ch_reg->in_bias_sel_dis_cont = (val >> 0) & 0x7;
> +}

I'd merge this function into log_channel_status. That gets rid of the
channel_register_bit_decoder struct as well.

> +
> +static void ths7303_log_channel_status(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct channel_register_bit_decoder ch_reg;
> +	u8 val = ths7303_read(sd, reg);
> +
> +	if ((val & 0x7) == 0) {
> +		v4l2_info(sd, "Channel %d Off\n", reg);
> +		return;
> +	}
> +
> +	parse_channel_register(&ch_reg, val);
> +
> +	v4l2_info(sd, "Channel %d On\n", reg);
> +	v4l2_info(sd, "  value 0x%x\n", val);
> +	v4l2_info(sd, "  %s\n", stc_lpf_sel_txt[ch_reg.stc_lpf_sel]);
> +	v4l2_info(sd, "  %s\n", in_mux_sel_txt[ch_reg.in_mux_sel]);
> +	v4l2_info(sd, "  %s\n", lpf_freq_sel_txt[ch_reg.lpf_freq_sel]);
> +	v4l2_info(sd, "  %s\n",
> +		  in_bias_sel_dis_cont_txt[ch_reg.in_bias_sel_dis_cont]);
> +}
> +
> +static int ths7303_log_status(struct v4l2_subdev *sd)
> +{
> +	struct ths7303_state *state = to_state(sd);
> +
> +	v4l2_info(sd, "stream %s\n", state->stream_on ? "On" : "Off");
> +
> +	if (state->dv_timings.type == V4L2_DV_BT_656_1120) {
> +		struct v4l2_bt_timings *bt = bt = &state->dv_timings.bt;
> +		u32 frame_width, frame_height;
> +
> +		frame_width = bt->width + bt->hfrontporch +
> +			      bt->hsync + bt->hbackporch;
> +		frame_height = bt->height + bt->vfrontporch +
> +			       bt->vsync + bt->vbackporch;
> +		v4l2_info(sd,
> +			  "timings: %dx%d%s%d (%dx%d). Pix freq. = %d Hz. Polarities = 0x%x\n",
> +			  bt->width, bt->height, bt->interlaced ? "i" : "p",
> +			  (frame_height * frame_width) > 0 ?
> +			  (int)bt->pixelclock /
> +			  (frame_height * frame_width) : 0,
> +			  frame_width, frame_height,
> +			  (int)bt->pixelclock, bt->polarities);
> +	} else {
> +		v4l2_info(sd, "no timings set\n");
> +	}
> +
> +	ths7303_log_channel_status(sd, THS7303_CHANNEL_1);
> +	ths7303_log_channel_status(sd, THS7303_CHANNEL_2);
> +	ths7303_log_channel_status(sd, THS7303_CHANNEL_3);
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_core_ops ths7303_core_ops = {
>  	.g_chip_ident = ths7303_g_chip_ident,
> +	.log_status = ths7303_log_status,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	.g_register = ths7303_g_register,
> +	.s_register = ths7303_s_register,
> +#endif
>  };
>  
>  static const struct v4l2_subdev_ops ths7303_ops = {
> @@ -166,8 +388,9 @@ static const struct v4l2_subdev_ops ths7303_ops = {
>  static int ths7303_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> +	struct ths7303_platform_data *pdata = client->dev.platform_data;
> +	struct ths7303_state *state;
>  	struct v4l2_subdev *sd;
> -	v4l2_std_id std_id = V4L2_STD_NTSC;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>  		return -ENODEV;
> @@ -175,13 +398,29 @@ static int ths7303_probe(struct i2c_client *client,
>  	v4l_info(client, "chip found @ 0x%x (%s)\n",
>  			client->addr << 1, client->adapter->name);
>  
> -	sd = devm_kzalloc(&client->dev, sizeof(struct v4l2_subdev), GFP_KERNEL);
> -	if (sd == NULL)
> +	state = devm_kzalloc(&client->dev, sizeof(struct ths7303_state),
> +			     GFP_KERNEL);
> +	if (!state)
>  		return -ENOMEM;
>  
> +	if (!pdata) {
> +		v4l_warn(client, "No platform data, using default data!\n");
> +		state->pdata.ch_1 = 0;
> +		state->pdata.ch_1 = 0;
> +		state->pdata.ch_1 = 0;

Copy-and-paste error? I guess this should be ch_1, ch_2 and ch_3 :-)
Also, since state is zeroed anyway, I'd just drop all assignments.

> +		state->pdata.init_enable = 0;
> +	} else {
> +		state->pdata = *pdata;
> +	}
> +
> +	sd = &state->sd;
>  	v4l2_i2c_subdev_init(sd, client, &ths7303_ops);
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	/* store the driver data to differntiate the chip */
> +	state->driver_data = (int)id->driver_data;
>  
> -	return ths7303_s_std_output(sd, std_id);
> +	return ths7303_setval(sd, THS7303_FILTER_MODE_480I_576I);
>  }
>  
>  static int ths7303_remove(struct i2c_client *client)
> @@ -195,6 +434,7 @@ static int ths7303_remove(struct i2c_client *client)
>  
>  static const struct i2c_device_id ths7303_id[] = {
>  	{"ths7303", 0},
> +	{"ths7353", 1},
>  	{},
>  };
>  
> @@ -203,7 +443,7 @@ MODULE_DEVICE_TABLE(i2c, ths7303_id);
>  static struct i2c_driver ths7303_driver = {
>  	.driver = {
>  		.owner	= THIS_MODULE,
> -		.name	= "ths7303",
> +		.name	= "ths73x3",
>  	},
>  	.probe		= ths7303_probe,
>  	.remove		= ths7303_remove,
> diff --git a/include/media/ths7303.h b/include/media/ths7303.h
> new file mode 100644
> index 0000000..818cfc3
> --- /dev/null
> +++ b/include/media/ths7303.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2013 Texas Instruments Inc
> + *
> + * Copyright 2013 Cisco Systems, Inc. and/or its affiliates.
> + *
> + * Contributors:
> + *     Lad, Prabhakar <prabhakar.lad@xxxxxx>
> + *     Hans Verkuil <hans.verkuil@xxxxxxxxx>
> + *     Martin Bugge <marbugge@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
> + */
> +
> +#ifndef THS7353_H
> +#define THS7353_H
> +
> +struct ths7303_platform_data {
> +	u8 ch_1;
> +	u8 ch_2;
> +	u8 ch_3;
> +	u8 init_enable;
> +};

This struct needs to be documented.

> +
> +#endif
> diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> index 4ee125b..84d1159 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -177,6 +177,9 @@ enum {
>  	/* module ths7303: just ident 7303 */
>  	V4L2_IDENT_THS7303 = 7303,
>  
> +	/* module ths7353: just ident 7353 */
> +	V4L2_IDENT_THS7353 = 7353,
> +

This should be moved after ADV7343: this list needs to be sorted numerically.

>  	/* module adv7343: just ident 7343 */
>  	V4L2_IDENT_ADV7343 = 7343,
>  
> 

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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