[PATCH] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[PATCH] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Ameya Palande]

This patch does following things:
1. Instead of GT_2trace() use pr_err(), since failure to enable or disable a
   clock is error which should be notified.
2. There is no need to check the return value of CLK_Enable and CLK_Disable
   and print error message, since these functions internally print the error.
3. Indentation changes and a typo fix.

Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 drivers/dsp/bridge/services/clk.c       |   42 ++++++++++++++----------------
 drivers/dsp/bridge/wmd/tiomap3430.c     |   13 +--------
 drivers/dsp/bridge/wmd/tiomap3430_pwr.c |   22 ++++-----------
 3 files changed, 28 insertions(+), 49 deletions(-)

diff --git a/drivers/dsp/bridge/services/clk.c b/drivers/dsp/bridge/services/clk.c
index 440706f..fbbde72 100644
--- a/drivers/dsp/bridge/services/clk.c
+++ b/drivers/dsp/bridge/services/clk.c
@@ -188,7 +188,7 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
 	struct clk *pClk;
 
 	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
-       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
+	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
 		"CLK dev id = %d\n", SERVICES_Clks[clk_id].clk_name,
 		SERVICES_Clks[clk_id].id);
 
@@ -197,18 +197,16 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
 		if (clk_enable(pClk) == 0x0) {
 			/* Success ? */
 		} else {
-		       GT_2trace(CLK_debugMask, GT_7CLASS,
-				 "CLK_Enable: failed to Enable CLK %s, "
-				 "CLK dev id = %d\n",
-				 SERVICES_Clks[clk_id].clk_name,
-				 SERVICES_Clks[clk_id].id);
+			pr_err("CLK_Enable: failed to Enable CLK %s, "
+					"CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 			status = DSP_EFAIL;
 		}
 	} else {
-	       GT_2trace(CLK_debugMask, GT_7CLASS,
-			 "CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
-			 SERVICES_Clks[clk_id].clk_name,
-			 SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 		status = DSP_EFAIL;
 	}
 	/* The SSI module need to configured not to have the Forced idle for
@@ -274,15 +272,15 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 
 	clkUseCnt = CLK_Get_UseCnt(clk_id);
 	if (clkUseCnt == -1) {
-	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: failed to "
-			"get CLK Use count for CLK %s, CLK dev id = %d\n",
-			SERVICES_Clks[clk_id].clk_name,
-			SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Disable: failed to get CLK Use count for CLK %s,
+				CLK dev id = %d\n",
+				SERVICES_Clks[clk_id].clk_name,
+				SERVICES_Clks[clk_id].id);
 	} else if (clkUseCnt == 0) {
-	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: CLK %s, "
-			"CLK dev id= %d is already disabled\n",
-			SERVICES_Clks[clk_id].clk_name,
-			SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Disable: CLK %s, CLK dev id= %d is already
+				disabled\n",
+				SERVICES_Clks[clk_id].clk_name,
+				SERVICES_Clks[clk_id].id);
 		 return status;
 	}
 	if (clk_id == SERVICESCLK_ssi_ick)
@@ -291,10 +289,10 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 		if (pClk) {
 			clk_disable(pClk);
 		} else {
-		       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: "
-				"failed to get CLK %s, CLK dev id = %d\n",
-				SERVICES_Clks[clk_id].clk_name,
-				SERVICES_Clks[clk_id].id);
+			pr_err("CLK_Disable: failed to get CLK %s,
+					CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 			status = DSP_EFAIL;
 		}
 	return status;
diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c b/drivers/dsp/bridge/wmd/tiomap3430.c
index 7fa6f8e..606de3c 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430.c
@@ -2119,7 +2119,6 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
 {
 	u32 temp;
 	DSP_STATUS status = DSP_SOK;
-	DSP_STATUS clk_status = DSP_SOK;
 	enum HW_PwrState_t    pwrState;
 
 	/* Read PM_PWSTST_IVA2 */
@@ -2134,11 +2133,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
 		/* Wait until the state has moved to ON */
 		HW_PWR_IVA2StateGet(prm_base, HW_PWR_DOMAIN_DSP, &pwrState);
 	}
-	clk_status = CLK_Disable(SERVICESCLK_iva2_ck);
-	if (DSP_FAILED(clk_status)) {
-		DBG_Trace(DBG_LEVEL6, "CLK_Disbale failed for clk = 0x%x \n",
-			  SERVICESCLK_iva2_ck);
-	}
+	CLK_Disable(SERVICESCLK_iva2_ck);
 	udelay(10);
 	/* Assert IVA2-RST1 and IVA2-RST2  */
 	*((REG_UWORD32 *)((u32)(prm_base) + 0x50)) = (u32)0x07;
@@ -2155,11 +2150,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
                temp =  (temp & 0xFFFFFC8) | 0x37;
                *((REG_UWORD32 *) ((u32) (cm_base) + 0x4)) =
                        (u32) temp;
-	clk_status = CLK_Enable(SERVICESCLK_iva2_ck);
-	if (DSP_FAILED(clk_status)) {
-		DBG_Trace(DBG_LEVEL6, "CLK_Enable failed for clk = 0x%x \n",
-			  SERVICESCLK_iva2_ck);
-	}
+	CLK_Enable(SERVICESCLK_iva2_ck);
 	udelay(20);
 	GetHWRegs(prm_base, cm_base);
 	/* Release Reset1 and Reset2 */
diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
index 488a512..1ad1565 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
@@ -285,7 +285,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
 #ifdef CONFIG_PM
 	struct CFG_HOSTRES resources;
 	enum HW_PwrState_t pwrState;
-       u32 temp;
+	u32 temp;
 
 	status = CFG_GetHostResources(
 		 (struct CFG_DEVNODE *)DRV_GetFirstDevExtension(), &resources);
@@ -306,7 +306,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
 		 pDevContext->uDspPerClks);
 	status = DSP_PeripheralClocks_Enable(pDevContext, NULL);
 
