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, ®->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, ®->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
