Re: [PATCH] davinci: vpif: add pm_runtime support

2013-04-01 Thread Sekhar Nori 
On 3/28/2013 3:50 PM, Prabhakar Lad wrote:
 Hi Laurent,
 
 On Thu, Mar 28, 2013 at 3:40 PM, Laurent Pinchart
 laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 On Thursday 28 March 2013 15:36:11 Prabhakar Lad wrote:
 On Thu, Mar 28, 2013 at 2:39 PM, Laurent Pinchart wrote:
 On Thursday 28 March 2013 14:20:32 Prabhakar lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 Add pm_runtime support to the TI Davinci VPIF driver.
 Along side this patch replaces clk_get() with devm_clk_get()
 to simplify the error handling.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---

  drivers/media/platform/davinci/vpif.c |   21 +++--
  1 files changed, 11 insertions(+), 10 deletions(-)

 diff --git a/drivers/media/platform/davinci/vpif.c
 b/drivers/media/platform/davinci/vpif.c index 28638a8..7d14625 100644
 --- a/drivers/media/platform/davinci/vpif.c
 +++ b/drivers/media/platform/davinci/vpif.c

 [snip]

 @@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
   goto fail;
   }

 - vpif_clk = clk_get(pdev-dev, vpif);
 + vpif_clk = devm_clk_get(pdev-dev, vpif);
   if (IS_ERR(vpif_clk)) {
   status = PTR_ERR(vpif_clk);
   goto clk_fail;
   }

 - clk_prepare_enable(vpif_clk);
 + clk_put(vpif_clk);

 Why do you need to call clk_put() here ?

 The above check is to see if the clock is provided, once done
 we free it using clk_put().

 In that case you shouldn't use devm_clk_get(), otherwise clk_put() will be
 called again automatically at remove() time.

 Yes agreed it should be clk_get() only.
 
 + pm_runtime_enable(pdev-dev);
 + pm_runtime_resume(pdev-dev);
 +
 + pm_runtime_get(pdev-dev);

 Does runtime PM automatically handle your clock ? If so can't you remove
 clock handling from the driver completely ?

 Yes  pm runtime take care of enabling/disabling the clocks
 so that we don't have to do it in drivers. I believe clock
 handling is removed with this patch, with just  devm_clk_get() remaining ;)

 When is the clk_get() call expected to fail ? If the clock is provided by the
 SoC and always available, can't the check be removed completely ?

 Yes I agree with you it can be removed completely assuming the clock
 is always available from the Soc.
 But may be I need feedback from others Hans/Sekhar what do you suggest ?

Unless you need the clk handle to get the clock rate or something, you
should simply rely on runtime PM calls to enable clocks for you and not
have any clk API call at all in your driver.

Thanks,
Sekhar
--
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

Re: [PATCH] davinci: vpif: add pm_runtime support

2013-04-01 Thread Prabhakar Lad 
Hi Sekhar,