-       /* Enablifg Dppll in lock mode*/
+	/* Enabling Dppll in lock mode */
                temp = (u32) *((REG_UWORD32 *)
                        ((u32) (resources.dwCmBase) + 0x34));
                temp = (temp & 0xFFFFFFFE) | 0x1;
@@ -546,27 +546,17 @@ DSP_STATUS DSP_PeripheralClocks_Enable(struct WMD_DEV_CONTEXT *pDevContext,
 				      IN void *pArgs)
 {
 	u32 clkIdx;
-	DSP_STATUS status = DSP_SOK;
+	DSP_STATUS int_clk_status = DSP_EFAIL, fun_clk_status = DSP_EFAIL;
 
 	for (clkIdx = 0; clkIdx < MBX_PM_MAX_RESOURCES; clkIdx++) {
 		if (((pDevContext->uDspPerClks) >> clkIdx) & 0x01) {
 			/* Enable the interface clock of the peripheral */
-			status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
-			if (DSP_FAILED(status)) {
-				DBG_Trace(DBG_LEVEL7,
-					 "Failed to Enable the DSP Peripheral"
-					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
-			}
+			int_clk_status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
 			/* Enable the functional clock of the periphearl */
-			status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
-			if (DSP_FAILED(status)) {
-				DBG_Trace(DBG_LEVEL7,
-					 "Failed to Enable the DSP Peripheral"
-					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
-			}
+			fun_clk_status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
 		}
 	}
-	return status;
+	return ((int_clk_status | fun_clk_status) != DSP_OK) ? DSP_EFAIL : DSP_OK;
 }
 
 void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
-- 
1.6.2.2

RE: [PATCH] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Kanigeri, Hari]

Ameya,

Patch looks good to me.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Ameya Palande
> Sent: Wednesday, April 15, 2009 7:19 AM
> To: linux-omap@vger.kernel.org
> Subject: [PATCH] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup
> 
> This patch does following things:
> 1. Instead of GT_2trace() use pr_err(), since failure to enable or disable
> a
>    clock is error which should be notified.
> 2. There is no need to check the return value of CLK_Enable and
> CLK_Disable
>    and print error message, since these functions internally print the
> error.
> 3. Indentation changes and a typo fix.
> 
> Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
> ---
>  drivers/dsp/bridge/services/clk.c       |   42 ++++++++++++++------------
> ----
>  drivers/dsp/bridge/wmd/tiomap3430.c     |   13 +--------
>  drivers/dsp/bridge/wmd/tiomap3430_pwr.c |   22 ++++-----------
>  3 files changed, 28 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/services/clk.c
> b/drivers/dsp/bridge/services/clk.c
> index 440706f..fbbde72 100644
> --- a/drivers/dsp/bridge/services/clk.c
> +++ b/drivers/dsp/bridge/services/clk.c
> @@ -188,7 +188,7 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
>  	struct clk *pClk;
> 
>  	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
> -       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
> +	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
>  		"CLK dev id = %d\n", SERVICES_Clks[clk_id].clk_name,
>  		SERVICES_Clks[clk_id].id);
> 
> @@ -197,18 +197,16 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
>  		if (clk_enable(pClk) == 0x0) {
>  			/* Success ? */
>  		} else {
> -		       GT_2trace(CLK_debugMask, GT_7CLASS,
> -				 "CLK_Enable: failed to Enable CLK %s, "
> -				 "CLK dev id = %d\n",
> -				 SERVICES_Clks[clk_id].clk_name,
> -				 SERVICES_Clks[clk_id].id);
> +			pr_err("CLK_Enable: failed to Enable CLK %s, "
> +					"CLK dev id = %d\n",
> +					SERVICES_Clks[clk_id].clk_name,
> +					SERVICES_Clks[clk_id].id);
>  			status = DSP_EFAIL;
>  		}
>  	} else {
> -	       GT_2trace(CLK_debugMask, GT_7CLASS,
> -			 "CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
> -			 SERVICES_Clks[clk_id].clk_name,
> -			 SERVICES_Clks[clk_id].id);
> +		pr_err("CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
> +					SERVICES_Clks[clk_id].clk_name,
> +					SERVICES_Clks[clk_id].id);
>  		status = DSP_EFAIL;
>  	}
>  	/* The SSI module need to configured not to have the Forced idle for
> @@ -274,15 +272,15 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId
> clk_id)
> 
>  	clkUseCnt = CLK_Get_UseCnt(clk_id);
>  	if (clkUseCnt == -1) {
> -	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: failed to "
> -			"get CLK Use count for CLK %s, CLK dev id = %d\n",
> -			SERVICES_Clks[clk_id].clk_name,
> -			SERVICES_Clks[clk_id].id);
> +		pr_err("CLK_Disable: failed to get CLK Use count for CLK %s,
> +				CLK dev id = %d\n",
> +				SERVICES_Clks[clk_id].clk_name,
> +				SERVICES_Clks[clk_id].id);
>  	} else if (clkUseCnt == 0) {
> -	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: CLK %s, "
> -			"CLK dev id= %d is already disabled\n",
> -			SERVICES_Clks[clk_id].clk_name,
> -			SERVICES_Clks[clk_id].id);
> +		pr_err("CLK_Disable: CLK %s, CLK dev id= %d is already
> +				disabled\n",
> +				SERVICES_Clks[clk_id].clk_name,
> +				SERVICES_Clks[clk_id].id);
>  		 return status;
>  	}
>  	if (clk_id == SERVICESCLK_ssi_ick)
> @@ -291,10 +289,10 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId
> clk_id)
>  		if (pClk) {
>  			clk_disable(pClk);
>  		} else {
> -		       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: "
> -				"failed to get CLK %s, CLK dev id = %d\n",
> -				SERVICES_Clks[clk_id].clk_name,
> -				SERVICES_Clks[clk_id].id);
> +			pr_err("CLK_Disable: failed to get CLK %s,
> +					CLK dev id = %d\n",
> +					SERVICES_Clks[clk_id].clk_name,
> +					SERVICES_Clks[clk_id].id);
>  			status = DSP_EFAIL;
>  		}
>  	return status;
> diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c
> b/drivers/dsp/bridge/wmd/tiomap3430.c
> index 7fa6f8e..606de3c 100644
> --- a/drivers/dsp/bridge/wmd/tiomap3430.c
> +++ b/drivers/dsp/bridge/wmd/tiomap3430.c
> @@ -2119,7 +2119,6 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32
> cm_base,
>  {
>  	u32 temp;
>  	DSP_STATUS status = DSP_SOK;
> -	DSP_STATUS clk_status = DSP_SOK;
>  	enum HW_PwrState_t    pwrState;
> 
>  	/* Read PM_PWSTST_IVA2 */
> @@ -2134,11 +2133,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32
> cm_base,
>  		/* Wait until the state has moved to ON */
>  		HW_PWR_IVA2StateGet(prm_base, HW_PWR_DOMAIN_DSP, &pwrState);
>  	}
> -	clk_status = CLK_Disable(SERVICESCLK_iva2_ck);
> -	if (DSP_FAILED(clk_status)) {
> -		DBG_Trace(DBG_LEVEL6, "CLK_Disbale failed for clk = 0x%x \n",
> -			  SERVICESCLK_iva2_ck);
> -	}
> +	CLK_Disable(SERVICESCLK_iva2_ck);
>  	udelay(10);
>  	/* Assert IVA2-RST1 and IVA2-RST2  */
>  	*((REG_UWORD32 *)((u32)(prm_base) + 0x50)) = (u32)0x07;
> @@ -2155,11 +2150,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32
> cm_base,
>                 temp =  (temp & 0xFFFFFC8) | 0x37;
>                 *((REG_UWORD32 *) ((u32) (cm_base) + 0x4))
>                         (u32) temp;
> -	clk_status = CLK_Enable(SERVICESCLK_iva2_ck);
> -	if (DSP_FAILED(clk_status)) {
> -		DBG_Trace(DBG_LEVEL6, "CLK_Enable failed for clk = 0x%x \n",
> -			  SERVICESCLK_iva2_ck);
> -	}
> +	CLK_Enable(SERVICESCLK_iva2_ck);
>  	udelay(20);
>  	GetHWRegs(prm_base, cm_base);
>  	/* Release Reset1 and Reset2 */
> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> index 488a512..1ad1565 100644
> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> @@ -285,7 +285,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT
> *pDevContext, IN void *pArgs)
>  #ifdef CONFIG_PM
>  	struct CFG_HOSTRES resources;
>  	enum HW_PwrState_t pwrState;
> -       u32 temp;
> +	u32 temp;
> 
>  	status = CFG_GetHostResources(
>  		 (struct CFG_DEVNODE *)DRV_GetFirstDevExtension(),
> &resources);
> @@ -306,7 +306,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT
> *pDevContext, IN void *pArgs)
>  		 pDevContext->uDspPerClks);
>  	status = DSP_PeripheralClocks_Enable(pDevContext, NULL);
> 
> -       /* Enablifg Dppll in lock mode*/
> +	/* Enabling Dppll in lock mode */
>                 temp = (u32) *((REG_UWORD32 *)
>                         ((u32) (resources.dwCmBase) + 0x34));
>                 temp = (temp & 0xFFFFFFFE) | 0x1;
> @@ -546,27 +546,17 @@ DSP_STATUS DSP_PeripheralClocks_Enable(struct
> WMD_DEV_CONTEXT *pDevContext,
>  				      IN void *pArgs)
>  {
>  	u32 clkIdx;
> -	DSP_STATUS status = DSP_SOK;
> +	DSP_STATUS int_clk_status = DSP_EFAIL, fun_clk_status = DSP_EFAIL;
> 
>  	for (clkIdx = 0; clkIdx < MBX_PM_MAX_RESOURCES; clkIdx++) {
>  		if (((pDevContext->uDspPerClks) >> clkIdx) & 0x01) {
>  			/* Enable the interface clock of the peripheral */
> -			status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
> -			if (DSP_FAILED(status)) {
> -				DBG_Trace(DBG_LEVEL7,
> -					 "Failed to Enable the DSP Peripheral"
> -					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
> -			}
> +			int_clk_status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
>  			/* Enable the functional clock of the periphearl */
> -			status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
> -			if (DSP_FAILED(status)) {
> -				DBG_Trace(DBG_LEVEL7,
> -					 "Failed to Enable the DSP Peripheral"
> -					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
> -			}
> +			fun_clk_status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
>  		}
>  	}
> -	return status;
> +	return ((int_clk_status | fun_clk_status) != DSP_OK) ? DSP_EFAIL :
> DSP_OK;
>  }
> 
>  void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
> --
> 1.6.2.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Artem Bityutskiy]

Ameya Palande wrote:
> -	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: CLK %s, "
> -			"CLK dev id= %d is already disabled\n",
> -			SERVICES_Clks[clk_id].clk_name,
> -			SERVICES_Clks[clk_id].id);
> +		pr_err("CLK_Disable: CLK %s, CLK dev id= %d is already
> +				disabled\n",
> +				SERVICES_Clks[clk_id].clk_name,
> +				SERVICES_Clks[clk_id].id);

