[PATCH 0/5] staging:ti dspbridge: Checkpatch errors cleanup series

[PATCH 0/5] staging:ti dspbridge: Checkpatch errors cleanup series

[Ernesto Ramos]

These patches are targetted to remove checkpatch errors within
dspbridge driver.

Ernesto Ramos (5):
  staging:ti dspbridge: remove unused typedef REG16
  staging:ti dspbridge: remove unnecessary check for NULL pointer in
    cmm.c
  staging:ti dspbridge: remove function delete_strm_mgr
  staging:ti dspbridge: remove unnecessary volatile variables
  staging:ti dspbridge: replace simple_strtoul by strict_strtoul

 drivers/staging/tidspbridge/core/_tiomap.h    |    2 --
 drivers/staging/tidspbridge/core/tiomap3430.c |    8 ++++----
 drivers/staging/tidspbridge/dynload/tramp.c   |    4 ++--
 drivers/staging/tidspbridge/pmgr/cmm.c        |   15 ++++++---------
 drivers/staging/tidspbridge/rmgr/dbdcd.c      |    6 +++++-
 drivers/staging/tidspbridge/rmgr/strm.c       |   16 ++--------------
 6 files changed, 19 insertions(+), 32 deletions(-)

[PATCH 1/5] staging:ti dspbridge: remove unused typedef REG16

[Ernesto Ramos]

Remove unused typedef REG16 in dspbridge.

Signed-off-by: Ernesto Ramos <ernesto@ti.com>
---
 drivers/staging/tidspbridge/core/_tiomap.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/_tiomap.h b/drivers/staging/tidspbridge/core/_tiomap.h
