From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.navvo.net ([74.208.67.6]:50272 "EHLO mail.navvo.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755742AbZJ0Px7 (ORCPT ); Tue, 27 Oct 2009 11:53:59 -0400 Message-ID: <4AE71793.3090502@ridgerun.com> Date: Tue, 27 Oct 2009 09:53:55 -0600 From: Santiago Nunez-Corrales Reply-To: santiago.nunez@ridgerun.com MIME-Version: 1.0 To: "Narnakaje, Snehaprabha" CC: Kevin Hilman , "davinci-linux-open-source@linux.davincidsp.com" , "todd.fischer@ridgerun.com" , "linux-media@vger.kernel.org" References: <1255617794-1401-1-git-send-email-santiago.nunez@ridgerun.com> <87skdk7aul.fsf@deeprootsystems.com> <4AE1E903.4030605@ridgerun.com> <87y6mxalep.fsf@deeprootsystems.com> <7A436F7769CA33409C6B44B358BFFF0C012ACE7054@dlee02.ent.ti.com> In-Reply-To: <7A436F7769CA33409C6B44B358BFFF0C012ACE7054@dlee02.ent.ti.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [PATCH 2/6 v5] Support for TVP7002 in dm365 board Sender: linux-media-owner@vger.kernel.org List-ID: Sneha, So, if I got it right, have_tvp7002 is unnecessary since that initialization is done via the CPLD interface. Therefore, all I need to do is actually remove the logic for the device from board-dm365-evm.h. Am I right? If so, should I also remove the logic for tvp5146? Regards, Narnakaje, Snehaprabha wrote: > Santiago, Kevin, > > >> -----Original Message----- >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] >> Sent: Monday, October 26, 2009 5:35 PM >> To: santiago.nunez@ridgerun.com >> Cc: Narnakaje, Snehaprabha; davinci-linux-open- >> source@linux.davincidsp.com; todd.fischer@ridgerun.com; linux- >> media@vger.kernel.org >> Subject: Re: [PATCH 2/6 v5] Support for TVP7002 in dm365 board >> >> Santiago Nunez-Corrales writes: >> >> >>> Kevin Hilman wrote: >>> >>>> writes: >>>> >>>> >>>> >>>>> From: Santiago Nunez-Corrales >>>>> >>>>> This patch provides support for TVP7002 in architecture definitions >>>>> within DM365. >>>>> >>>>> Signed-off-by: Santiago Nunez-Corrales >>>>> --- >>>>> arch/arm/mach-davinci/board-dm365-evm.c | 170 >>>>> >> ++++++++++++++++++++++++++++++- >> >>>>> 1 files changed, 166 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach- >>>>> >> davinci/board-dm365-evm.c >> >>>>> index a1d5e7d..6c544d3 100644 >>>>> --- a/arch/arm/mach-davinci/board-dm365-evm.c >>>>> +++ b/arch/arm/mach-davinci/board-dm365-evm.c >>>>> @@ -38,6 +38,11 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> static inline int have_imager(void) >>>>> @@ -48,8 +53,11 @@ 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 >>>>> >>>>> >>>> I've said this before, but I'll say it again. I don't like the >>>> #ifdef-on-Kconfig-option here. >>>> >>>> Can you add a probe hook to the platform_data so that when the tvp7002 >>>> is found it can call pdata->probe() which could then set a flag >>>> for use by have_tvp7002(). >>>> >>>> This will have he same effect without the ifdef since if the driver >>>> is not compiled in, its probe can never be triggered. >>>> >>>> Kevin >>>> >>>> >>>> >>> Kevin, >>> >>> I've been working on this particular implementation. This >>> board-dm365-evm.c is specific to the board, therefore I don't still >>> get the point of not having those values wired to the board file, but >>> I know it'd be nice to have the CPLD configuration triggered upon >>> TVP7002 detection. I see two options: >>> >> Having them in the board file is appropriate, what I object to is the >> selection by Kconfig. Run-time detection is always preferred when >> possible. >> > > We have this CPLD init API - evm_init_cpld() called from the dm365_evm_init() function. The CPLD init API was trying to initialize the CPLD, based on the default configuration. I believe David Brownell had this placeholder for have_imager() and have_tvp7002() APIs since, we have different CPLD settings for the imager, tvp7002 and tvp5146 for the video input source. > > >>> 1. Do the callback function inside pdata and initialize it at driver >>> load time (tvp7002_probe). Set tvp5146 as default and override when >>> driver loads (and restore when unloads). >>> >> This is the preferred option to me. >> > > We can decide on the default video input source to be TVP5146. However we do not need a new callback function. We already have the VPFE .setup_input callback API dm365evm_setup_video_input() for the same purpose. The VPFE .setup_input API is called when each of the decoders (sub-devices) registered with VPFE capture driver. It is also called when the application decides on an input source and switches between the input sources. > > So, TVP5146 remains as the default video input source, only until VPFE probe is called, which registers the all decoders (sub-devices) defined in the VPFE platform data. This also means that the last decoder in the VPFE platform data remains active after boot-up. One can change the order in the VPFE platform data if a particular input source needs to remain active after boot-up. Note that application can always switch the input-source at run time. > > I quickly tried removing the have_imager() construct in the evm_init_cpld() and here is the output - > > Starting kernel ... > > Uncompressing Linux............................................................................................... > ........................................ done, booting the kernel. > Linux version 2.6.32-rc2-davinci1-dirty > ... > CPU: Testing write buffer coherency: ok > DaVinci: 8 gpio irqs > NET: Registered protocol family 16 > EVM: tvp5146 SD video input > bio: create slab at 0 > SCSI subsystem initialized > ... > vpfe_init > vpfe-capture: vpss clock vpss_master enabled > vpfe-capture vpfe-capture: v4l2 device registered > vpfe-capture vpfe-capture: video device registered > EVM: switch to tvp5146 SD video input > tvp514x 1-005d: tvp514x 1-005d decoder driver registered !! > vpfe-capture vpfe-capture: v4l2 sub device tvp5146 registered > EVM: switch to tvp7002 HD video input > tvp7002 1-005c: tvp7002 1-005c decoder driver registered !! > vpfe-capture vpfe-capture: v4l2 sub device tvp7002 registered > ths7353 1-002e: chip found @ 0x5c (DaVinci I2C adapter) > vpfe-capture vpfe-capture: v4l2 sub device ths7353 registered > ... > > As you can see, the default video input source is TVP5146 and it has been video input source has been switched to TVP7002. > > Thanks > Sneha > > >>> 2. Add an entry to sysfs such that it can be user-configurable whether >>> to activate one of the other regardless of whether tvp5156 or tvp7002 >>> are actually there (the only result would be fail to access the >>> device). >>> >> Why do you need sysfs options for switching? Wouldn't building as >> modules and loading/unloading the needed modules serve the same >> purpose? >> >> Remeber that the 'probe' isn't going to be called until the >> platform_driver >> is registered, and that will (usually) happen at module load time. >> >> >>> Sneha, do you have any suggestions on this one? >>> >> Kevin >> >> > > -- Santiago Nunez-Corrales, Eng. RidgeRun Engineering, LLC Guayabos, Curridabat San Jose, Costa Rica +(506) 2271 1487 +(506) 8313 0536 http://www.ridgerun.com