NACK.

pr_err("A"
       "B");

not

pr_err("A
        B");

please.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Ameya Palande]

From: Ameya Palande <ameya.palande@nokia.com>

This patch does following things:
1. Instead of GT_2trace() use pr_err(), since failure to enable or disable a
   clock is error which should be notified.
2. There is no need to check the return value of CLK_Enable and CLK_Disable
   and print error message, since these functions internally print the error.
3. Indentation changes and a typo fix.

Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
Acked-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 drivers/dsp/bridge/services/clk.c       |   42 ++++++++++++++----------------
 drivers/dsp/bridge/wmd/tiomap3430.c     |   13 +--------
 drivers/dsp/bridge/wmd/tiomap3430_pwr.c |   23 +++++-----------
 3 files changed, 29 insertions(+), 49 deletions(-)

diff --git a/drivers/dsp/bridge/services/clk.c b/drivers/dsp/bridge/services/clk.c
index 440706f..b45603f 100644
--- a/drivers/dsp/bridge/services/clk.c
+++ b/drivers/dsp/bridge/services/clk.c
@@ -188,7 +188,7 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
 	struct clk *pClk;
 
 	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
-       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
+	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
 		"CLK dev id = %d\n", SERVICES_Clks[clk_id].clk_name,
 		SERVICES_Clks[clk_id].id);
 
