RE: [PATCH v1 13/16] OMAP3: hwmod DSS: VENC Move init,exit to driver

2010-10-08 Thread Guruswamy, Senthilvadivu 


 -Original Message-
 From: Thomas Petazzoni [mailto:thomas.petazz...@free-electrons.com]
 Sent: Friday, October 08, 2010 1:28 AM
 To: Guruswamy, Senthilvadivu
 Cc: khil...@deeprootsystems.com; tomi.valkei...@nokia.com; p...@pwsan.com;
 Hiremath, Vaibhav; linux-omap@vger.kernel.org
 Subject: Re: [PATCH v1 13/16] OMAP3: hwmod DSS: VENC Move init,exit to
 driver
 
 Hello Senthil,
 
 On Wed,  6 Oct 2010 16:44:56 +0530
 Guruswamy Senthilvadivu svad...@ti.com wrote:
 
  diff --git a/drivers/video/omap2/dss/venc.c
 b/drivers/video/omap2/dss/venc.c
  index ec17b28..3a121cb 100644
  --- a/drivers/video/omap2/dss/venc.c
  +++ b/drivers/video/omap2/dss/venc.c
  @@ -87,26 +87,6 @@
   #define VENC_OUTPUT_TEST   0xC8
   #define VENC_DAC_B__DAC_C  0xC8
 
  -/* VENC HW IP initialisation */
  -static int omap_venchw_probe(struct platform_device *pdev)
  -{
  -   return 0;
  -}
  -
  -static int omap_venchw_remove(struct platform_device *pdev)
  -{
  -   return 0;
  -}
  -
  -static struct platform_driver omap_venchw_driver = {
  -   .probe  = omap_venchw_probe,
  -   .remove = omap_venchw_remove,
  -   .driver = {
  -   .name   = dss_venc,
  -   .owner  = THIS_MODULE,
  -   },
  -};
 
 Would be better in patch 7/16 to put this stuff at the correct place
 (bottom of the file) so it does not need to be moved here.
 
[Senthil]  Taken.  Will do the movement in the first patch itself.
  +/* VENC HW IP initialisation */
  +static int omap_venchw_probe(struct platform_device *pdev)
  +{
  +   int r;
  +   venc.pdev = pdev;
  +
  +   r = venc_init(pdev);
  +   if (r) {
  +   DSSERR(Failed to initialize venc\n);
  +   goto err_venc;
  +   }
  +
  +err_venc:
  +   return r;
  +}
  +
  +static int omap_venchw_remove(struct platform_device *pdev)
  +{
  +   venc_exit();
  +   return 0;
  +}
 
 Same comment as before: include the code of venc_init() and venc_exit()
 directly in the -probe() and -remove() hooks respectively.
 
  +struct regulator *dss_get_vdda_dac(void)
  +{
  +   struct regulator *reg;
  +
  +   if (venc.vdda_dac_reg != NULL)
  +   return venc.vdda_dac_reg;
  +
  +   reg = regulator_get(venc.pdev-dev, vdda_dac);
  +   if (!IS_ERR(reg))
  +   venc.vdda_dac_reg = reg;
 
  +   return reg;
  +}
 
 As far as I can see, this function is now only used in venc_init(),
 which is in the same file, so the function should be static, and the
 prototype removed from drivers/video/omap2/dss/core.h.
 
 I'm also a bit skeptical about what this function does. It is called
 this way in venc_init():
 
   venc.vdda_dac_reg = dss_get_vdda_dac();
 
 so it is dss_get_vdda_dac() responsability to set venc.vdda_dac_reg, or
 is it the caller responsability ?
 
 Moreover, the logic in dss_get_vdda_dac() that tests whether
 venc.vdda_dac_reg is already initialized seems to indicate that this
 function could be called several times. However, I only see it called
 from venc_init(), which as far as I understand is called only once.
 
[Senthil] when code was in core.c there were chances of it getting called more 
than once.  Now it is called only once as you see, so it could be made static.
 Isn't it possible to simplify this by removing the dss_get_vdda_dac()
 function and just doing:
 
   venc.vdda_dac_reg = regulator_get(venc.pdev-dev, vdda_dac);
 
 in the venc_init() function (which should become the -probe() method).
 
 Thanks!
 
 Thomas
 --
 Thomas Petazzoni, Free Electrons
 Kernel, drivers, real-time and embedded Linux
 development, consulting, training and support.
 http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 13/16] OMAP3: hwmod DSS: VENC Move init,exit to driver