index b754e49..1c1f157 100644
--- a/drivers/staging/tidspbridge/core/_tiomap.h
+++ b/drivers/staging/tidspbridge/core/_tiomap.h
@@ -295,8 +295,6 @@ static const struct bpwr_clk_t bpwr_clks[] = {
 #define LOW_LEVEL                       false
 
 /* Macro's */
-#define REG16(A)    (*(reg_uword16 *)(A))
-
 #define CLEAR_BIT(reg, mask)             (reg &= ~mask)
 #define SET_BIT(reg, mask)               (reg |= mask)
 
-- 
1.5.4.5

[PATCH 2/5] staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c

[Ernesto Ramos]

Remove unnecessary check for NULL pointer in cmm.c.

Signed-off-by: Ernesto Ramos <ernesto@ti.com>
---
 drivers/staging/tidspbridge/pmgr/cmm.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c b/drivers/staging/tidspbridge/pmgr/cmm.c
index 8e808d9..874ed64 100644
--- a/drivers/staging/tidspbridge/pmgr/cmm.c
+++ b/drivers/staging/tidspbridge/pmgr/cmm.c
@@ -992,16 +992,12 @@ int cmm_xlator_create(struct cmm_xlatorobject **xlator,
 int cmm_xlator_delete(struct cmm_xlatorobject *xlator, bool force)
 {
 	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
-	int status = 0;
 
 	DBC_REQUIRE(refs > 0);
 
-	if (xlator_obj)
-		kfree(xlator_obj);
-	else
-		status = -EFAULT;
+	kfree(xlator_obj);
 
-	return status;
+	return 0;
 }
 
 /*
-- 
1.5.4.5

[PATCH 3/5] staging:ti dspbridge: remove function delete_strm_mgr

[Ernesto Ramos]

Remove function delete_strm_mgr in strm.c and
use kfree instead.

Signed-off-by: Ernesto Ramos <ernesto@ti.com>
---
 drivers/staging/tidspbridge/rmgr/strm.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/strm.c b/drivers/staging/tidspbridge/rmgr/strm.c
index 73888e3..6c184c6 100644
--- a/drivers/staging/tidspbridge/rmgr/strm.c
+++ b/drivers/staging/tidspbridge/rmgr/strm.c
@@ -90,7 +90,6 @@ static u32 refs;		/* module reference count */
 
 /*  ----------------------------------- Function Prototypes */
 static int delete_strm(struct strm_object *stream_obj);
-static void delete_strm_mgr(struct strm_mgr *strm_mgr_obj);
 
 /*
  *  ======== strm_allocate_buffer ========
@@ -232,7 +231,7 @@ int strm_create(struct strm_mgr **strm_man,
 	if (DSP_SUCCEEDED(status))
 		*strm_man = strm_mgr_obj;
 	else
-		delete_strm_mgr(strm_mgr_obj);
+		kfree(strm_mgr_obj);
 
 	DBC_ENSURE((DSP_SUCCEEDED(status) && *strm_man) ||
 				(DSP_FAILED(status) && *strm_man == NULL));
@@ -250,7 +249,7 @@ void strm_delete(struct strm_mgr *strm_mgr_obj)
 	DBC_REQUIRE(refs > 0);
 	DBC_REQUIRE(strm_mgr_obj);
 
-	delete_strm_mgr(strm_mgr_obj);
+	kfree(strm_mgr_obj);
 }
 
 /*
@@ -859,14 +858,3 @@ static int delete_strm(struct strm_object *stream_obj)
 	}
 	return status;
 }
-
-/*
- *  ======== delete_strm_mgr ========
- *  Purpose:
- *      Frees stream manager.
- */
-static void delete_strm_mgr(struct strm_mgr *strm_mgr_obj)
-{
-	if (strm_mgr_obj)
-		kfree(strm_mgr_obj);
-}
-- 
1.5.4.5

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

[Ernesto Ramos]

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);
 
 	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))
 				;;
 
 		/* 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);
 		}
 	}
 	return pbuf;
-- 
1.5.4.5

[PATCH 5/5] staging:ti dspbridge: replace simple_strtoul by strict_strtoul

[Ernesto Ramos]

Replace simple_strtoul by strict_strtoul in atoi function.

Signed-off-by: Ernesto Ramos <ernesto@ti.com>
---
 drivers/staging/tidspbridge/rmgr/dbdcd.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/dbdcd.c b/drivers/staging/tidspbridge/rmgr/dbdcd.c
index cceceb9..b96aea7 100644
--- a/drivers/staging/tidspbridge/rmgr/dbdcd.c
+++ b/drivers/staging/tidspbridge/rmgr/dbdcd.c
@@ -1012,6 +1012,8 @@ static s32 atoi(char *psz_buf)
 {
 	char *pch = psz_buf;
 	s32 base = 0;
+	unsigned long res;
+	int ret_val;
 
 	while (isspace(*pch))
 		pch++;
@@ -1023,7 +1025,9 @@ static s32 atoi(char *psz_buf)
 		base = 16;
 	}
 
-	return simple_strtoul(pch, NULL, base);
+	ret_val = strict_strtoul(pch, base, &res);
+
+	return ret_val ? : res;
 }
 
 /*
-- 
1.5.4.5

Re: [PATCH 5/5] staging:ti dspbridge: replace simple_strtoul by strict_strtoul

[Greg KH]

On Wed, Jul 28, 2010 at 09:40:52AM -0500, Ernesto Ramos wrote:
> Replace simple_strtoul by strict_strtoul in atoi function.

Why not use the built-in kernel function instead of having your own atoi
function?

thanks,

greg k-h

Re: [PATCH 5/5] staging:ti dspbridge: replace simple_strtoul by strict_strtoul

[Andy Shevchenko]

On Wed, Jul 28, 2010 at 6:09 PM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Jul 28, 2010 at 09:40:52AM -0500, Ernesto Ramos wrote:
>> Replace simple_strtoul by strict_strtoul in atoi function.
>
> Why not use the built-in kernel function instead of having your own atoi
> function?
The commit message left opened questions about
http://dev.omapzoom.org/?p=tidspbridge/kernel-dspbridge.git;a=commit;h=b8af1123b47741086ef5a307ad1ec8fec6fc7f0d

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] staging:ti dspbridge: replace simple_strtoul by strict_strtoul

[Greg KH]

On Wed, Jul 28, 2010 at 09:40:19PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 28, 2010 at 6:09 PM, Greg KH <gregkh@suse.de> wrote:
> > On Wed, Jul 28, 2010 at 09:40:52AM -0500, Ernesto Ramos wrote:
> >> Replace simple_strtoul by strict_strtoul in atoi function.
> >
> > Why not use the built-in kernel function instead of having your own atoi
> > function?
> The commit message left opened questions about
> http://dev.omapzoom.org/?p=tidspbridge/kernel-dspbridge.git;a=commit;h=b8af1123b47741086ef5a307ad1ec8fec6fc7f0d

Ok, but you will have to fix this up eventually, you need to use the
in-kernel library functions, or fix the in-kernel ones to add the
support you need.

thanks,

greg k-h

Re: [PATCH 5/5] staging:ti dspbridge: replace simple_strtoul by strict_strtoul

[Andy Shevchenko]

On Wed, Jul 28, 2010 at 5:40 PM, Ernesto Ramos <ernesto@ti.com> wrote:
> Replace simple_strtoul by strict_strtoul in atoi function.
In general the question what is the purpose here to do the change?

> @@ -1023,7 +1025,9 @@ static s32 atoi(char *psz_buf)
>                base = 16;
>        }
>
> -       return simple_strtoul(pch, NULL, base);
> +       ret_val = strict_strtoul(pch, base, &res);
> +
> +       return ret_val ? : res;
May be better to use explicit values in condition?

-- 
With Best Regards,
Andy Shevchenko
--
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 5/5] staging:ti dspbridge: replace simple_strtoul by strict_strtoul

[Ramos Falcon, Ernesto]

Hi,

>-----Original Message-----
>From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>Sent: Wednesday, July 28, 2010 1:44 PM
>To: Ramos Falcon, Ernesto
>Cc: gregkh@suse.de; Ramirez Luna, Omar; ohad@wizery.com;
>ameya.palande@nokia.com; felipe.contreras@nokia.com; Guzman Lugo, Fernando;
>linux-kernel@vger.kernel.org; Menon, Nishanth; linux-omap@vger.kernel.org
>Subject: Re: [PATCH 5/5] staging:ti dspbridge: replace simple_strtoul by
>strict_strtoul
>
>On Wed, Jul 28, 2010 at 5:40 PM, Ernesto Ramos <ernesto@ti.com> wrote:
>> Replace simple_strtoul by strict_strtoul in atoi function.
>In general the question what is the purpose here to do the change?
>

It is recommended to use strict_strtoul instead of simple_strtoul since ensures the string is really a valid unsigned long. This also removes checkpatch warning message.

>> @@ -1023,7 +1025,9 @@ static s32 atoi(char *psz_buf)
>>                base = 16;
>>        }
>>
>> -       return simple_strtoul(pch, NULL, base);
>> +       ret_val = strict_strtoul(pch, base, &res);
>> +
>> +       return ret_val ? : res;
>May be better to use explicit values in condition?
>

I think this is ok as it is, I'm not sure but I think it maybe defined in any C ansi book, in this case if ret_val is unequal to 0 ret_val is returned.

>--
>With Best Regards,
>Andy Shevchenko

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

[Nishanth Menon]

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

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

[Ramos Falcon, Ernesto]

Hi,

>-----Original Message-----
>From: Menon, Nishanth
>Sent: Wednesday, July 28, 2010 10:04 AM
>To: Ramos Falcon, Ernesto
>Cc: gregkh@suse.de; Ramirez Luna, Omar; ohad@wizery.com;
>ameya.palande@nokia.com; felipe.contreras@nokia.com; Guzman Lugo, Fernando;
>linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com; linux-
>omap@vger.kernel.org
>Subject: Re: [PATCH 4/5] staging:ti dspbridge: remove unnecessary volatile
>variables

<snip>

>> 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?
>
I was not aware of the existence of writel, but I think I should use writel instead.

>>
>>  	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..
>
This is done intentionally and it is used to debug the dsp.  The system will hang here, and then Lautherback or jtag can be connected to take control of the program execution from this point.

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

Agree!

Re: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c

[Nishanth Menon]

Ramos Falcon, Ernesto had written, on 07/28/2010 09:40 AM, the following:
> Remove unnecessary check for NULL pointer in cmm.c.
> 
> Signed-off-by: Ernesto Ramos <ernesto@ti.com>
> ---
>  drivers/staging/tidspbridge/pmgr/cmm.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c b/drivers/staging/tidspbridge/pmgr/cmm.c
> index 8e808d9..874ed64 100644
> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
> @@ -992,16 +992,12 @@ int cmm_xlator_create(struct cmm_xlatorobject **xlator,
>  int cmm_xlator_delete(struct cmm_xlatorobject *xlator, bool force)
>  {
>  	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
> -	int status = 0;
>  
>  	DBC_REQUIRE(refs > 0);
>  
> -	if (xlator_obj)
> -		kfree(xlator_obj);
> -	else
> -		status = -EFAULT;
> +	kfree(xlator_obj);
>  
> -	return status;
> +	return 0;
>  }
>  
>  /*
which kind of begs the question - does this function need to any longer 
return int?

-- 
Regards,
Nishanth Menon

Re: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c

[Nishanth Menon]

Nishanth Menon had written, on 07/28/2010 10:04 AM, the following:
> Ramos Falcon, Ernesto had written, on 07/28/2010 09:40 AM, the following:
>> Remove unnecessary check for NULL pointer in cmm.c.
>>
>> Signed-off-by: Ernesto Ramos <ernesto@ti.com>
>> ---
>>  drivers/staging/tidspbridge/pmgr/cmm.c |    8 ++------
>>  1 files changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c b/drivers/staging/tidspbridge/pmgr/cmm.c
>> index 8e808d9..874ed64 100644
>> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
>> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
>> @@ -992,16 +992,12 @@ int cmm_xlator_create(struct cmm_xlatorobject **xlator,
>>  int cmm_xlator_delete(struct cmm_xlatorobject *xlator, bool force)
>>  {
>>  	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
>> -	int status = 0;
>>  
>>  	DBC_REQUIRE(refs > 0);
>>  
>> -	if (xlator_obj)
>> -		kfree(xlator_obj);
>> -	else
>> -		status = -EFAULT;
>> +	kfree(xlator_obj);
>>  
>> -	return status;
>> +	return 0;
>>  }
>>  
>>  /*
> which kind of begs the question - does this function need to any longer 
> return int?
> 
here is a better approach:
remove cmm_xlator_delete altogether
diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c 
b/drivers/staging/tidspbridge/pmgr/cmm.c
index 8e808d9..1a4ebca 100644
--- a/drivers/staging/tidspbridge/pmgr/cmm.c
+++ b/drivers/staging/tidspbridge/pmgr/cmm.c
@@ -984,27 +984,6 @@ int cmm_xlator_create(struct cmm_xlatorobject **xlator,
  }

  /*
- *  ======== cmm_xlator_delete ========
- *  Purpose:
- *      Free the Xlator resources.
- *      VM gets freed later.
- */
-int cmm_xlator_delete(struct cmm_xlatorobject *xlator, bool force)
-{
-	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
-	int status = 0;
-
-	DBC_REQUIRE(refs > 0);
-
-	if (xlator_obj)
-		kfree(xlator_obj);
-	else
-		status = -EFAULT;
-
-	return status;
-}
-
-/*
   *  ======== cmm_xlator_alloc_buf ========
   */
  void *cmm_xlator_alloc_buf(struct cmm_xlatorobject *xlator, void *va_buf,
diff --git a/drivers/staging/tidspbridge/rmgr/node.c 
b/drivers/staging/tidspbridge/rmgr/node.c
index 9f07c81..0c39288 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -2503,7 +2503,6 @@ static void delete_node(struct node_object *hnode,
  			struct process_context *pr_ctxt)
  {
  	struct node_mgr *hnode_mgr;
-	struct cmm_xlatorobject *xlator;
  	struct bridge_drv_interface *intf_fxns;
  	u32 i;
  	enum node_type node_type;
@@ -2521,7 +2520,6 @@ static void delete_node(struct node_object *hnode,
  	hnode_mgr = hnode->hnode_mgr;
  	if (!hnode_mgr)
  		goto func_end;
-	xlator = hnode->xlator;
  	node_type = node_get_type(hnode);
  	if (node_type != NODE_DEVICE) {
  		node_msg_args = hnode->create_args.asa.node_msg_args;
@@ -2617,10 +2615,7 @@ static void delete_node(struct node_object *hnode,
  	hnode->dcd_props.obj_data.node_obj.pstr_i_alg_name = NULL;

  	/* Free all SM address translator resources */
-	if (xlator) {
-		(void)cmm_xlator_delete(xlator, true);	/* force free */
-		xlator = NULL;
-	}
+	kfree(hnode->xlator);

  	kfree(hnode->nldr_node_obj);
  	hnode->nldr_node_obj = NULL;
diff --git a/drivers/staging/tidspbridge/rmgr/strm.c 
b/drivers/staging/tidspbridge/rmgr/strm.c
index 73888e3..05a8d13 100644
--- a/drivers/staging/tidspbridge/rmgr/strm.c
+++ b/drivers/staging/tidspbridge/rmgr/strm.c
@@ -844,14 +844,8 @@ static int delete_strm(struct strm_object *stream_obj)
  			status = (*intf_fxns->pfn_chnl_close)
  					(stream_obj->chnl_obj);
  			/* Free all SM address translator resources */
-			if (DSP_SUCCEEDED(status)) {
-				if (stream_obj->xlator) {
-					/* force free */
-					(void)cmm_xlator_delete(stream_obj->
-								xlator,
-								true);
-				}
-			}
+			if (DSP_SUCCEEDED(status))
+				kfree(stream_obj->xlator);
  		}
  		kfree(stream_obj);
  	} else {
-- 
Regards,
Nishanth Menon

RE: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c

[Ramos Falcon, Ernesto]

>-----Original Message-----
>From: Menon, Nishanth
>Sent: Wednesday, July 28, 2010 10:32 AM
>To: Ramos Falcon, Ernesto
>Cc: gregkh@suse.de; Ramirez Luna, Omar; ohad@wizery.com;
>ameya.palande@nokia.com; felipe.contreras@nokia.com; Guzman Lugo, Fernando;
>linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com; linux-
>omap@vger.kernel.org
>Subject: Re: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for
>NULL pointer in cmm.c
>
>Nishanth Menon had written, on 07/28/2010 10:04 AM, the following:
>> Ramos Falcon, Ernesto had written, on 07/28/2010 09:40 AM, the following:
>>> Remove unnecessary check for NULL pointer in cmm.c.
>>>
>>> Signed-off-by: Ernesto Ramos <ernesto@ti.com>
>>> ---
>>>  drivers/staging/tidspbridge/pmgr/cmm.c |    8 ++------
>>>  1 files changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c
>b/drivers/staging/tidspbridge/pmgr/cmm.c
>>> index 8e808d9..874ed64 100644
>>> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
>>> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
>>> @@ -992,16 +992,12 @@ int cmm_xlator_create(struct cmm_xlatorobject
>**xlator,
>>>  int cmm_xlator_delete(struct cmm_xlatorobject *xlator, bool force)
>>>  {
>>>  	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
>>> -	int status = 0;
>>>
>>>  	DBC_REQUIRE(refs > 0);
>>>
>>> -	if (xlator_obj)
>>> -		kfree(xlator_obj);
>>> -	else
>>> -		status = -EFAULT;
>>> +	kfree(xlator_obj);
>>>
>>> -	return status;
>>> +	return 0;
>>>  }
>>>
>>>  /*
>> which kind of begs the question - does this function need to any longer
>> return int?
>>
>here is a better approach:
>remove cmm_xlator_delete altogether
>diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c
>b/drivers/staging/tidspbridge/pmgr/cmm.c
>index 8e808d9..1a4ebca 100644
>--- a/drivers/staging/tidspbridge/pmgr/cmm.c
>+++ b/drivers/staging/tidspbridge/pmgr/cmm.c
>@@ -984,27 +984,6 @@ int cmm_xlator_create(struct cmm_xlatorobject
>**xlator,
>  }
>
>  /*
>- *  ======== cmm_xlator_delete ========
>- *  Purpose:
>- *      Free the Xlator resources.
>- *      VM gets freed later.
>- */
>-int cmm_xlator_delete(struct cmm_xlatorobject *xlator, bool force)
>-{
>-	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
>-	int status = 0;
>-
>-	DBC_REQUIRE(refs > 0);
>-
>-	if (xlator_obj)
>-		kfree(xlator_obj);
>-	else
>-		status = -EFAULT;
>-
>-	return status;
>-}
>-
>-/*
>   *  ======== cmm_xlator_alloc_buf ========
>   */
>  void *cmm_xlator_alloc_buf(struct cmm_xlatorobject *xlator, void *va_buf,
>diff --git a/drivers/staging/tidspbridge/rmgr/node.c
>b/drivers/staging/tidspbridge/rmgr/node.c
>index 9f07c81..0c39288 100644
>--- a/drivers/staging/tidspbridge/rmgr/node.c
>+++ b/drivers/staging/tidspbridge/rmgr/node.c
>@@ -2503,7 +2503,6 @@ static void delete_node(struct node_object *hnode,
>  			struct process_context *pr_ctxt)
>  {
>  	struct node_mgr *hnode_mgr;
>-	struct cmm_xlatorobject *xlator;
>  	struct bridge_drv_interface *intf_fxns;
>  	u32 i;
>  	enum node_type node_type;
>@@ -2521,7 +2520,6 @@ static void delete_node(struct node_object *hnode,
>  	hnode_mgr = hnode->hnode_mgr;
>  	if (!hnode_mgr)
>  		goto func_end;
>-	xlator = hnode->xlator;
>  	node_type = node_get_type(hnode);
>  	if (node_type != NODE_DEVICE) {
>  		node_msg_args = hnode->create_args.asa.node_msg_args;
>@@ -2617,10 +2615,7 @@ static void delete_node(struct node_object *hnode,
>  	hnode->dcd_props.obj_data.node_obj.pstr_i_alg_name = NULL;
>
>  	/* Free all SM address translator resources */
>-	if (xlator) {
>-		(void)cmm_xlator_delete(xlator, true);	/* force free */
>-		xlator = NULL;
>-	}
>+	kfree(hnode->xlator);
>
>  	kfree(hnode->nldr_node_obj);
>  	hnode->nldr_node_obj = NULL;
>diff --git a/drivers/staging/tidspbridge/rmgr/strm.c
>b/drivers/staging/tidspbridge/rmgr/strm.c
>index 73888e3..05a8d13 100644
>--- a/drivers/staging/tidspbridge/rmgr/strm.c
>+++ b/drivers/staging/tidspbridge/rmgr/strm.c
>@@ -844,14 +844,8 @@ static int delete_strm(struct strm_object *stream_obj)
>  			status = (*intf_fxns->pfn_chnl_close)
>  					(stream_obj->chnl_obj);
>  			/* Free all SM address translator resources */
>-			if (DSP_SUCCEEDED(status)) {
>-				if (stream_obj->xlator) {
>-					/* force free */
>-					(void)cmm_xlator_delete(stream_obj->
>-								xlator,
>-								true);
>-				}
>-			}
>+			if (DSP_SUCCEEDED(status))
>+				kfree(stream_obj->xlator);
>  		}
>  		kfree(stream_obj);
>  	} else {
>--
>Regards,
>Nishanth Menon

Thanks Nishanth,

I considered this approach before but in terms of maintainability I thought it was easier to locate where translator tables are destroy if we keep cmm_xlator_delete function.

/Ernesto

Re: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c

[Felipe Contreras]

Hi Ernesto,

On Wed, Jul 28, 2010 at 7:53 PM, Ramos Falcon, Ernesto <ernesto@ti.com> wrote:
>>here is a better approach:
>>remove cmm_xlator_delete altogether

[...]

> I considered this approach before but in terms of maintainability I thought it was easier to locate where translator tables are destroy if we keep cmm_xlator_delete function.

That's not maintainability, that's debugging convenience. AFAIU linux
is all about maintenance, because that provides real, tangible, and
proven gains. Debugging convenience gains are hypothetical; you don't
really know how useful it will be to have a separate free function.

And please avoid bottom-posting, instead use interleaved style,
otherwise people see a comment, and then have to scroll down to see
the answer.
http://en.wikipedia.org/wiki/Posting_style#Interleaved_style

-- 
Felipe Contreras

RE: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c

[Ramos Falcon, Ernesto]

Agree! I'll send new version of this patch.

>-----Original Message-----
>From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
>Sent: Wednesday, July 28, 2010 12:12 PM
>To: Ramos Falcon, Ernesto
>Cc: Menon, Nishanth; gregkh@suse.de; Ramirez Luna, Omar; ohad@wizery.com;
>ameya.palande@nokia.com; felipe.contreras@nokia.com; Guzman Lugo, Fernando;
>linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com; linux-
>omap@vger.kernel.org
>Subject: Re: [PATCH 2/5] staging:ti dspbridge: remove unnecessary check for
>NULL pointer in cmm.c
>
>Hi Ernesto,
>
>On Wed, Jul 28, 2010 at 7:53 PM, Ramos Falcon, Ernesto <ernesto@ti.com>
>wrote:
>>>here is a better approach:
>>>remove cmm_xlator_delete altogether
>
>[...]
>
>> I considered this approach before but in terms of maintainability I
>thought it was easier to locate where translator tables are destroy if we
>keep cmm_xlator_delete function.
>
>That's not maintainability, that's debugging convenience. AFAIU linux
>is all about maintenance, because that provides real, tangible, and
>proven gains. Debugging convenience gains are hypothetical; you don't
>really know how useful it will be to have a separate free function.
>
>And please avoid bottom-posting, instead use interleaved style,
>otherwise people see a comment, and then have to scroll down to see
>the answer.
>http://en.wikipedia.org/wiki/Posting_style#Interleaved_style
>
>--
>Felipe Contreras