@@ -197,18 +197,16 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
 		if (clk_enable(pClk) == 0x0) {
 			/* Success ? */
 		} else {
-		       GT_2trace(CLK_debugMask, GT_7CLASS,
-				 "CLK_Enable: failed to Enable CLK %s, "
-				 "CLK dev id = %d\n",
-				 SERVICES_Clks[clk_id].clk_name,
-				 SERVICES_Clks[clk_id].id);
+			pr_err("CLK_Enable: failed to Enable CLK %s, "
+					"CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 			status = DSP_EFAIL;
 		}
 	} else {
-	       GT_2trace(CLK_debugMask, GT_7CLASS,
-			 "CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
-			 SERVICES_Clks[clk_id].clk_name,
-			 SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 		status = DSP_EFAIL;
 	}
 	/* The SSI module need to configured not to have the Forced idle for
@@ -274,15 +272,15 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 
 	clkUseCnt = CLK_Get_UseCnt(clk_id);
 	if (clkUseCnt == -1) {
-	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: failed to "
-			"get CLK Use count for CLK %s, CLK dev id = %d\n",
-			SERVICES_Clks[clk_id].clk_name,
-			SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Disable: failed to get CLK Use count for CLK %s,"
+				"CLK dev id = %d\n",
+				SERVICES_Clks[clk_id].clk_name,
+				SERVICES_Clks[clk_id].id);
 	} else if (clkUseCnt == 0) {
-	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: CLK %s, "
-			"CLK dev id= %d is already disabled\n",
-			SERVICES_Clks[clk_id].clk_name,
-			SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Disable: CLK %s, CLK dev id= %d is already"
+				"disabled\n",
+				SERVICES_Clks[clk_id].clk_name,
+				SERVICES_Clks[clk_id].id);
 		 return status;
 	}
 	if (clk_id == SERVICESCLK_ssi_ick)
@@ -291,10 +289,10 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 		if (pClk) {
 			clk_disable(pClk);
 		} else {
-		       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: "
-				"failed to get CLK %s, CLK dev id = %d\n",
-				SERVICES_Clks[clk_id].clk_name,
-				SERVICES_Clks[clk_id].id);
+			pr_err("CLK_Disable: failed to get CLK %s,"
+					"CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 			status = DSP_EFAIL;
 		}
 	return status;
diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c b/drivers/dsp/bridge/wmd/tiomap3430.c
index df350c6..fb71e96 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430.c
@@ -2240,7 +2240,6 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
 {
 	u32 temp;
 	DSP_STATUS status = DSP_SOK;
-	DSP_STATUS clk_status = DSP_SOK;
 	enum HW_PwrState_t    pwrState;
 
 	/* Read PM_PWSTST_IVA2 */
@@ -2255,11 +2254,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
 		/* Wait until the state has moved to ON */
 		HW_PWR_IVA2StateGet(prm_base, HW_PWR_DOMAIN_DSP, &pwrState);
 	}
-	clk_status = CLK_Disable(SERVICESCLK_iva2_ck);
-	if (DSP_FAILED(clk_status)) {
-		DBG_Trace(DBG_LEVEL6, "CLK_Disbale failed for clk = 0x%x \n",
-			  SERVICESCLK_iva2_ck);
-	}
+	CLK_Disable(SERVICESCLK_iva2_ck);
 	udelay(10);
 	/* Assert IVA2-RST1 and IVA2-RST2  */
 	*((REG_UWORD32 *)((u32)(prm_base) + 0x50)) = (u32)0x07;
@@ -2276,11 +2271,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
                temp =  (temp & 0xFFFFFC8) | 0x37;
                *((REG_UWORD32 *) ((u32) (cm_base) + 0x4)) =
                        (u32) temp;
-	clk_status = CLK_Enable(SERVICESCLK_iva2_ck);
-	if (DSP_FAILED(clk_status)) {
-		DBG_Trace(DBG_LEVEL6, "CLK_Enable failed for clk = 0x%x \n",
-			  SERVICESCLK_iva2_ck);
-	}
+	CLK_Enable(SERVICESCLK_iva2_ck);
 	udelay(20);
 	GetHWRegs(prm_base, cm_base);
 	/* Release Reset1 and Reset2 */
diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
index 488a512..ffe2e3c 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
@@ -285,7 +285,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
 #ifdef CONFIG_PM
 	struct CFG_HOSTRES resources;
 	enum HW_PwrState_t pwrState;
-       u32 temp;
+	u32 temp;
 
 	status = CFG_GetHostResources(
 		 (struct CFG_DEVNODE *)DRV_GetFirstDevExtension(), &resources);
@@ -306,7 +306,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
 		 pDevContext->uDspPerClks);
 	status = DSP_PeripheralClocks_Enable(pDevContext, NULL);
 
-       /* Enablifg Dppll in lock mode*/
+	/* Enabling Dppll in lock mode */
                temp = (u32) *((REG_UWORD32 *)
                        ((u32) (resources.dwCmBase) + 0x34));
                temp = (temp & 0xFFFFFFFE) | 0x1;
