| Message ID | 1252018323-17344-1-git-send-email-santiago.nunez@ridgerun.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Sep 04, 2009 at 04:22:03, santiago.nunez@ridgerun.com wrote: > From: Santiago Nunez-Corrales <santiago.nunez@ridgerun.com> > > This patch provides support for TVP7002 in architecture definitions > within DM365. Moved tvp7002 platform data here and cleaned up code. > > Signed-off-by: Santiago Nunez-Corrales <santiago.nunez@ridgerun.com> > --- > arch/arm/mach-davinci/board-dm365-evm.c | 65 +++++++++++++++++++++++++++++-- > 1 files changed, 61 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c > index 362ac62..4398d92 100644 > --- a/arch/arm/mach-davinci/board-dm365-evm.c > +++ b/arch/arm/mach-davinci/board-dm365-evm.c > @@ -42,7 +42,9 @@ > #include <mach/mmc.h> > #include <mach/nand.h> > #include <linux/videodev2.h> > +#include <media/davinci/videohd.h> > #include <media/tvp514x.h> > +#include <media/tvp7002.h> > > > static inline int have_imager(void) > @@ -53,14 +55,19 @@ static inline int have_imager(void) > > static inline int have_tvp7002(void) > { > - /* REVISIT when it's supported, trigger via Kconfig */ > +#ifdef CONFIG_VIDEO_TVP7002 > + return 1; > +#else > return 0; > +#endif > } May be this can simply be: #ifdef CONFIG_VIDEO_TVP7002 #define HAS_TVP7002 1 #else #define HAS_TVP7002 0 #endif However, you don't seem to use this in your patch set anyway. > > - > #define DM365_ASYNC_EMIF_CONTROL_BASE 0x01d10000 > #define DM365_ASYNC_EMIF_DATA_CE0_BASE 0x02000000 > #define DM365_ASYNC_EMIF_DATA_CE1_BASE 0x04000000 > +#define DM365_ASYNC_EMIF_DATA_CE1_REG3 0x18 > +#define DM365_ASYNC_EMIF_VIDEO_MUX_MASK (0x07070707) > +#define DM365_ASYNC_EMIF_TVP7002_SEL (0x01010101) Brackets unneeded. > > #define DM365_EVM_PHY_MASK (0x2) > #define DM365_EVM_MDIO_FREQUENCY (2200000) /* PHY bus frequency */ > @@ -109,6 +116,15 @@ static struct tvp514x_platform_data tvp5146_pdata = { > .vs_polarity = 1 > }; > > +/* tvp7002 platform data, used during reset and probe operations */ > +static struct tvp7002_config tvp7002_pdata = { > + .clk_polarity = 0, > + .hs_polarity = 0, > + .vs_polarity = 0, > + .fid_polarity = 0, > + .sog_polarity = 0, > +}; No need to initialize to 0s. > + > /* NOTE: this is geared for the standard config, with a socketed > * 2 GByte Micron NAND (MT29F16G08FAA) using 128KB sectors. If you > * swap chips, maybe with a different block size, partitioning may > @@ -243,6 +259,22 @@ static struct v4l2_input tvp5146_inputs[] = { > }, > }; > > +#define TVP7002_STD_ALL (V4L2_STD_525P_60 | V4L2_STD_625P_50 |\ > + V4L2_STD_NTSC | V4L2_STD_PAL |\ > + V4L2_STD_720P_50 | V4L2_STD_720P_60 |\ > + V4L2_STD_1080I_50 | V4L2_STD_1080I_60 |\ > + V4L2_STD_1080P_50 | V4L2_STD_1080P_60) > + > +/* Inputs available at the TVP7002 */ > +static struct v4l2_input tvp7002_inputs[] = { > + { > + .index = 0, > + .name = "Component", > + .type = V4L2_INPUT_TYPE_CAMERA, > + .std = TVP7002_STD_ALL, > + }, > +}; > + > /* > * this is the route info for connecting each input to decoder > * ouput that goes to vpfe. There is a one to one correspondence > @@ -276,6 +308,19 @@ static struct vpfe_subdev_info vpfe_sub_devs[] = { > I2C_BOARD_INFO("tvp5146", 0x5d), > .platform_data = &tvp5146_pdata, > }, > + }, > + { > + .module_name = "tvp7002", > + .grp_id = 0, > + .ccdc_if_params = { > + .if_type = VPFE_BT1120, > + .hdpol = VPFE_PINPOL_POSITIVE, > + .vdpol = VPFE_PINPOL_POSITIVE, > + }, > + .board_info = { > + I2C_BOARD_INFO("tvp7002", 0x5c), > + .platform_data = &tvp7002_pdata, > + }, > } > }; > > @@ -439,6 +484,16 @@ static int __init cpld_leds_init(void) > /* run after subsys_initcall() for LEDs */ > fs_initcall(cpld_leds_init); > > +/* Set the input mux for TVP7002 */ > +int tvp7002_set_input_mux(void) This is not used in any of your later patches. You wanted this to be static? Thanks, Sekhar
Sekhar, Thanks for your review. Comments inlined. Regards, Nori, Sekhar wrote: > On Fri, Sep 04, 2009 at 04:22:03, santiago.nunez@ridgerun.com wrote: > >> From: Santiago Nunez-Corrales <santiago.nunez@ridgerun.com> >> >> This patch provides support for TVP7002 in architecture definitions >> within DM365. Moved tvp7002 platform data here and cleaned up code. >> >> Signed-off-by: Santiago Nunez-Corrales <santiago.nunez@ridgerun.com> >> --- >> arch/arm/mach-davinci/board-dm365-evm.c | 65 +++++++++++++++++++++++++++++-- >> 1 files changed, 61 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c >> index 362ac62..4398d92 100644 >> --- a/arch/arm/mach-davinci/board-dm365-evm.c >> +++ b/arch/arm/mach-davinci/board-dm365-evm.c >> @@ -42,7 +42,9 @@ >> #include <mach/mmc.h> >> #include <mach/nand.h> >> #include <linux/videodev2.h> >> +#include <media/davinci/videohd.h> >> #include <media/tvp514x.h> >> +#include <media/tvp7002.h> >> >> >> static inline int have_imager(void) >> @@ -53,14 +55,19 @@ static inline int have_imager(void) >> >> static inline int have_tvp7002(void) >> { >> - /* REVISIT when it's supported, trigger via Kconfig */ >> +#ifdef CONFIG_VIDEO_TVP7002 >> + return 1; >> +#else >> return 0; >> +#endif >> } >> > > May be this can simply be: > > #ifdef CONFIG_VIDEO_TVP7002 > #define HAS_TVP7002 1 > #else > #define HAS_TVP7002 0 > #endif > > However, you don't seem to use this in your > patch set anyway. > > [SN] It is used in this same file. The reason for coding this is to follow the standard in the implementation. >> - >> #define DM365_ASYNC_EMIF_CONTROL_BASE 0x01d10000 >> #define DM365_ASYNC_EMIF_DATA_CE0_BASE 0x02000000 >> #define DM365_ASYNC_EMIF_DATA_CE1_BASE 0x04000000 >> +#define DM365_ASYNC_EMIF_DATA_CE1_REG3 0x18 >> +#define DM365_ASYNC_EMIF_VIDEO_MUX_MASK (0x07070707) >> +#define DM365_ASYNC_EMIF_TVP7002_SEL (0x01010101) >> > > Brackets unneeded. > [SN] Ok. > >> #define DM365_EVM_PHY_MASK (0x2) >> #define DM365_EVM_MDIO_FREQUENCY (2200000) /* PHY bus frequency */ >> @@ -109,6 +116,15 @@ static struct tvp514x_platform_data tvp5146_pdata = { >> .vs_polarity = 1 >> }; >> >> +/* tvp7002 platform data, used during reset and probe operations */ >> +static struct tvp7002_config tvp7002_pdata = { >> + .clk_polarity = 0, >> + .hs_polarity = 0, >> + .vs_polarity = 0, >> + .fid_polarity = 0, >> + .sog_polarity = 0, >> +}; >> > > No need to initialize to 0s. > > >> + >> /* NOTE: this is geared for the standard config, with a socketed >> * 2 GByte Micron NAND (MT29F16G08FAA) using 128KB sectors. If you >> * swap chips, maybe with a different block size, partitioning may >> @@ -243,6 +259,22 @@ static struct v4l2_input tvp5146_inputs[] = { >> }, >> }; >> >> +#define TVP7002_STD_ALL (V4L2_STD_525P_60 | V4L2_STD_625P_50 |\ >> + V4L2_STD_NTSC | V4L2_STD_PAL |\ >> + V4L2_STD_720P_50 | V4L2_STD_720P_60 |\ >> + V4L2_STD_1080I_50 | V4L2_STD_1080I_60 |\ >> + V4L2_STD_1080P_50 | V4L2_STD_1080P_60) >> + >> +/* Inputs available at the TVP7002 */ >> +static struct v4l2_input tvp7002_inputs[] = { >> + { >> + .index = 0, >> + .name = "Component", >> + .type = V4L2_INPUT_TYPE_CAMERA, >> + .std = TVP7002_STD_ALL, >> + }, >> +}; >> + >> /* >> * this is the route info for connecting each input to decoder >> * ouput that goes to vpfe. There is a one to one correspondence >> @@ -276,6 +308,19 @@ static struct vpfe_subdev_info vpfe_sub_devs[] = { >> I2C_BOARD_INFO("tvp5146", 0x5d), >> .platform_data = &tvp5146_pdata, >> }, >> + }, >> + { >> + .module_name = "tvp7002", >> + .grp_id = 0, >> + .ccdc_if_params = { >> + .if_type = VPFE_BT1120, >> + .hdpol = VPFE_PINPOL_POSITIVE, >> + .vdpol = VPFE_PINPOL_POSITIVE, >> + }, >> + .board_info = { >> + I2C_BOARD_INFO("tvp7002", 0x5c), >> + .platform_data = &tvp7002_pdata, >> + }, >> } >> }; >> >> @@ -439,6 +484,16 @@ static int __init cpld_leds_init(void) >> /* run after subsys_initcall() for LEDs */ >> fs_initcall(cpld_leds_init); >> >> +/* Set the input mux for TVP7002 */ >> +int tvp7002_set_input_mux(void) >> > > This is not used in any of your later patches. > You wanted this to be static? > [SN] True, this should be static. > Thanks, > Sekhar >
On Sat, Sep 05, 2009 at 01:16:43, Santiago Nunez-Corrales wrote: > Sekhar, > > > Thanks for your review. Comments inlined. > > Regards, > > Nori, Sekhar wrote: > > On Fri, Sep 04, 2009 at 04:22:03, santiago.nunez@ridgerun.com wrote: > > > >> From: Santiago Nunez-Corrales <santiago.nunez@ridgerun.com> > >> > >> This patch provides support for TVP7002 in architecture definitions > >> within DM365. Moved tvp7002 platform data here and cleaned up code. [...] > >> diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c [...] > >> > >> > >> static inline int have_imager(void) > >> @@ -53,14 +55,19 @@ static inline int have_imager(void) > >> > >> static inline int have_tvp7002(void) > >> { > >> - /* REVISIT when it's supported, trigger via Kconfig */ > >> +#ifdef CONFIG_VIDEO_TVP7002 > >> + return 1; > >> +#else > >> return 0; > >> +#endif > >> } > >> > > > > May be this can simply be: > > > > #ifdef CONFIG_VIDEO_TVP7002 > > #define HAS_TVP7002 1 > > #else > > #define HAS_TVP7002 0 > > #endif > > > > However, you don't seem to use this in your > > patch set anyway. > > > > > [SN] It is used in this same file. The reason for coding this is to > follow the standard in the implementation. If using a function is must, then the implementation can be: #ifdef CONFIG_VIDEO_TVP7002 static inline int have_tvp7002(void) { return 1; } #else static inline int have_tvp7002(void) { return 0; } #endif Thanks, Sekhar
"Nori, Sekhar" <nsekhar@ti.com> writes: > On Sat, Sep 05, 2009 at 01:16:43, Santiago Nunez-Corrales wrote: >> Sekhar, >> >> >> Thanks for your review. Comments inlined. >> >> Regards, >> >> Nori, Sekhar wrote: >> > On Fri, Sep 04, 2009 at 04:22:03, santiago.nunez@ridgerun.com wrote: >> > >> >> From: Santiago Nunez-Corrales <santiago.nunez@ridgerun.com> >> >> >> >> This patch provides support for TVP7002 in architecture definitions >> >> within DM365. Moved tvp7002 platform data here and cleaned up code. > > [...] > >> >> diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c > > [...] > >> >> >> >> >> >> static inline int have_imager(void) >> >> @@ -53,14 +55,19 @@ static inline int have_imager(void) >> >> >> >> static inline int have_tvp7002(void) >> >> { >> >> - /* REVISIT when it's supported, trigger via Kconfig */ >> >> +#ifdef CONFIG_VIDEO_TVP7002 >> >> + return 1; >> >> +#else >> >> return 0; >> >> +#endif >> >> } >> >> >> > >> > May be this can simply be: >> > >> > #ifdef CONFIG_VIDEO_TVP7002 >> > #define HAS_TVP7002 1 >> > #else >> > #define HAS_TVP7002 0 >> > #endif >> > >> > However, you don't seem to use this in your >> > patch set anyway. >> > >> > >> [SN] It is used in this same file. The reason for coding this is to >> follow the standard in the implementation. > > If using a function is must, then the implementation > can be: > > #ifdef CONFIG_VIDEO_TVP7002 > static inline int have_tvp7002(void) > { > return 1; > } > #else > static inline int have_tvp7002(void) > { > return 0; > } > #endif > Not sure that's any better. What's the point of having a function that essentially returns the value of a Kconfig varible? What's being done is basically this: static inline int have_tvp7002(void) { return CONFIG_VIDEO_TVP7002; } and I don't see the need for that. Kevin
diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c index 362ac62..4398d92 100644 --- a/arch/arm/mach-davinci/board-dm365-evm.c +++ b/arch/arm/mach-davinci/board-dm365-evm.c @@ -42,7 +42,9 @@ #include <mach/mmc.h> #include <mach/nand.h> #include <linux/videodev2.h> +#include <media/davinci/videohd.h> #include <media/tvp514x.h> +#include <media/tvp7002.h> static inline int have_imager(void) @@ -53,14 +55,19 @@ static inline int have_imager(void) static inline int have_tvp7002(void) { - /* REVISIT when it's supported, trigger via Kconfig */ +#ifdef CONFIG_VIDEO_TVP7002 + return 1; +#else return 0; +#endif } - #define DM365_ASYNC_EMIF_CONTROL_BASE 0x01d10000 #define DM365_ASYNC_EMIF_DATA_CE0_BASE 0x02000000 #define DM365_ASYNC_EMIF_DATA_CE1_BASE 0x04000000 +#define DM365_ASYNC_EMIF_DATA_CE1_REG3 0x18 +#define DM365_ASYNC_EMIF_VIDEO_MUX_MASK (0x07070707) +#define DM365_ASYNC_EMIF_TVP7002_SEL (0x01010101) #define DM365_EVM_PHY_MASK (0x2) #define DM365_EVM_MDIO_FREQUENCY (2200000) /* PHY bus frequency */ @@ -109,6 +116,15 @@ static struct tvp514x_platform_data tvp5146_pdata = { .vs_polarity = 1 }; +/* tvp7002 platform data, used during reset and probe operations */ +static struct tvp7002_config tvp7002_pdata = { + .clk_polarity = 0, + .hs_polarity = 0, + .vs_polarity = 0, + .fid_polarity = 0, + .sog_polarity = 0, +}; + /* NOTE: this is geared for the standard config, with a socketed * 2 GByte Micron NAND (MT29F16G08FAA) using 128KB sectors. If you * swap chips, maybe with a different block size, partitioning may @@ -243,6 +259,22 @@ static struct v4l2_input tvp5146_inputs[] = { }, }; +#define TVP7002_STD_ALL (V4L2_STD_525P_60 | V4L2_STD_625P_50 |\ + V4L2_STD_NTSC | V4L2_STD_PAL |\ + V4L2_STD_720P_50 | V4L2_STD_720P_60 |\ + V4L2_STD_1080I_50 | V4L2_STD_1080I_60 |\ + V4L2_STD_1080P_50 | V4L2_STD_1080P_60) + +/* Inputs available at the TVP7002 */ +static struct v4l2_input tvp7002_inputs[] = { + { + .index = 0, + .name = "Component", + .type = V4L2_INPUT_TYPE_CAMERA, + .std = TVP7002_STD_ALL, + }, +}; + /* * this is the route info for connecting each input to decoder * ouput that goes to vpfe. There is a one to one correspondence @@ -276,6 +308,19 @@ static struct vpfe_subdev_info vpfe_sub_devs[] = { I2C_BOARD_INFO("tvp5146", 0x5d), .platform_data = &tvp5146_pdata, }, + }, + { + .module_name = "tvp7002", + .grp_id = 0, + .ccdc_if_params = { + .if_type = VPFE_BT1120, + .hdpol = VPFE_PINPOL_POSITIVE, + .vdpol = VPFE_PINPOL_POSITIVE, + }, + .board_info = { + I2C_BOARD_INFO("tvp7002", 0x5c), + .platform_data = &tvp7002_pdata, + }, } }; @@ -439,6 +484,16 @@ static int __init cpld_leds_init(void) /* run after subsys_initcall() for LEDs */ fs_initcall(cpld_leds_init); +/* Set the input mux for TVP7002 */ +int tvp7002_set_input_mux(void) +{ + u32 val; + val = __raw_readl(cpld + DM365_ASYNC_EMIF_DATA_CE1_REG3); + val &= ~DM365_ASYNC_EMIF_VIDEO_MUX_MASK; + val |= DM365_ASYNC_EMIF_TVP7002_SEL; + __raw_writel(val, cpld + DM365_ASYNC_EMIF_DATA_CE1_REG3); + return 0; +} static void __init evm_init_cpld(void) { @@ -519,6 +574,8 @@ fail: mux |= 2; resets &= ~BIT(2); label = "tvp7002 HD"; + /* Call the input setter */ + tvp7002_set_input_mux(); } else { /* default to tvp5146 */ mux |= 5; @@ -526,8 +583,8 @@ fail: label = "tvp5146 SD"; } } - __raw_writeb(mux, cpld + CPLD_MUX); - __raw_writeb(resets, cpld + CPLD_RESETS); + __raw_writel(mux, cpld + CPLD_MUX); + __raw_writel(resets, cpld + CPLD_RESETS); pr_info("EVM: %s video input\n", label); /* REVISIT export switches: NTSC/PAL (SW5.6), EXTRA1 (SW5.2), etc */