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(-)
Remove unnecessary volatile variables; use accessor
functions __raw_readl/__raw_writel instead when applicable.
Signed-off-by: Ernesto Ramos <[email protected]>
---
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
Remove unused typedef REG16 in dspbridge.
Signed-off-by: Ernesto Ramos <[email protected]>
---
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
Remove unnecessary check for NULL pointer in cmm.c.
Signed-off-by: Ernesto Ramos <[email protected]>
---
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
Remove function delete_strm_mgr in strm.c and
use kfree instead.
Signed-off-by: Ernesto Ramos <[email protected]>
---
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
Replace simple_strtoul by strict_strtoul in atoi function.
Signed-off-by: Ernesto Ramos <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
>> ---
>> 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
>-----Original Message-----
>From: Menon, Nishanth
>Sent: Wednesday, July 28, 2010 10:32 AM
>To: Ramos Falcon, Ernesto
>Cc: [email protected]; Ramirez Luna, Omar; [email protected];
>[email protected]; [email protected]; Guzman Lugo, Fernando;
>[email protected]; [email protected]; linux-
>[email protected]
>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 <[email protected]>
>>> ---
>>> 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
Hi Ernesto,
On Wed, Jul 28, 2010 at 7:53 PM, Ramos Falcon, Ernesto <[email protected]> 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
Agree! I'll send new version of this patch.
>-----Original Message-----
>From: Felipe Contreras [mailto:[email protected]]
>Sent: Wednesday, July 28, 2010 12:12 PM
>To: Ramos Falcon, Ernesto
>Cc: Menon, Nishanth; [email protected]; Ramirez Luna, Omar; [email protected];
>[email protected]; [email protected]; Guzman Lugo, Fernando;
>[email protected]; [email protected]; linux-
>[email protected]
>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 <[email protected]>
>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
On Wed, Jul 28, 2010 at 6:09 PM, Greg KH <[email protected]> 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
On Wed, Jul 28, 2010 at 5:40 PM, Ernesto Ramos <[email protected]> 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
On Wed, Jul 28, 2010 at 09:40:19PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 28, 2010 at 6:09 PM, Greg KH <[email protected]> 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
Hi,
>-----Original Message-----
>From: Menon, Nishanth
>Sent: Wednesday, July 28, 2010 10:04 AM
>To: Ramos Falcon, Ernesto
>Cc: [email protected]; Ramirez Luna, Omar; [email protected];
>[email protected]; [email protected]; Guzman Lugo, Fernando;
>[email protected]; [email protected]; linux-
>[email protected]
>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!
Hi,
>-----Original Message-----
>From: Andy Shevchenko [mailto:[email protected]]
>Sent: Wednesday, July 28, 2010 1:44 PM
>To: Ramos Falcon, Ernesto
>Cc: [email protected]; Ramirez Luna, Omar; [email protected];
>[email protected]; [email protected]; Guzman Lugo, Fernando;
>[email protected]; Menon, Nishanth; [email protected]
>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 <[email protected]> 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