@@ -546,27 +546,18 @@ DSP_STATUS DSP_PeripheralClocks_Enable(struct WMD_DEV_CONTEXT *pDevContext,
 				      IN void *pArgs)
 {
 	u32 clkIdx;
-	DSP_STATUS status = DSP_SOK;
+	DSP_STATUS int_clk_status = DSP_EFAIL, fun_clk_status = DSP_EFAIL;
 
 	for (clkIdx = 0; clkIdx < MBX_PM_MAX_RESOURCES; clkIdx++) {
 		if (((pDevContext->uDspPerClks) >> clkIdx) & 0x01) {
 			/* Enable the interface clock of the peripheral */
-			status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
-			if (DSP_FAILED(status)) {
-				DBG_Trace(DBG_LEVEL7,
-					 "Failed to Enable the DSP Peripheral"
-					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
-			}
+			int_clk_status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
 			/* Enable the functional clock of the periphearl */
-			status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
-			if (DSP_FAILED(status)) {
-				DBG_Trace(DBG_LEVEL7,
-					 "Failed to Enable the DSP Peripheral"
-					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
-			}
+			fun_clk_status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
 		}
 	}
-	return status;
+	return ((int_clk_status | fun_clk_status) != DSP_SOK) ? DSP_EFAIL :
+								DSP_SOK;
 }
 
 void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
-- 
1.6.2.2

Re: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Artem Bityutskiy]

Ameya Palande wrote:
>  	}
> -	return status;
> +	return ((int_clk_status | fun_clk_status) != DSP_SOK) ? DSP_EFAIL :
> +								DSP_SOK;
>  }

Side note:

Is it possible to rework the code and remove thse insane
DSP_EFAIL and DSP_SOK? Wouldn't it be possible to use
the good practices we have in kernel? Namely,

1. Return 0 on success.
2. Return negative error code on failure? 

This is C, not visual basic, after all...

P.S. Ameya, do not take this as a personal attack. The
code is just so ugly and embarrassing, that I cannot stop
flaming :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Artem Bityutskiy]

I'm sorry. Authors of the code may delete this e-mail.
More flame and suggestions.

Ameya Palande wrote:
> diff --git a/drivers/dsp/bridge/services/clk.c b/drivers/dsp/bridge/services/clk.c
> index 440706f..b45603f 100644
> --- a/drivers/dsp/bridge/services/clk.c
> +++ b/drivers/dsp/bridge/services/clk.c
> @@ -188,7 +188,7 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
>  	struct clk *pClk;
>  
>  	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
> -       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
> +	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
>  		"CLK dev id = %d\n", SERVICES_Clks[clk_id].clk_name,
>  		SERVICES_Clks[clk_id].id);

Is it possible to get rid of this crap? I mean trace.
First of all, if you pretend your code is production-ready,
you should not need that much of this. Second of all, we
have tools like ftrace.

>  
> @@ -197,18 +197,16 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
>  		if (clk_enable(pClk) == 0x0) {
>  			/* Success ? */
>  		} else {
> -		       GT_2trace(CLK_debugMask, GT_7CLASS,
> -				 "CLK_Enable: failed to Enable CLK %s, "
> -				 "CLK dev id = %d\n",
> -				 SERVICES_Clks[clk_id].clk_name,
> -				 SERVICES_Clks[clk_id].id);
> +			pr_err("CLK_Enable: failed to Enable CLK %s, "
> +					"CLK dev id = %d\n",
> +					SERVICES_Clks[clk_id].clk_name,
> +					SERVICES_Clks[clk_id].id);

Is it possible to wake up, and realize you are writing
linux kernel code, and realize you should socialize?
Namely, these marginal naming conventions you use are
absolutely anti-social. DSPBRIGE is around for quite a
time already, its time to start socializing.

>  	} else if (clkUseCnt == 0) {
> -	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: CLK %s, "
> -			"CLK dev id= %d is already disabled\n",
> -			SERVICES_Clks[clk_id].clk_name,
> -			SERVICES_Clks[clk_id].id);
> +		pr_err("CLK_Disable: CLK %s, CLK dev id= %d is already"
> +				"disabled\n",
> +				SERVICES_Clks[clk_id].clk_name,
> +				SERVICES_Clks[clk_id].id);
>  		 return status;
>  	}
>  	if (clk_id == SERVICESCLK_ssi_ick)
> @@ -291,10 +289,10 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)

Is it possible to stop using excessive enums? This is not
C++, and C does not do any type-checking. Named enums
make almost zero sense.

> diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c b/drivers/dsp/bridge/wmd/tiomap3430.c
> index df350c6..fb71e96 100644
> --- a/drivers/dsp/bridge/wmd/tiomap3430.c
> +++ b/drivers/dsp/bridge/wmd/tiomap3430.c
> @@ -2240,7 +2240,6 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
>  {
>  	u32 temp;
>  	DSP_STATUS status = DSP_SOK;
> -	DSP_STATUS clk_status = DSP_SOK;

Is it possible to remove these insane types. Use normal types.

>  	enum HW_PwrState_t    pwrState;

Similar. What is the point of enums like this? Obfuscate
the code?

>  	udelay(20);
>  	GetHWRegs(prm_base, cm_base);
>  	/* Release Reset1 and Reset2 */
> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> index 488a512..ffe2e3c 100644
> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
> @@ -285,7 +285,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
>  #ifdef CONFIG_PM
>  	struct CFG_HOSTRES resources;
>  	enum HW_PwrState_t pwrState;
> -       u32 temp;
> +	u32 temp;

I'm not 100% sure about this particular case, but overall,
the code excessively uses these u32 types. This makse few
sense. Unless you work with registers, packets, or something
which needs some strict types, use int or long. Use natural
C types. Do not try to be too smart, do not try to limit
CPU and compiler by u32, if it is not necessary.


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH][Take 2] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Ameya Palande]

This patch does following things:
1. Instead of GT_2trace() use pr_err(), since failure to enable or disable a
   clock is error which should be notified.
2. There is no need to check the return value of CLK_Enable and CLK_Disable
   and print error message, since these functions internally print the error.
3. Indentation changes and a typo fix.

Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
Acked-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 drivers/dsp/bridge/services/clk.c       |   61 +++++++++++++++----------------
 drivers/dsp/bridge/wmd/tiomap3430.c     |   13 +------
 drivers/dsp/bridge/wmd/tiomap3430_pwr.c |   24 ++++--------
 3 files changed, 40 insertions(+), 58 deletions(-)