2010-10-07 Thread Thomas Petazzoni 
Hello Senthil,

On Wed,  6 Oct 2010 16:44:56 +0530
Guruswamy Senthilvadivu svad...@ti.com wrote:

 diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
 index ec17b28..3a121cb 100644
 --- a/drivers/video/omap2/dss/venc.c
 +++ b/drivers/video/omap2/dss/venc.c
 @@ -87,26 +87,6 @@
  #define VENC_OUTPUT_TEST 0xC8
  #define VENC_DAC_B__DAC_C0xC8
  
 -/* VENC HW IP initialisation */
 -static int omap_venchw_probe(struct platform_device *pdev)
 -{
 - return 0;
 -}
 -
 -static int omap_venchw_remove(struct platform_device *pdev)
 -{
 - return 0;
 -}
 -
 -static struct platform_driver omap_venchw_driver = {
 - .probe  = omap_venchw_probe,
 - .remove = omap_venchw_remove,
 - .driver = {
 - .name   = dss_venc,
 - .owner  = THIS_MODULE,
 - },
 -};

Would be better in patch 7/16 to put this stuff at the correct place
(bottom of the file) so it does not need to be moved here.

 +/* VENC HW IP initialisation */
 +static int omap_venchw_probe(struct platform_device *pdev)
 +{
 + int r;
 + venc.pdev = pdev;
 +
 + r = venc_init(pdev);
 + if (r) {
 + DSSERR(Failed to initialize venc\n);
 + goto err_venc;
 + }
 +
 +err_venc:
 + return r;
 +}
 +
 +static int omap_venchw_remove(struct platform_device *pdev)
 +{
 + venc_exit();
 + return 0;
 +}

Same comment as before: include the code of venc_init() and venc_exit()
directly in the -probe() and -remove() hooks respectively.

 +struct regulator *dss_get_vdda_dac(void)
 +{
 + struct regulator *reg;
 +
 + if (venc.vdda_dac_reg != NULL)
 + return venc.vdda_dac_reg;
 +
 + reg = regulator_get(venc.pdev-dev, vdda_dac);
 + if (!IS_ERR(reg))
 + venc.vdda_dac_reg = reg;
  
 + return reg;
 +}

As far as I can see, this function is now only used in venc_init(),
which is in the same file, so the function should be static, and the
prototype removed from drivers/video/omap2/dss/core.h.

I'm also a bit skeptical about what this function does. It is called
this way in venc_init():

  venc.vdda_dac_reg = dss_get_vdda_dac();

so it is dss_get_vdda_dac() responsability to set venc.vdda_dac_reg, or
is it the caller responsability ?

Moreover, the logic in dss_get_vdda_dac() that tests whether
venc.vdda_dac_reg is already initialized seems to indicate that this
function could be called several times. However, I only see it called
from venc_init(), which as far as I understand is called only once.

Isn't it possible to simplify this by removing the dss_get_vdda_dac()
function and just doing:

  venc.vdda_dac_reg = regulator_get(venc.pdev-dev, vdda_dac);

in the venc_init() function (which should become the -probe() method).

Thanks!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 13/16] OMAP3: hwmod DSS: VENC Move init,exit to driver

2010-10-06 Thread Guruswamy Senthilvadivu 
From: Senthilvadivu Guruswamy svad...@ti.com

Move init exit methods to its driver probe, remove.
pdev member has to be maintained by its own drivers.
regulator has to be privately handled in each driver.

Signed-off-by: Senthilvadivu Guruswamy svad...@ti.com
---
 arch/arm/mach-omap2/board-3430sdp.c |2 +-
 drivers/video/omap2/dss/core.c  |   29 ---
 drivers/video/omap2/dss/venc.c  |   69 +--
 3 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/arch/arm/mach-omap2/board-3430sdp.c 
b/arch/arm/mach-omap2/board-3430sdp.c
index 62b6523..edcfaff 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -303,7 +303,7 @@ static struct omap_dss_board_info sdp3430_dss_data = {
 };
 
 static struct regulator_consumer_supply sdp3430_vdda_dac_supply =
