Re: [PATCH 3/4 v6] TVP7002 driver for DM365
Hi Santiago, See review comments below: On Friday 06 November 2009 16:42:56 santiago.nu...@ridgerun.com wrote: > From: Santiago Nunez-Corrales > > This patch provides the implementation of the TVP7002 decoder > driver for DM365. Implemented using the V4L2 DV presets API. > > Signed-off-by: Santiago Nunez-Corrales > --- > drivers/media/video/tvp7002.c | 1423 > + > 1 files changed, 1423 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/tvp7002.c > > diff --git a/drivers/media/video/tvp7002.c b/drivers/media/video/tvp7002.c > new file mode 100644 > index 000..7d945d9 > --- /dev/null > +++ b/drivers/media/video/tvp7002.c > @@ -0,0 +1,1423 @@ > +/* Texas Instruments Triple 8-/10-BIT 165-/110-MSPS Video and Graphics > + * Digitizer with Horizontal PLL registers > + * > + * Copyright (C) 2009 Texas Instruments Inc > + * Author: Santiago Nunez-Corrales > + * > + * This code is partially based upon the TVP5150 driver > + * written by Mauro Carvalho Chehab (mche...@infradead.org), > + * the TVP514x driver written by Vaibhav Hiremath > + * and the TVP7002 driver in the TI LSP 2.10.00.14. Revisions by > + * Muralidharan Karicheri and Snehaprabha Narnakaje (TI). > + * > + * 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; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include "tvp7002_reg.h" > + > +MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver"); > +MODULE_AUTHOR("Santiago Nunez-Corrales "); > +MODULE_LICENSE("GPL"); > + > +/* Module Name */ > +#define TVP7002_MODULE_NAME "tvp7002" > + > +/* I2C retry attempts */ > +#define I2C_RETRY_COUNT (5) > + > +/* End of registers */ > +#define TVP7002_EOR 0x5c > + > +/* Debug functions */ > +static int debug; > +module_param(debug, bool, 0644); > +MODULE_PARM_DESC(debug, "Debug level (0-2)"); > + > +/* Structure for register values */ > +struct i2c_reg_value { > + u8 reg; > + u8 value; > + u8 type; > +}; > + > +/* > + * Register default values (according to tvp7002 datasheet) > + * In the case of read-only registers, the value (0xff) is > + * never written. R/W functionality is controlled by the > + * writable bit in the register struct definition. > + */ > +static const struct i2c_reg_value tvp7002_init_default[] = { > + { TVP7002_CHIP_REV, 0xff, TVP7002_READ }, > + { TVP7002_HPLL_FDBK_DIV_MSBS, 0x67, TVP7002_WRITE }, > + { TVP7002_HPLL_FDBK_DIV_LSBS, 0x20, TVP7002_WRITE }, > + { TVP7002_HPLL_CRTL, 0xa0, TVP7002_WRITE }, > + { TVP7002_HPLL_PHASE_SEL, 0x80, TVP7002_WRITE }, > + { TVP7002_CLAMP_START, 0x32, TVP7002_WRITE }, > + { TVP7002_CLAMP_W, 0x20, TVP7002_WRITE }, > + { TVP7002_HSYNC_OUT_W, 0x60, TVP7002_WRITE }, > + { TVP7002_B_FINE_GAIN, 0x00, TVP7002_WRITE }, > + { TVP7002_G_FINE_GAIN, 0x00, TVP7002_WRITE }, > + { TVP7002_R_FINE_GAIN, 0x00, TVP7002_WRITE }, > + { TVP7002_B_FINE_OFF_MSBS, 0x80, TVP7002_WRITE }, > + { TVP7002_G_FINE_OFF_MSBS, 0x80, TVP7002_WRITE }, > + { TVP7002_R_FINE_OFF_MSBS, 0x80, TVP7002_WRITE }, > + { TVP7002_SYNC_CTL_1, 0x20, TVP7002_WRITE }, > + { TVP7002_HPLL_AND_CLAMP_CTL, 0x2e, TVP7002_WRITE }, > + { TVP7002_SYNC_ON_G_THRS, 0x5d, TVP7002_WRITE }, > + { TVP7002_SYNC_SEPARATOR_THRS, 0x47, TVP7002_WRITE }, > + { TVP7002_HPLL_PRE_COAST, 0x00, TVP7002_WRITE }, > + { TVP7002_HPLL_POST_COAST, 0x00, TVP7002_WRITE }, > + { TVP7002_SYNC_DETECT_STAT, 0xff, TVP7002_READ }, > + { TVP7002_OUT_FORMATTER, 0x47, TVP7002_WRITE }, > + { TVP7002_MISC_CTL_1, 0x01, TVP7002_WRITE }, > + { TVP7002_MISC_CTL_2, 0x00, TVP7002_WRITE }, > + { TVP7002_MISC_CTL_3, 0x01, TVP7002_WRITE }, > + { TVP7002_IN_MUX_SEL_1, 0x00, TVP7002_WRITE }, > + { TVP7002_IN_MUX_SEL_2, 0x67, TVP7002_WRITE }, > + { TVP7002_B_AND_G_COARSE_GAIN, 0x77, TVP7002_WRITE }, > + { TVP7002_R_COARSE_GAIN, 0x07, TVP7002_WRITE }, > + { TVP7002_FINE_OFF_LSBS, 0x00, TVP7002_WRITE }, > + { TVP7002_B_COARSE_OFF, 0x10, TVP7002_WRITE }, > + { TVP7002_G_COARSE_OFF, 0x10, TVP7002_WRITE }, > + { TVP7002_R_COARSE_OFF, 0x10, TVP7002_WRITE }, > + { TVP7002_HSOUT_OUT_START, 0x08, TVP7002_WRITE }, > + { TVP7002_MISC_CTL_4, 0
Re: [PATCH 3/4 v6] TVP7002 driver for DM365
Hans, Thanks for all your patient and detailed reviews. I've addressed most of the comments. There is one thing though that seems odd but is there due to a good reason. The devices requires to be turned off and on again during the s_stream function, in which it 'forgets' its previous state and therefore register values have to be kept in memory and set back for initialization purposes. Regards, Hans Verkuil wrote: Hi Santiago, See review comments below: On Friday 06 November 2009 16:42:56 santiago.nu...@ridgerun.com wrote: From: Santiago Nunez-Corrales This patch provides the implementation of the TVP7002 decoder driver for DM365. Implemented using the V4L2 DV presets API. Signed-off-by: Santiago Nunez-Corrales --- drivers/media/video/tvp7002.c | 1423 + 1 files changed, 1423 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/tvp7002.c diff --git a/drivers/media/video/tvp7002.c b/drivers/media/video/tvp7002.c new file mode 100644 index 000..7d945d9 --- /dev/null +++ b/drivers/media/video/tvp7002.c @@ -0,0 +1,1423 @@ +/* Texas Instruments Triple 8-/10-BIT 165-/110-MSPS Video and Graphics + * Digitizer with Horizontal PLL registers + * + * Copyright (C) 2009 Texas Instruments Inc + * Author: Santiago Nunez-Corrales + * + * This code is partially based upon the TVP5150 driver + * written by Mauro Carvalho Chehab (mche...@infradead.org), + * the TVP514x driver written by Vaibhav Hiremath + * and the TVP7002 driver in the TI LSP 2.10.00.14. Revisions by + * Muralidharan Karicheri and Snehaprabha Narnakaje (TI). + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ +#include +#include +#include +#include +#include +#include +#include "tvp7002_reg.h" + +MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver"); +MODULE_AUTHOR("Santiago Nunez-Corrales "); +MODULE_LICENSE("GPL"); + +/* Module Name */ +#define TVP7002_MODULE_NAME"tvp7002" + +/* I2C retry attempts */ +#define I2C_RETRY_COUNT(5) + +/* End of registers */ +#define TVP7002_EOR0x5c + +/* Debug functions */ +static int debug; +module_param(debug, bool, 0644); +MODULE_PARM_DESC(debug, "Debug level (0-2)"); + +/* Structure for register values */ +struct i2c_reg_value { + u8 reg; + u8 value; + u8 type; +}; + +/* + * Register default values (according to tvp7002 datasheet) + * In the case of read-only registers, the value (0xff) is + * never written. R/W functionality is controlled by the + * writable bit in the register struct definition. + */ +static const struct i2c_reg_value tvp7002_init_default[] = { + { TVP7002_CHIP_REV, 0xff, TVP7002_READ }, + { TVP7002_HPLL_FDBK_DIV_MSBS, 0x67, TVP7002_WRITE }, + { TVP7002_HPLL_FDBK_DIV_LSBS, 0x20, TVP7002_WRITE }, + { TVP7002_HPLL_CRTL, 0xa0, TVP7002_WRITE }, + { TVP7002_HPLL_PHASE_SEL, 0x80, TVP7002_WRITE }, + { TVP7002_CLAMP_START, 0x32, TVP7002_WRITE }, + { TVP7002_CLAMP_W, 0x20, TVP7002_WRITE }, + { TVP7002_HSYNC_OUT_W, 0x60, TVP7002_WRITE }, + { TVP7002_B_FINE_GAIN, 0x00, TVP7002_WRITE }, + { TVP7002_G_FINE_GAIN, 0x00, TVP7002_WRITE }, + { TVP7002_R_FINE_GAIN, 0x00, TVP7002_WRITE }, + { TVP7002_B_FINE_OFF_MSBS, 0x80, TVP7002_WRITE }, + { TVP7002_G_FINE_OFF_MSBS, 0x80, TVP7002_WRITE }, + { TVP7002_R_FINE_OFF_MSBS, 0x80, TVP7002_WRITE }, + { TVP7002_SYNC_CTL_1, 0x20, TVP7002_WRITE }, + { TVP7002_HPLL_AND_CLAMP_CTL, 0x2e, TVP7002_WRITE }, + { TVP7002_SYNC_ON_G_THRS, 0x5d, TVP7002_WRITE }, + { TVP7002_SYNC_SEPARATOR_THRS, 0x47, TVP7002_WRITE }, + { TVP7002_HPLL_PRE_COAST, 0x00, TVP7002_WRITE }, + { TVP7002_HPLL_POST_COAST, 0x00, TVP7002_WRITE }, + { TVP7002_SYNC_DETECT_STAT, 0xff, TVP7002_READ }, + { TVP7002_OUT_FORMATTER, 0x47, TVP7002_WRITE }, + { TVP7002_MISC_CTL_1, 0x01, TVP7002_WRITE }, + { TVP7002_MISC_CTL_2, 0x00, TVP7002_WRITE }, + { TVP7002_MISC_CTL_3, 0x01, TVP7002_WRITE }, + { TVP7002_IN_MUX_SEL_1, 0x00, TVP7002_WRITE }, + { TVP7002_IN_MUX_SEL_2, 0x67, TVP7002_WRITE }, + { TVP7002_B_AND_G_COARSE_GAIN, 0x77, TVP7002_WRITE }, + { TVP7002_R_COARSE_GAIN, 0x07, TVP7002_WRITE }, + { TVP7002_FINE_OFF_LSB
Re: [PATCH 3/4 v6] TVP7002 driver for DM365
On Tuesday 10 November 2009 19:12:13 Santiago Nunez-Corrales wrote: > Hans, > > > Thanks for all your patient and detailed reviews. I've addressed most of > the comments. There is one thing though that seems odd but is there due > to a good reason. The devices requires to be turned off and on again > during the s_stream function, in which it 'forgets' its previous state > and therefore register values have to be kept in memory and set back for > initialization purposes. Hmm. It's still pretty ugly. Wouldn't it be more elegant to record this state at a higher level? E.g. instead of restoring gain registers you would store the current gain value in the state struct and just set the gain again. Ditto for things like the output standard, etc. Another question is: why does it need to be reset? That's rather peculiar. Are there powersaving considerations? Is this normal behavior for this device? I can't imagine that this is how the tvp7002 designers expected it to be used, so I get the feeling that what you are doing is more a kludge to hammer the driver into the v4l2_subdev API. Can you look at this some more? I have downloaded the tvp7002 datasheet, so you can also point me to the relevant sections/register descriptions of the datasheet. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html