diff --git a/drivers/dsp/bridge/services/clk.c b/drivers/dsp/bridge/services/clk.c
index 440706f..b499b14 100644
--- a/drivers/dsp/bridge/services/clk.c
+++ b/drivers/dsp/bridge/services/clk.c
@@ -188,7 +188,7 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
 	struct clk *pClk;
 
 	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
-       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
+	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Enable: CLK %s, "
 		"CLK dev id = %d\n", SERVICES_Clks[clk_id].clk_name,
 		SERVICES_Clks[clk_id].id);
 
@@ -197,18 +197,16 @@ DSP_STATUS CLK_Enable(IN enum SERVICES_ClkId clk_id)
 		if (clk_enable(pClk) == 0x0) {
 			/* Success ? */
 		} else {
-		       GT_2trace(CLK_debugMask, GT_7CLASS,
-				 "CLK_Enable: failed to Enable CLK %s, "
-				 "CLK dev id = %d\n",
-				 SERVICES_Clks[clk_id].clk_name,
-				 SERVICES_Clks[clk_id].id);
+			pr_err("CLK_Enable: failed to Enable CLK %s, "
+					"CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 			status = DSP_EFAIL;
 		}
 	} else {
-	       GT_2trace(CLK_debugMask, GT_7CLASS,
-			 "CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
-			 SERVICES_Clks[clk_id].clk_name,
-			 SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Enable: failed to get CLK %s, CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 		status = DSP_EFAIL;
 	}
 	/* The SSI module need to configured not to have the Forced idle for
@@ -236,7 +234,7 @@ DSP_STATUS CLK_Set_32KHz(IN enum SERVICES_ClkId clk_id)
 	pClkParent =  SERVICES_Clks[sys_32k_id].clk_handle;
 
 	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
-       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Set_32KHz: CLK %s, "
+	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Set_32KHz: CLK %s, "
 		"CLK dev id = %d is setting to 32KHz \n",
 		SERVICES_Clks[clk_id].clk_name,
 		SERVICES_Clks[clk_id].id);
@@ -266,7 +264,7 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 	s32 clkUseCnt;
 
 	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
-       GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Disable: CLK %s, "
+	GT_2trace(CLK_debugMask, GT_6CLASS, "CLK_Disable: CLK %s, "
 		"CLK dev id = %d\n", SERVICES_Clks[clk_id].clk_name,
 		SERVICES_Clks[clk_id].id);
 
@@ -274,16 +272,16 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 
 	clkUseCnt = CLK_Get_UseCnt(clk_id);
 	if (clkUseCnt == -1) {
-	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: failed to "
-			"get CLK Use count for CLK %s, CLK dev id = %d\n",
-			SERVICES_Clks[clk_id].clk_name,
-			SERVICES_Clks[clk_id].id);
+		pr_err("CLK_Disable: failed to get CLK Use count for CLK %s,"
+				"CLK dev id = %d\n",
+				SERVICES_Clks[clk_id].clk_name,
+				SERVICES_Clks[clk_id].id);
 	} else if (clkUseCnt == 0) {
-	       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: CLK %s, "
-			"CLK dev id= %d is already disabled\n",
-			SERVICES_Clks[clk_id].clk_name,
-			SERVICES_Clks[clk_id].id);
-		 return status;
+		pr_err("CLK_Disable: CLK %s, CLK dev id= %d is already"
+				"disabled\n",
+				SERVICES_Clks[clk_id].clk_name,
+				SERVICES_Clks[clk_id].id);
+		return status;
 	}
 	if (clk_id == SERVICESCLK_ssi_ick)
 		SSI_Clk_Prepare(false);
@@ -291,10 +289,10 @@ DSP_STATUS CLK_Disable(IN enum SERVICES_ClkId clk_id)
 		if (pClk) {
 			clk_disable(pClk);
 		} else {
-		       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_Disable: "
-				"failed to get CLK %s, CLK dev id = %d\n",
-				SERVICES_Clks[clk_id].clk_name,
-				SERVICES_Clks[clk_id].id);
+			pr_err("CLK_Disable: failed to get CLK %s,"
+					"CLK dev id = %d\n",
+					SERVICES_Clks[clk_id].clk_name,
+					SERVICES_Clks[clk_id].id);
 			status = DSP_EFAIL;
 		}
 	return status;
@@ -316,7 +314,7 @@ DSP_STATUS CLK_GetRate(IN enum SERVICES_ClkId clk_id, u32 *speedKhz)
 	DBC_Require(clk_id < SERVICESCLK_NOT_DEFINED);
 	*speedKhz = 0x0;
 
-       GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_GetRate: CLK %s, "
+	GT_2trace(CLK_debugMask, GT_7CLASS, "CLK_GetRate: CLK %s, "
 		"CLK dev Id = %d \n", SERVICES_Clks[clk_id].clk_name,
 		SERVICES_Clks[clk_id].id);
 	pClk = SERVICES_Clks[clk_id].clk_handle;
@@ -327,7 +325,7 @@ DSP_STATUS CLK_GetRate(IN enum SERVICES_ClkId clk_id, u32 *speedKhz)
 			  "CLK_GetRate: clkSpeedHz = %d , "
 			 "speedinKhz=%d\n", clkSpeedHz, *speedKhz);
 	} else {
-	       GT_2trace(CLK_debugMask, GT_7CLASS,
+		GT_2trace(CLK_debugMask, GT_7CLASS,
 			 "CLK_GetRate: failed to get CLK %s, "
 			 "CLK dev Id = %d\n", SERVICES_Clks[clk_id].clk_name,
 			 SERVICES_Clks[clk_id].id);
@@ -348,7 +346,7 @@ s32 CLK_Get_UseCnt(IN enum SERVICES_ClkId clk_id)
 	if (pClk) {
 		useCount =  pClk->usecount; /* FIXME: usecount shouldn't be used */
 	} else {
-	       GT_2trace(CLK_debugMask, GT_7CLASS,
+		GT_2trace(CLK_debugMask, GT_7CLASS,
 			 "CLK_GetRate: failed to get CLK %s, "
 			 "CLK dev Id = %d\n", SERVICES_Clks[clk_id].clk_name,
 			 SERVICES_Clks[clk_id].id);
@@ -362,14 +360,15 @@ void SSI_Clk_Prepare(bool FLAG)
 	u32 ssi_sysconfig;
 	ssi_sysconfig = __raw_readl((SSI_BASE) + 0x10);
 
-
 	if (FLAG) {
 		/* Set Autoidle, SIDLEMode to smart idle, and MIDLEmode to
-		 * no idle */
+		 * no idle
+		 */
 		ssi_sysconfig = 0x1011;
 	} else {
 		/* Set Autoidle, SIDLEMode to forced idle, and MIDLEmode to
-		 * forced idle*/
+		 * forced idle
+		 */
 		ssi_sysconfig = 0x1;
 	}
 	__raw_writel((u32)ssi_sysconfig, SSI_BASE + 0x10);
diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c b/drivers/dsp/bridge/wmd/tiomap3430.c
index df350c6..fb71e96 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430.c
@@ -2240,7 +2240,6 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
 {
 	u32 temp;
 	DSP_STATUS status = DSP_SOK;
-	DSP_STATUS clk_status = DSP_SOK;
 	enum HW_PwrState_t    pwrState;
 
 	/* Read PM_PWSTST_IVA2 */
@@ -2255,11 +2254,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
 		/* Wait until the state has moved to ON */
 		HW_PWR_IVA2StateGet(prm_base, HW_PWR_DOMAIN_DSP, &pwrState);
 	}
-	clk_status = CLK_Disable(SERVICESCLK_iva2_ck);
-	if (DSP_FAILED(clk_status)) {
-		DBG_Trace(DBG_LEVEL6, "CLK_Disbale failed for clk = 0x%x \n",
-			  SERVICESCLK_iva2_ck);
-	}
+	CLK_Disable(SERVICESCLK_iva2_ck);
 	udelay(10);
 	/* Assert IVA2-RST1 and IVA2-RST2  */
 	*((REG_UWORD32 *)((u32)(prm_base) + 0x50)) = (u32)0x07;
@@ -2276,11 +2271,7 @@ static DSP_STATUS run_IdleBoot(u32 prm_base, u32 cm_base,
                temp =  (temp & 0xFFFFFC8) | 0x37;
                *((REG_UWORD32 *) ((u32) (cm_base) + 0x4)) =
                        (u32) temp;
-	clk_status = CLK_Enable(SERVICESCLK_iva2_ck);
-	if (DSP_FAILED(clk_status)) {
-		DBG_Trace(DBG_LEVEL6, "CLK_Enable failed for clk = 0x%x \n",
-			  SERVICESCLK_iva2_ck);
-	}
+	CLK_Enable(SERVICESCLK_iva2_ck);
 	udelay(20);
 	GetHWRegs(prm_base, cm_base);
 	/* Release Reset1 and Reset2 */
diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
index 488a512..68d5dff 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
@@ -285,7 +285,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
 #ifdef CONFIG_PM
 	struct CFG_HOSTRES resources;
 	enum HW_PwrState_t pwrState;
-       u32 temp;
+	u32 temp;
 
 	status = CFG_GetHostResources(
 		 (struct CFG_DEVNODE *)DRV_GetFirstDevExtension(), &resources);
@@ -306,7 +306,7 @@ DSP_STATUS WakeDSP(struct WMD_DEV_CONTEXT *pDevContext, IN void *pArgs)
 		 pDevContext->uDspPerClks);
 	status = DSP_PeripheralClocks_Enable(pDevContext, NULL);
 
-       /* Enablifg Dppll in lock mode*/
+	/* Enabling Dppll in lock mode */
                temp = (u32) *((REG_UWORD32 *)
                        ((u32) (resources.dwCmBase) + 0x34));
                temp = (temp & 0xFFFFFFFE) | 0x1;
@@ -546,27 +546,19 @@ DSP_STATUS DSP_PeripheralClocks_Enable(struct WMD_DEV_CONTEXT *pDevContext,
 				      IN void *pArgs)
 {
 	u32 clkIdx;
-	DSP_STATUS status = DSP_SOK;
+	DSP_STATUS int_clk_status = DSP_EFAIL, fun_clk_status = DSP_EFAIL;
 
 	for (clkIdx = 0; clkIdx < MBX_PM_MAX_RESOURCES; clkIdx++) {
 		if (((pDevContext->uDspPerClks) >> clkIdx) & 0x01) {
 			/* Enable the interface clock of the peripheral */
-			status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
-			if (DSP_FAILED(status)) {
-				DBG_Trace(DBG_LEVEL7,
-					 "Failed to Enable the DSP Peripheral"
-					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
-			}
+			int_clk_status = CLK_Enable(BPWR_Clks[clkIdx].intClk);
 			/* Enable the functional clock of the periphearl */
-			status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
-			if (DSP_FAILED(status)) {
-				DBG_Trace(DBG_LEVEL7,
-					 "Failed to Enable the DSP Peripheral"
-					 "Clk 0x%x \n", BPWR_Clks[clkIdx]);
-			}
+			fun_clk_status = CLK_Enable(BPWR_Clks[clkIdx].funClk);
 		}
 	}
-	return status;
+	if ((int_clk_status | fun_clk_status) != DSP_SOK)
+		return DSP_EFAIL;
+	return DSP_SOK;
 }
 
 void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
-- 
1.6.2.2

RE: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Ramirez Luna, Omar]

Hi,

Artem Bityutskiy wrote:
>
>I'm sorry. Authors of the code may delete this e-mail.
>More flame and suggestions.
>

Thanks for your suggestions; it is true that bridge code has been out for a while, almost a year I think, and we are still trying to enhance it and clean it up along the way, although it may seem we have stalled on the task.

Please friendly flame about other parts of bridge code too ;) as we can take very good points of your's/everyone's comments and try to put in better shape the driver to be pushed into mainline (as this has been the goal all the time). Thanks for the time reviewing the code.

- omar ramirez

RE: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Kanigeri, Hari]