On Mon, Apr 1, 2013 at 11:33 AM, Sekhar Nori nsek...@ti.com wrote:
 On 3/28/2013 3:50 PM, Prabhakar Lad wrote:
 Hi Laurent,

 On Thu, Mar 28, 2013 at 3:40 PM, Laurent Pinchart
 laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 On Thursday 28 March 2013 15:36:11 Prabhakar Lad wrote:
 On Thu, Mar 28, 2013 at 2:39 PM, Laurent Pinchart wrote:
 On Thursday 28 March 2013 14:20:32 Prabhakar lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 Add pm_runtime support to the TI Davinci VPIF driver.
 Along side this patch replaces clk_get() with devm_clk_get()
 to simplify the error handling.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---

  drivers/media/platform/davinci/vpif.c |   21 +++--
  1 files changed, 11 insertions(+), 10 deletions(-)

 diff --git a/drivers/media/platform/davinci/vpif.c
 b/drivers/media/platform/davinci/vpif.c index 28638a8..7d14625 100644
 --- a/drivers/media/platform/davinci/vpif.c
 +++ b/drivers/media/platform/davinci/vpif.c

 [snip]

 @@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
   goto fail;
   }

 - vpif_clk = clk_get(pdev-dev, vpif);
 + vpif_clk = devm_clk_get(pdev-dev, vpif);
   if (IS_ERR(vpif_clk)) {
   status = PTR_ERR(vpif_clk);
   goto clk_fail;
   }

 - clk_prepare_enable(vpif_clk);
 + clk_put(vpif_clk);

 Why do you need to call clk_put() here ?

 The above check is to see if the clock is provided, once done
 we free it using clk_put().

 In that case you shouldn't use devm_clk_get(), otherwise clk_put() will be
 called again automatically at remove() time.

 Yes agreed it should be clk_get() only.

 + pm_runtime_enable(pdev-dev);
 + pm_runtime_resume(pdev-dev);
 +
 + pm_runtime_get(pdev-dev);

 Does runtime PM automatically handle your clock ? If so can't you remove
 clock handling from the driver completely ?

 Yes  pm runtime take care of enabling/disabling the clocks
 so that we don't have to do it in drivers. I believe clock
 handling is removed with this patch, with just  devm_clk_get() remaining ;)

 When is the clk_get() call expected to fail ? If the clock is provided by 
 the
 SoC and always available, can't the check be removed completely ?

 Yes I agree with you it can be removed completely assuming the clock
 is always available from the Soc.
 But may be I need feedback from others Hans/Sekhar what do you suggest ?

 Unless you need the clk handle to get the clock rate or something, you
 should simply rely on runtime PM calls to enable clocks for you and not
 have any clk API call at all in your driver.

Thanks this helps!, I'll post a v2 fixing it.

Regards,
--Prabhakar

 Thanks,
 Sekhar
--
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

[PATCH] davinci: vpif: add pm_runtime support

2013-03-28 Thread Prabhakar lad 
From: Lad, Prabhakar prabhakar.cse...@gmail.com

Add pm_runtime support to the TI Davinci VPIF driver.
Along side this patch replaces clk_get() with devm_clk_get()
to simplify the error handling.

Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
---
 drivers/media/platform/davinci/vpif.c |   21 +++--
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c 
b/drivers/media/platform/davinci/vpif.c
index 28638a8..7d14625 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -25,6 +25,7 @@
 #include linux/io.h
 #include linux/clk.h
 #include linux/err.h
+#include linux/pm_runtime.h
 #include linux/v4l2-dv-timings.h
 
 #include mach/hardware.h
@@ -44,7 +45,6 @@ static struct resource*res;
 spinlock_t vpif_lock;
 
 void __iomem *vpif_base;
