RE: [PATCH - v0 1/2] V4L - vpfe capture - make clocks configurable

2009-11-30 Thread Karicheri, Muralidharan
Vaibhav,

Thanks for testing this. I will update the patch for your comments.

Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-kariche...@ti.com

>-Original Message-
>From: Hiremath, Vaibhav
>Sent: Tuesday, November 24, 2009 11:25 PM
>To: Karicheri, Muralidharan; linux-media@vger.kernel.org;
>hverk...@xs4all.nl; khil...@deeprootsystems.com
>Cc: davinci-linux-open-sou...@linux.davincidsp.com
>Subject: RE: [PATCH - v0 1/2] V4L - vpfe capture - make clocks configurable
>
>
>> -Original Message-
>> From: davinci-linux-open-source-boun...@linux.davincidsp.com
>> [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com] On
>> Behalf Of Karicheri, Muralidharan
>> Sent: Wednesday, November 25, 2009 5:07 AM
>> To: linux-media@vger.kernel.org; hverk...@xs4all.nl;
>> khil...@deeprootsystems.com
>> Cc: davinci-linux-open-sou...@linux.davincidsp.com
>> Subject: [PATCH - v0 1/2] V4L - vpfe capture - make clocks
>> configurable
>>
>> From: Muralidharan Karicheri 
>>
>> On DM365 we use only vpss master clock, where as on DM355 and
>> DM6446, we use vpss master and slave clocks for vpfe capture.
>[Hiremath, Vaibhav] Adding one more platform here, AM3517, which uses 2
>clocks, internal clock used by internal logic and pixel clock.
>
>Some minor review comments below -
>
>Please note that I have validated these changes already on AM3517EVM, so
>adding -
>
>Tested-by: Vaibhav Hiremath 
>Acked-by: Vaibhav Hiremath 
>
>Thanks,
>Vaibhav
>> So
>> this
>> patch makes it configurable on a per platform basis. This is
>> needed for supporting DM365 for which patches will be available
>> soon.
>>
>> This is for review only. For merge to upstream, it will be
>> re-crated against the v4l-dbv tree.
>>
>> Signed-off-by: Muralidharan Karicheri 
>> ---
>>  drivers/media/video/davinci/vpfe_capture.c |   98
>> +--
>>  include/media/davinci/vpfe_capture.h   |   11 ++-
>>  2 files changed, 70 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/video/davinci/vpfe_capture.c
>> b/drivers/media/video/davinci/vpfe_capture.c
>> index cad60e3..1ba9d07 100644
>> --- a/drivers/media/video/davinci/vpfe_capture.c
>> +++ b/drivers/media/video/davinci/vpfe_capture.c
>> @@ -1743,61 +1743,87 @@ static struct vpfe_device
>> *vpfe_initialize(void)
>>  return vpfe_dev;
>>  }
>>
>> +/**
>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>> + * @vpfe_dev - ptr to vpfe capture device
>> + *
>> + * Disables clocks defined in vpfe configuration.
>> + */
>>  static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>>  {
>>  struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>> +int i;
>>
>> -clk_disable(vpfe_cfg->vpssclk);
>> -clk_put(vpfe_cfg->vpssclk);
>> -clk_disable(vpfe_cfg->slaveclk);
>> -clk_put(vpfe_cfg->slaveclk);
>> -v4l2_info(vpfe_dev->pdev->driver,
>> - "vpfe vpss master & slave clocks disabled\n");
>> +for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>> +clk_disable(vpfe_dev->clks[i]);
>> +clk_put(vpfe_dev->clks[i]);
>> +}
>> +kfree(vpfe_dev->clks);
>> +v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks
>> disabled\n");
>>  }
>>
>> +/**
>> + * vpfe_enable_clock() - Enable clocks for vpfe capture driver
>> + * @vpfe_dev - ptr to vpfe capture device
>> + *
>> + * Enables clocks defined in vpfe configuration. The function
>> + * assumes that at least one clock is to be defined which is
>> + * true as of now. re-visit this if this assumption is not true
>> + */
>>  static int vpfe_enable_clock(struct vpfe_device *vpfe_dev)
>>  {
>>  struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>> -int ret = -ENOENT;
>> +int ret = -EFAULT, i;
>[Hiremath, Vaibhav] No need of variable "ret", since we are not making use
>of it. We can directly return -EFAULT from out: label.
>
>
>Thanks,
>Vaibhav
>>
>> -vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master");
>> -if (NULL == vpfe_cfg->vpssclk) {
>> -v4l2_err(vpfe_dev->pdev->driver, "No clock defined for"
>> - "vpss_master\n");
>> -return ret;
>> -}
>> +if (!vpfe_cfg->num_clocks)
>> +   

RE: [PATCH - v0 1/2] V4L - vpfe capture - make clocks configurable

2009-11-24 Thread Hiremath, Vaibhav

> -Original Message-
> From: davinci-linux-open-source-boun...@linux.davincidsp.com
> [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com] On
> Behalf Of Karicheri, Muralidharan
> Sent: Wednesday, November 25, 2009 5:07 AM
> To: linux-media@vger.kernel.org; hverk...@xs4all.nl;
> khil...@deeprootsystems.com
> Cc: davinci-linux-open-sou...@linux.davincidsp.com
> Subject: [PATCH - v0 1/2] V4L - vpfe capture - make clocks
> configurable
> 
> From: Muralidharan Karicheri 
> 
> On DM365 we use only vpss master clock, where as on DM355 and
> DM6446, we use vpss master and slave clocks for vpfe capture. 
[Hiremath, Vaibhav] Adding one more platform here, AM3517, which uses 2 clocks, 
internal clock used by internal logic and pixel clock.

Some minor review comments below - 

Please note that I have validated these changes already on AM3517EVM, so adding 
-

Tested-by: Vaibhav Hiremath 
Acked-by: Vaibhav Hiremath 

Thanks,
Vaibhav
> So
> this
> patch makes it configurable on a per platform basis. This is
> needed for supporting DM365 for which patches will be available
> soon.
> 
> This is for review only. For merge to upstream, it will be
> re-crated against the v4l-dbv tree.
> 
> Signed-off-by: Muralidharan Karicheri 
> ---
>  drivers/media/video/davinci/vpfe_capture.c |   98
> +--
>  include/media/davinci/vpfe_capture.h   |   11 ++-
>  2 files changed, 70 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/video/davinci/vpfe_capture.c
> b/drivers/media/video/davinci/vpfe_capture.c
> index cad60e3..1ba9d07 100644
> --- a/drivers/media/video/davinci/vpfe_capture.c
> +++ b/drivers/media/video/davinci/vpfe_capture.c
> @@ -1743,61 +1743,87 @@ static struct vpfe_device
> *vpfe_initialize(void)
>   return vpfe_dev;
>  }
> 
> +/**
> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
> + * @vpfe_dev - ptr to vpfe capture device
> + *
> + * Disables clocks defined in vpfe configuration.
> + */
>  static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>  {
>   struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
> + int i;
> 
> - clk_disable(vpfe_cfg->vpssclk);
> - clk_put(vpfe_cfg->vpssclk);
> - clk_disable(vpfe_cfg->slaveclk);
> - clk_put(vpfe_cfg->slaveclk);
> - v4l2_info(vpfe_dev->pdev->driver,
> -  "vpfe vpss master & slave clocks disabled\n");
> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
> + clk_disable(vpfe_dev->clks[i]);
> + clk_put(vpfe_dev->clks[i]);
> + }
> + kfree(vpfe_dev->clks);
> + v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks
> disabled\n");
>  }
> 
> +/**
> + * vpfe_enable_clock() - Enable clocks for vpfe capture driver
> + * @vpfe_dev - ptr to vpfe capture device
> + *
> + * Enables clocks defined in vpfe configuration. The function
> + * assumes that at least one clock is to be defined which is
> + * true as of now. re-visit this if this assumption is not true
> + */
>  static int vpfe_enable_clock(struct vpfe_device *vpfe_dev)
>  {
>   struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
> - int ret = -ENOENT;
> + int ret = -EFAULT, i;
[Hiremath, Vaibhav] No need of variable "ret", since we are not making use of 
it. We can directly return -EFAULT from out: label.


Thanks,
Vaibhav
> 
> - vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master");
> - if (NULL == vpfe_cfg->vpssclk) {
> - v4l2_err(vpfe_dev->pdev->driver, "No clock defined for"
> -  "vpss_master\n");
> - return ret;
> - }
> + if (!vpfe_cfg->num_clocks)
> + return 0;
> 
> - if (clk_enable(vpfe_cfg->vpssclk)) {
> - v4l2_err(vpfe_dev->pdev->driver,
> - "vpfe vpss master clock not enabled\n");
> - goto out;
> - }
> - v4l2_info(vpfe_dev->pdev->driver,
> -  "vpfe vpss master clock enabled\n");
> + vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks *
> +sizeof(struct clock *), GFP_KERNEL);
> 
> - vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave");
> - if (NULL == vpfe_cfg->slaveclk) {
> - v4l2_err(vpfe_dev->pdev->driver,
> - "No clock defined for vpss slave\n");
> - goto out;
> + if (NULL == vpfe_dev->clks) {
> + v4l2_err(vpfe_dev->pdev->driver, "Memory allocation
> failed\n");
> + return -ENOMEM;
>   }
> 
> - if (clk_enable(vpfe_cfg->slaveclk)) {
> - v4l2_err(vpfe_dev->pdev->driver,
> -  "vpfe vpss slave clock not enabled\n");
> - goto out;
> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
> + if (NULL == vpfe_cfg->clocks[i]) {
> + v4l2_err(vpfe_dev->pdev->driver,
> + "clock %s is not defined in vpfe config\n",
> + vpfe_cfg->clocks[i]);
> + goto out;
> +