> Artem Bityutskiy wrote:
> >
> >I'm sorry. Authors of the code may delete this e-mail.
> >More flame and suggestions.
> >
> 
> Thanks for your suggestions; it is true that bridge code has been out for
> a while, almost a year I think, and we are still trying to enhance it and
> clean it up along the way, although it may seem we have stalled on the
> task.
> 
> Please friendly flame about other parts of bridge code too ;) as we can
> take very good points of your's/everyone's comments and try to put in
> better shape the driver to be pushed into mainline (as this has been the
> goal all the time). Thanks for the time reviewing the code.
> 
-- Good point, Omar. Artem, please continue sending your comments so that we can continue to improve this code. Progressing from the stage where the Bridge code used to be delivered in the form of tarballs a year ago to delivering it on GIT tree where the patches can be applied was only possible because of comments/suggestions/flames that were received on this mailing list.  

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Ramirez Luna, Omar
> Sent: Friday, April 17, 2009 12:45 PM
> To: Artem Bityutskiy; Ameya Palande
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code
> cleanup
> 
> Hi,
> 
> Artem Bityutskiy wrote:
> >
> >I'm sorry. Authors of the code may delete this e-mail.
> >More flame and suggestions.
> >
> 
> Thanks for your suggestions; it is true that bridge code has been out for
> a while, almost a year I think, and we are still trying to enhance it and
> clean it up along the way, although it may seem we have stalled on the
> task.
> 
> Please friendly flame about other parts of bridge code too ;) as we can
> take very good points of your's/everyone's comments and try to put in
> better shape the driver to be pushed into mainline (as this has been the
> goal all the time). Thanks for the time reviewing the code.
> 
> - omar ramirez
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Felipe Contreras]