-struct clk *vpif_clk;
 
 /**
  * ch_params: video standard configuration parameters for vpif
@@ -421,6 +421,7 @@ EXPORT_SYMBOL(vpif_channel_getfid);
 
 static int vpif_probe(struct platform_device *pdev)
 {
+   struct clk *vpif_clk;
int status = 0;
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
goto fail;
}
 
-   vpif_clk = clk_get(pdev-dev, vpif);
+   vpif_clk = devm_clk_get(pdev-dev, vpif);
if (IS_ERR(vpif_clk)) {
status = PTR_ERR(vpif_clk);
goto clk_fail;
}
-   clk_prepare_enable(vpif_clk);
+   clk_put(vpif_clk);
+
+   pm_runtime_enable(pdev-dev);
+   pm_runtime_resume(pdev-dev);
+
+   pm_runtime_get(pdev-dev);
 
spin_lock_init(vpif_lock);
dev_info(pdev-dev, vpif probe success\n);
@@ -459,11 +465,6 @@ fail:
 
 static int vpif_remove(struct platform_device *pdev)
 {
-   if (vpif_clk) {
-   clk_disable_unprepare(vpif_clk);
-   clk_put(vpif_clk);
-   }
-
iounmap(vpif_base);
release_mem_region(res-start, res_len);
return 0;
@@ -472,13 +473,13 @@ static int vpif_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int vpif_suspend(struct device *dev)
 {
-   clk_disable_unprepare(vpif_clk);
+   pm_runtime_put(dev);
return 0;
 }
 
 static int vpif_resume(struct device *dev)
 {
-   clk_prepare_enable(vpif_clk);
+   pm_runtime_get(dev);
return 0;
 }
 
-- 
1.7.4.1

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

Re: [PATCH] davinci: vpif: add pm_runtime support

2013-03-28 Thread Laurent Pinchart 
Hi Prabhakar,

Thanks for the patch.

On Thursday 28 March 2013 14:20:32 Prabhakar lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 Add pm_runtime support to the TI Davinci VPIF driver.
 Along side this patch replaces clk_get() with devm_clk_get()
 to simplify the error handling.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---
  drivers/media/platform/davinci/vpif.c |   21 +++--
  1 files changed, 11 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/media/platform/davinci/vpif.c
 b/drivers/media/platform/davinci/vpif.c index 28638a8..7d14625 100644
 --- a/drivers/media/platform/davinci/vpif.c
 +++ b/drivers/media/platform/davinci/vpif.c
 @@ -25,6 +25,7 @@
  #include linux/io.h
  #include linux/clk.h
  #include linux/err.h
 +#include linux/pm_runtime.h
  #include linux/v4l2-dv-timings.h
 
  #include mach/hardware.h
 @@ -44,7 +45,6 @@ static struct resource  *res;
  spinlock_t vpif_lock;
 
  void __iomem *vpif_base;
 -struct clk *vpif_clk;
 
  /**
   * ch_params: video standard configuration parameters for vpif
 @@ -421,6 +421,7 @@ EXPORT_SYMBOL(vpif_channel_getfid);
 
  static int vpif_probe(struct platform_device *pdev)
  {
 + struct clk *vpif_clk;
   int status = 0;
 
   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 @@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
   goto fail;
   }
 
 - vpif_clk = clk_get(pdev-dev, vpif);
 + vpif_clk = devm_clk_get(pdev-dev, vpif);
   if (IS_ERR(vpif_clk)) {
   status = PTR_ERR(vpif_clk);
   goto clk_fail;
   }
 - clk_prepare_enable(vpif_clk);
 + clk_put(vpif_clk);

Why do you need to call clk_put() here ?

 + pm_runtime_enable(pdev-dev);
 + pm_runtime_resume(pdev-dev);
 +
 + pm_runtime_get(pdev-dev);

Does runtime PM automatically handle your clock ? If so can't you remove clock 
handling from the driver completely ?

   spin_lock_init(vpif_lock);
   dev_info(pdev-dev, vpif probe success\n);
 @@ -459,11 +465,6 @@ fail:
 
  static int vpif_remove(struct platform_device *pdev)
  {
 - if (vpif_clk) {
 - clk_disable_unprepare(vpif_clk);
 - clk_put(vpif_clk);
 - }
 -
   iounmap(vpif_base);
   release_mem_region(res-start, res_len);
   return 0;
 @@ -472,13 +473,13 @@ static int vpif_remove(struct platform_device *pdev)
  #ifdef CONFIG_PM
  static int vpif_suspend(struct device *dev)
  {
 - clk_disable_unprepare(vpif_clk);
 + pm_runtime_put(dev);
   return 0;
  }
 
  static int vpif_resume(struct device *dev)
  {
 - clk_prepare_enable(vpif_clk);
 + pm_runtime_get(dev);
   return 0;
  }
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] davinci: vpif: add pm_runtime support

2013-03-28 Thread Prabhakar Lad 
Hi Laurent,

Thanks for the quick review!

On Thu, Mar 28, 2013 at 2:39 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 Thanks for the patch.

 On Thursday 28 March 2013 14:20:32 Prabhakar lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 Add pm_runtime support to the TI Davinci VPIF driver.
 Along side this patch replaces clk_get() with devm_clk_get()
 to simplify the error handling.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---
  drivers/media/platform/davinci/vpif.c |   21 +++--
  1 files changed, 11 insertions(+), 10 deletions(-)

 diff --git a/drivers/media/platform/davinci/vpif.c
 b/drivers/media/platform/davinci/vpif.c index 28638a8..7d14625 100644
 --- a/drivers/media/platform/davinci/vpif.c
 +++ b/drivers/media/platform/davinci/vpif.c
 @@ -25,6 +25,7 @@
  #include linux/io.h
  #include linux/clk.h
  #include linux/err.h
 +#include linux/pm_runtime.h
  #include linux/v4l2-dv-timings.h

  #include mach/hardware.h
 @@ -44,7 +45,6 @@ static struct resource  *res;
  spinlock_t vpif_lock;

  void __iomem *vpif_base;
 -struct clk *vpif_clk;

  /**
   * ch_params: video standard configuration parameters for vpif
 @@ -421,6 +421,7 @@ EXPORT_SYMBOL(vpif_channel_getfid);

  static int vpif_probe(struct platform_device *pdev)
  {
 + struct clk *vpif_clk;
   int status = 0;

   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 @@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
   goto fail;
   }

 - vpif_clk = clk_get(pdev-dev, vpif);
 + vpif_clk = devm_clk_get(pdev-dev, vpif);
   if (IS_ERR(vpif_clk)) {
   status = PTR_ERR(vpif_clk);
   goto clk_fail;
   }
 - clk_prepare_enable(vpif_clk);
 + clk_put(vpif_clk);

 Why do you need to call clk_put() here ?

The above check is to see if the clock is provided, once done
we free it using clk_put().

 + pm_runtime_enable(pdev-dev);
 + pm_runtime_resume(pdev-dev);
 +
 + pm_runtime_get(pdev-dev);

 Does runtime PM automatically handle your clock ? If so can't you remove clock
 handling from the driver completely ?

Yes  pm runtime take care of enabling/disabling the clocks
so that we don't have to do it in drivers. I believe clock
handling is removed with this patch, with just  devm_clk_get() remaining ;)

Regards,
--Prabhakar

   spin_lock_init(vpif_lock);
   dev_info(pdev-dev, vpif probe success\n);
 @@ -459,11 +465,6 @@ fail:

  static int vpif_remove(struct platform_device *pdev)
  {
 - if (vpif_clk) {
 - clk_disable_unprepare(vpif_clk);
 - clk_put(vpif_clk);
 - }
 -
   iounmap(vpif_base);
   release_mem_region(res-start, res_len);
   return 0;
 @@ -472,13 +473,13 @@ static int vpif_remove(struct platform_device *pdev)
  #ifdef CONFIG_PM
  static int vpif_suspend(struct device *dev)
  {
 - clk_disable_unprepare(vpif_clk);
 + pm_runtime_put(dev);
   return 0;
  }

  static int vpif_resume(struct device *dev)
  {
 - clk_prepare_enable(vpif_clk);
 + pm_runtime_get(dev);
   return 0;
  }
 --
 Regards,

 Laurent Pinchart

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

Re: [PATCH] davinci: vpif: add pm_runtime support

2013-03-28 Thread Laurent Pinchart 
Hi Prabhakar,

On Thursday 28 March 2013 15:36:11 Prabhakar Lad wrote:
 On Thu, Mar 28, 2013 at 2:39 PM, Laurent Pinchart wrote:
  On Thursday 28 March 2013 14:20:32 Prabhakar lad wrote:
  From: Lad, Prabhakar prabhakar.cse...@gmail.com
  
  Add pm_runtime support to the TI Davinci VPIF driver.
  Along side this patch replaces clk_get() with devm_clk_get()
  to simplify the error handling.
  
  Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
  ---
  
   drivers/media/platform/davinci/vpif.c |   21 +++--
   1 files changed, 11 insertions(+), 10 deletions(-)
  
  diff --git a/drivers/media/platform/davinci/vpif.c
  b/drivers/media/platform/davinci/vpif.c index 28638a8..7d14625 100644
  --- a/drivers/media/platform/davinci/vpif.c
  +++ b/drivers/media/platform/davinci/vpif.c

[snip]

  @@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
goto fail;
}
  
  - vpif_clk = clk_get(pdev-dev, vpif);
  + vpif_clk = devm_clk_get(pdev-dev, vpif);
if (IS_ERR(vpif_clk)) {
status = PTR_ERR(vpif_clk);
goto clk_fail;
}
  
  - clk_prepare_enable(vpif_clk);
  + clk_put(vpif_clk);
  
  Why do you need to call clk_put() here ?
 
 The above check is to see if the clock is provided, once done
 we free it using clk_put().

In that case you shouldn't use devm_clk_get(), otherwise clk_put() will be 
called again automatically at remove() time.

  + pm_runtime_enable(pdev-dev);
  + pm_runtime_resume(pdev-dev);
  +
  + pm_runtime_get(pdev-dev);
  
  Does runtime PM automatically handle your clock ? If so can't you remove
  clock handling from the driver completely ?
 
 Yes  pm runtime take care of enabling/disabling the clocks
 so that we don't have to do it in drivers. I believe clock
 handling is removed with this patch, with just  devm_clk_get() remaining ;)

When is the clk_get() call expected to fail ? If the clock is provided by the 
SoC and always available, can't the check be removed completely ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] davinci: vpif: add pm_runtime support

2013-03-28 Thread Prabhakar Lad 
Hi Laurent,

On Thu, Mar 28, 2013 at 3:40 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 On Thursday 28 March 2013 15:36:11 Prabhakar Lad wrote:
 On Thu, Mar 28, 2013 at 2:39 PM, Laurent Pinchart wrote:
  On Thursday 28 March 2013 14:20:32 Prabhakar lad wrote:
  From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
  Add pm_runtime support to the TI Davinci VPIF driver.
  Along side this patch replaces clk_get() with devm_clk_get()
  to simplify the error handling.
 
  Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
  ---
 
   drivers/media/platform/davinci/vpif.c |   21 +++--
   1 files changed, 11 insertions(+), 10 deletions(-)
 
  diff --git a/drivers/media/platform/davinci/vpif.c
  b/drivers/media/platform/davinci/vpif.c index 28638a8..7d14625 100644
  --- a/drivers/media/platform/davinci/vpif.c
  +++ b/drivers/media/platform/davinci/vpif.c

 [snip]

  @@ -439,12 +440,17 @@ static int vpif_probe(struct platform_device *pdev)
goto fail;
}
 
  - vpif_clk = clk_get(pdev-dev, vpif);
  + vpif_clk = devm_clk_get(pdev-dev, vpif);
if (IS_ERR(vpif_clk)) {
status = PTR_ERR(vpif_clk);
goto clk_fail;
}
 
  - clk_prepare_enable(vpif_clk);
  + clk_put(vpif_clk);
 
  Why do you need to call clk_put() here ?

 The above check is to see if the clock is provided, once done
 we free it using clk_put().

 In that case you shouldn't use devm_clk_get(), otherwise clk_put() will be
 called again automatically at remove() time.

Yes agreed it should be clk_get() only.

  + pm_runtime_enable(pdev-dev);
  + pm_runtime_resume(pdev-dev);
  +
  + pm_runtime_get(pdev-dev);
 
  Does runtime PM automatically handle your clock ? If so can't you remove
  clock handling from the driver completely ?

 Yes  pm runtime take care of enabling/disabling the clocks
 so that we don't have to do it in drivers. I believe clock
 handling is removed with this patch, with just  devm_clk_get() remaining ;)

 When is the clk_get() call expected to fail ? If the clock is provided by the
 SoC and always available, can't the check be removed completely ?

Yes I agree with you it can be removed completely assuming the clock
is always available from the Soc.
But may be I need feedback from others Hans/Sekhar what do you suggest ?

Regards,
--Prabhakar

 --
 Regards,

 Laurent Pinchart

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