Re: [PATCH 4/5] staging:ti dspbridge: remove unnecessary volatile variables

[Nishanth Menon <nm@ti.com>]

Ramos Falcon, Ernesto had written, on 07/28/2010 09:40 AM, the following:
> Remove unnecessary volatile variables; use accessor
> functions __raw_readl/__raw_writel instead when applicable.
> 
> Signed-off-by: Ernesto Ramos <ernesto@ti.com>
> ---
>  drivers/staging/tidspbridge/core/tiomap3430.c |    8 ++++----
>  drivers/staging/tidspbridge/dynload/tramp.c   |    4 ++--
>  drivers/staging/tidspbridge/pmgr/cmm.c        |    7 ++++---
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
> index 08a2f5f..ae1f394 100644
> --- a/drivers/staging/tidspbridge/core/tiomap3430.c
> +++ b/drivers/staging/tidspbridge/core/tiomap3430.c
> @@ -404,7 +404,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
>  		pr_err("%s: Illegal SM base\n", __func__);
>  		status = -EPERM;
>  	} else
> -		*((volatile u32 *)dw_sync_addr) = 0xffffffff;
> +		__raw_writel(0xffffffff, dw_sync_addr);
curious question: any reason not to use writel instead of __raw_writel?

>  
>  	if (DSP_SUCCEEDED(status)) {
>  		resources = dev_context->resources;
> @@ -584,7 +584,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
>  		dev_dbg(bridge, "Waiting for Sync @ 0x%x\n", dw_sync_addr);
>  		dev_dbg(bridge, "DSP c_int00 Address =  0x%x\n", dsp_addr);
>  		if (dsp_debug)
> -			while (*((volatile u16 *)dw_sync_addr))
> +			while (__raw_readw(dw_sync_addr))
>  				;;

may be for a later on patch -> we seem to have a potential infinite loop 
here..

>  
>  		/* Wait for DSP to clear word in shared memory */
> @@ -602,7 +602,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
>  			/* Write the synchronization bit to indicate the
>  			 * completion of OPP table update to DSP
>  			 */
> -			*((volatile u32 *)dw_sync_addr) = 0XCAFECAFE;
> +			__raw_writel(0XCAFECAFE, dw_sync_addr);
>  
>  			/* update board state */
>  			dev_context->dw_brd_state = BRD_RUNNING;
> @@ -1852,7 +1852,7 @@ bool wait_for_start(struct bridge_dev_context *dev_context, u32 dw_sync_addr)
>  	u16 timeout = TIHELEN_ACKTIMEOUT;
>  
>  	/*  Wait for response from board */
> -	while (*((volatile u16 *)dw_sync_addr) && --timeout)
> +	while (__raw_readw(dw_sync_addr) && --timeout)
>  		udelay(10);
>  
>  	/*  If timed out: return false */
> diff --git a/drivers/staging/tidspbridge/dynload/tramp.c b/drivers/staging/tidspbridge/dynload/tramp.c
> index 81314d2..60d22ea 100644
> --- a/drivers/staging/tidspbridge/dynload/tramp.c
> +++ b/drivers/staging/tidspbridge/dynload/tramp.c
> @@ -86,8 +86,8 @@ static u8 priv_h2a(u8 value)
>  static void priv_tramp_sym_gen_name(u32 value, char *dst)
>  {
>  	u32 i;
> -	volatile char *prefix = TRAMP_SYM_PREFIX;
> -	volatile char *dst_local = dst;
> +	char *prefix = TRAMP_SYM_PREFIX;
> +	char *dst_local = dst;
>  	u8 tmp;
>  
>  	/*  Clear out the destination, including the ending NULL */
> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c b/drivers/staging/tidspbridge/pmgr/cmm.c
> index 874ed64..b7cba1b 100644
> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
> @@ -1008,6 +1008,7 @@ void *cmm_xlator_alloc_buf(struct cmm_xlatorobject *xlator, void *va_buf,
>  {
>  	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
>  	void *pbuf = NULL;
> +	void *tmp_va_buff;
>  	struct cmm_attrs attrs;
>  
>  	DBC_REQUIRE(refs > 0);
> @@ -1019,16 +1020,16 @@ void *cmm_xlator_alloc_buf(struct cmm_xlatorobject *xlator, void *va_buf,
>  
>  	if (xlator_obj) {
>  		attrs.ul_seg_id = xlator_obj->ul_seg_id;
> -		*(volatile u32 *)va_buf = 0;
> +		__raw_writel(0, va_buf);
>  		/* Alloc SM */
>  		pbuf =
>  		    cmm_calloc_buf(xlator_obj->hcmm_mgr, pa_size, &attrs, NULL);
>  		if (pbuf) {
>  			/* convert to translator(node/strm) process Virtual
>  			 * address */
> -			*(volatile u32 **)va_buf =
> -			    (u32 *) cmm_xlator_translate(xlator,
> +			 tmp_va_buff = cmm_xlator_translate(xlator,
>  							 pbuf, CMM_PA2VA);
> +			__raw_writel((u32)tmp_va_buff, va_buf);

a) is the u32 cast of tmp_va_buff really necessary?
b) tmp_va_buff is used only in this branch, why not reduce the scope of 
the variable to just to this if branch
or an optional way could be:
--- a/drivers/staging/tidspbridge/pmgr/cmm.c
+++ b/drivers/staging/tidspbridge/pmgr/cmm.c
@@ -1022,18 +1022,17 @@ void *cmm_xlator_alloc_buf(struct 
cmm_xlatorobject *xlator, void *va_buf,
  	DBC_REQUIRE(xlator_obj->ul_seg_id > 0);

  	if (xlator_obj) {
+		void *t_buf = 0;
  		attrs.ul_seg_id = xlator_obj->ul_seg_id;
-		*(volatile u32 *)va_buf = 0;
  		/* Alloc SM */
  		pbuf =
  		    cmm_calloc_buf(xlator_obj->hcmm_mgr, pa_size, &attrs, NULL);
  		if (pbuf) {
  			/* convert to translator(node/strm) process Virtual
  			 * address */
-			*(volatile u32 **)va_buf =
-			    (u32 *) cmm_xlator_translate(xlator,
-							 pbuf, CMM_PA2VA);
+			t_buf = cmm_xlator_translate(xlator, pbuf, CMM_PA2VA);
  		}
+		writel(t_buf, va_buf);
  	}
  	return pbuf;
  }


>  		}
>  	}
>  	return pbuf;


-- 
Regards,
Nishanth Menon