On Fri, Apr 17, 2009 at 8:45 PM, Ramirez Luna, Omar <x00omar@ti.com> wrote:
> Hi,
>
> Artem Bityutskiy wrote:
>>
>>I'm sorry. Authors of the code may delete this e-mail.
>>More flame and suggestions.
>>
>
> Thanks for your suggestions; it is true that bridge code has been out for a while, almost a year I think, and we are still trying to enhance it and clean it up along the way, although it may seem we have stalled on the task.
>
> Please friendly flame about other parts of bridge code too ;) as we can take very good points of your's/everyone's comments and try to put in better shape the driver to be pushed into mainline (as this has been the goal all the time). Thanks for the time reviewing the code.

The purpose of this mailing list is not to make friends, it's to
develop good quality software. You might not like this "unfriendly"
approach, but it is the style of many linux maintainers, and I believe
for good reason.

When you think the code is ugly, and you want the message to get
across effectively you say it's ugly. This might hurt some people
sensitivities but it's the truth, and sugar-coating will not make the
code any less uglier.

While there has been good progress in the bridgedriver, it is still
*very* far away from meeting mainline standards and this makes many
people uneasy, including me.

Artem complains will need to be addressed one way or another. So, can
we have at least have a TODO file with these complaints?

-- 
Felipe Contreras

Re: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Nishanth Menon]

Felipe Contreras said the following on 04/25/2009 06:42 AM:
> While there has been good progress in the bridgedriver, it is still
> *very* far away from meeting mainline standards and this makes many
> people uneasy, including me.
>   
me too.. :)
> Artem complains will need to be addressed one way or another. So, can
> we have at least have a TODO file with these complaints?
>
>   
;) Acked-by: Nishanth
Regards,
Nishanth Menon

RE: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Ramirez Luna, Omar]

Felipe Contreras wrote:
>Fri, Apr 17, 2009 at 8:45 PM, Ramirez Luna, Omar <x00omar@ti.com> wrote:
>> Artem Bityutskiy wrote:
>>>
>>>I'm sorry. Authors of the code may delete this e-mail.
>>>More flame and suggestions.
>>
>> Thanks for your suggestions; it is true that bridge code has been out for a while, almost a year I think, and we are still trying to enhance it and clean it up along the way, although it may seem we have stalled on the task.
>>
>> Please friendly flame about other parts of bridge code too ;) as we can take very good points of your's/everyone's comments and try to put in better shape the driver to be pushed into mainline (as this has been the goal all the time). Thanks for the time reviewing the code.

>The purpose of this mailing list is not to make friends, it's to
>develop good quality software. You might not like this "unfriendly"
>approach, but it is the style of many linux maintainers, and I believe
>for good reason.

mmm I think I should put a sarcastic flag just after my comment to be understand... anyway a discussion about the nature of the list really doesn't matter.

>Artem complains will need to be addressed one way or another. So, can
>we have at least have a TODO file with these complaints?

I agree on this one, I'll start another thread with what I have so far and anyone is welcomed to*friendly flame* or whatever on there.

- omar ramirez

Re: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Koen Kooi]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

Op 25 apr 2009, om 13:42 heeft Felipe Contreras het volgende geschreven:

> While there has been good progress in the bridgedriver, it is still
> *very* far away from meeting mainline standards and this makes many
> people uneasy, including me.


Is it "good enough" to go into Greg KHs staging tree?

regards,

Koen


[-- Attachment #2: Dit deel van het bericht is digitaal ondertekend --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH][UPDATED] DSPBRIDGE: CLK_Enable and CLK_Disable Code cleanup

[Tony Lindgren]

* Koen Kooi <k.kooi@student.utwente.nl> [090425 07:48]:
> Op 25 apr 2009, om 13:42 heeft Felipe Contreras het volgende geschreven:
>
>> While there has been good progress in the bridgedriver, it is still
>> *very* far away from meeting mainline standards and this makes many
>> people uneasy, including me.
>
>
> Is it "good enough" to go into Greg KHs staging tree?

I'd rather keep any code that does non-standard tinkering
of omap registers out of the mainline tree until the basics are
fixed.

Regards,

Tony