-   REGULATOR_SUPPLY(vdda_dac, omapdisplay);
+   REGULATOR_SUPPLY(vdda_dac, dss_venc);
 
 static struct omap_board_config_kernel sdp3430_config[] __initdata = {
 };
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 9652ed7..df74116 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -43,7 +43,6 @@ static struct {
 
struct regulator *vdds_dsi_reg;
struct regulator *vdds_sdi_reg;
-   struct regulator *vdda_dac_reg;
 } core;
 
 
@@ -86,20 +85,6 @@ struct regulator *dss_get_vdds_sdi(void)
return reg;
 }
 
-struct regulator *dss_get_vdda_dac(void)
-{
-   struct regulator *reg;
-
-   if (core.vdda_dac_reg != NULL)
-   return core.vdda_dac_reg;
-
-   reg = regulator_get(core.pdev-dev, vdda_dac);
-   if (!IS_ERR(reg))
-   core.vdda_dac_reg = reg;
-
-   return reg;
-}
-
 #if defined(CONFIG_DEBUG_FS)  defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
 static int dss_debug_show(struct seq_file *s, void *unused)
 {
@@ -199,12 +184,6 @@ static int omap_dss_probe(struct platform_device *pdev)
goto err_dpi;
}
 
-   r = venc_init(pdev);
-   if (r) {
-   DSSERR(Failed to initialize venc\n);
-   goto err_venc;
-   }
-
if (cpu_is_omap34xx()) {
r = sdi_init(skip_init);
if (r) {
@@ -254,8 +233,6 @@ err_dsi:
if (cpu_is_omap34xx())
sdi_exit();
 err_sdi:
-   venc_exit();
-err_venc:
dpi_exit();
 err_dpi:
 
@@ -269,7 +246,6 @@ static int omap_dss_remove(struct platform_device *pdev)
 
dss_uninitialize_debugfs();
 
-   venc_exit();
dpi_exit();
if (cpu_is_omap34xx()) {
dsi_exit();
@@ -562,11 +538,6 @@ static void __exit omap_dss_exit(void)
core.vdds_sdi_reg = NULL;
}
 
-   if (core.vdda_dac_reg != NULL) {
-   regulator_put(core.vdda_dac_reg);
-   core.vdda_dac_reg = NULL;
-   }
-
platform_driver_unregister(omap_dss_driver);
 
omap_dss_bus_unregister();
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index ec17b28..3a121cb 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -87,26 +87,6 @@
 #define VENC_OUTPUT_TEST   0xC8
 #define VENC_DAC_B__DAC_C  0xC8
 
-/* VENC HW IP initialisation */
-static int omap_venchw_probe(struct platform_device *pdev)
-{
-   return 0;
-}
-
-static int omap_venchw_remove(struct platform_device *pdev)
-{
-   return 0;
-}
-
-static struct platform_driver omap_venchw_driver = {
-   .probe  = omap_venchw_probe,
-   .remove = omap_venchw_remove,
-   .driver = {
-   .name   = dss_venc,
-   .owner  = THIS_MODULE,
-   },
-};
-
 struct venc_config {
u32 f_control;
u32 vidout_ctrl;
@@ -309,6 +289,7 @@ const struct omap_video_timings omap_dss_ntsc_timings = {
 EXPORT_SYMBOL(omap_dss_ntsc_timings);
 
 static struct {
+   struct platform_device *pdev;
void __iomem *base;
struct mutex venc_lock;
u32 wss_data;
@@ -326,6 +307,37 @@ static inline u32 venc_read_reg(int idx)
return l;
 }
 
+/* VENC HW IP initialisation */
+static int omap_venchw_probe(struct platform_device *pdev)
+{
+   int r;
+   venc.pdev = pdev;
+
+   r = venc_init(pdev);
+   if (r) {
+   DSSERR(Failed to initialize venc\n);
+   goto err_venc;
+   }
+
+err_venc:
+   return r;
+}
+
+static int omap_venchw_remove(struct platform_device *pdev)
+{
+   venc_exit();
+   return 0;
+}
+
+static struct platform_driver omap_venchw_driver = {
+   .probe  = omap_venchw_probe,
+   .remove = omap_venchw_remove,
+   .driver = {
+   .name   = dss_venc,
+   .owner  = THIS_MODULE,
+   },
+};
+
 static void venc_write_config(const struct venc_config *config)
 {
DSSDBG(write venc conf\n);
@@ -661,7 +673,19 @@