[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padmavathi Venna]

This patch removes the usage of DMACH_DT_PROP and dt_dmach_prop
from dma code as the new generic dma dt binding support has been
added.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---

The functionality of this patch is dependent on following patches in the link.
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg15402.html

This patch is made based on Mark Brown next branch.

 arch/arm/mach-s3c24xx/include/mach/dma.h       |    1 -
 arch/arm/mach-s3c64xx/include/mach/dma.h       |    1 -
 arch/arm/plat-samsung/dma-ops.c                |   10 +---------
 arch/arm/plat-samsung/include/plat/dma-ops.h   |    1 -
 arch/arm/plat-samsung/include/plat/dma-pl330.h |    1 -
 5 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/include/mach/dma.h b/arch/arm/mach-s3c24xx/include/mach/dma.h
index 6b72d5a..b55da1d 100644
--- a/arch/arm/mach-s3c24xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c24xx/include/mach/dma.h
@@ -24,7 +24,6 @@
 */
 
 enum dma_ch {
-	DMACH_DT_PROP = -1,	/* not yet supported, do not use */
 	DMACH_XD0 = 0,
 	DMACH_XD1,
 	DMACH_SDI,
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 57b1ff4..fe1a98c 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -21,7 +21,6 @@
  */
 enum dma_ch {
 	/* DMA0/SDMA0 */
-	DMACH_DT_PROP = -1, /* not yet supported, do not use */
 	DMACH_UART0 = 0,
 	DMACH_UART0_SRC2,
 	DMACH_UART1,
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index 71d58dd..ec0d731 100644
--- a/arch/arm/plat-samsung/dma-ops.c
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
 				struct device *dev, char *ch_name)
 {
 	dma_cap_mask_t mask;
-	void *filter_param;
 
 	dma_cap_zero(mask);
 	dma_cap_set(param->cap, mask);
 
-	/*
-	 * If a dma channel property of a device node from device tree is
-	 * specified, use that as the fliter parameter.
-	 */
-	filter_param = (dma_ch == DMACH_DT_PROP) ?
-		(void *)param->dt_dmach_prop : (void *)dma_ch;
-
 	if (dev->of_node)
 		return (unsigned)dma_request_slave_channel(dev, ch_name);
 	else
 		return (unsigned)dma_request_channel(mask, pl330_filter,
-							filter_param);
+							(void *)dma_ch);
 }
 
 static int samsung_dmadev_release(unsigned ch, void *param)
diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h
index 1141782..ce6d763 100644
--- a/arch/arm/plat-samsung/include/plat/dma-ops.h
+++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
@@ -18,7 +18,6 @@
 
 struct samsung_dma_req {
 	enum dma_transaction_type cap;
-	struct property *dt_dmach_prop;
 	struct s3c2410_dma_client *client;
 };
 
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index d384a80..abe07fa 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -21,7 +21,6 @@
  * use these just as IDs.
  */
 enum dma_ch {
-	DMACH_DT_PROP = -1,
 	DMACH_UART0_RX = 0,
 	DMACH_UART0_TX,
 	DMACH_UART1_RX,
-- 
1.7.4.4

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padmavathi Venna]

This patch removes the usage of DMACH_DT_PROP and dt_dmach_prop
from dma code as the new generic dma dt binding support has been
added.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---

The functionality of this patch is dependent on following patches in the link.
http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg15402.html

This patch is made based on Mark Brown next branch.

 arch/arm/mach-s3c24xx/include/mach/dma.h       |    1 -
 arch/arm/mach-s3c64xx/include/mach/dma.h       |    1 -
 arch/arm/plat-samsung/dma-ops.c                |   10 +---------
 arch/arm/plat-samsung/include/plat/dma-ops.h   |    1 -
 arch/arm/plat-samsung/include/plat/dma-pl330.h |    1 -
 5 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/include/mach/dma.h b/arch/arm/mach-s3c24xx/include/mach/dma.h
index 6b72d5a..b55da1d 100644
--- a/arch/arm/mach-s3c24xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c24xx/include/mach/dma.h
@@ -24,7 +24,6 @@
 */
 
 enum dma_ch {
-	DMACH_DT_PROP = -1,	/* not yet supported, do not use */
 	DMACH_XD0 = 0,
 	DMACH_XD1,
 	DMACH_SDI,
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 57b1ff4..fe1a98c 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -21,7 +21,6 @@
  */
 enum dma_ch {
 	/* DMA0/SDMA0 */
-	DMACH_DT_PROP = -1, /* not yet supported, do not use */
 	DMACH_UART0 = 0,
 	DMACH_UART0_SRC2,
 	DMACH_UART1,
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index 71d58dd..ec0d731 100644
--- a/arch/arm/plat-samsung/dma-ops.c
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
 				struct device *dev, char *ch_name)
 {
 	dma_cap_mask_t mask;
-	void *filter_param;
 
 	dma_cap_zero(mask);
 	dma_cap_set(param->cap, mask);
 
-	/*
-	 * If a dma channel property of a device node from device tree is
-	 * specified, use that as the fliter parameter.
-	 */
-	filter_param = (dma_ch == DMACH_DT_PROP) ?
-		(void *)param->dt_dmach_prop : (void *)dma_ch;
-
 	if (dev->of_node)
 		return (unsigned)dma_request_slave_channel(dev, ch_name);
 	else
 		return (unsigned)dma_request_channel(mask, pl330_filter,
-							filter_param);
+							(void *)dma_ch);
 }
 
 static int samsung_dmadev_release(unsigned ch, void *param)
diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h
index 1141782..ce6d763 100644
--- a/arch/arm/plat-samsung/include/plat/dma-ops.h
+++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
@@ -18,7 +18,6 @@
 
 struct samsung_dma_req {
 	enum dma_transaction_type cap;
-	struct property *dt_dmach_prop;
 	struct s3c2410_dma_client *client;
 };
 
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index d384a80..abe07fa 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -21,7 +21,6 @@
  * use these just as IDs.
  */
 enum dma_ch {
-	DMACH_DT_PROP = -1,
 	DMACH_UART0_RX = 0,
 	DMACH_UART0_TX,
 	DMACH_UART1_RX,
-- 
1.7.4.4

Re: [PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Arnd Bergmann]

On Monday 04 February 2013, Padmavathi Venna wrote:
> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> index 71d58dd..ec0d731 100644
> --- a/arch/arm/plat-samsung/dma-ops.c
> +++ b/arch/arm/plat-samsung/dma-ops.c
> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>  				struct device *dev, char *ch_name)
>  {
>  	dma_cap_mask_t mask;
> -	void *filter_param;
>  
>  	dma_cap_zero(mask);
>  	dma_cap_set(param->cap, mask);
>  
> -	/*
> -	 * If a dma channel property of a device node from device tree is
> -	 * specified, use that as the fliter parameter.
> -	 */
> -	filter_param = (dma_ch == DMACH_DT_PROP) ?
> -		(void *)param->dt_dmach_prop : (void *)dma_ch;
> -
>  	if (dev->of_node)
>  		return (unsigned)dma_request_slave_channel(dev, ch_name);
>  	else
>  		return (unsigned)dma_request_channel(mask, pl330_filter,
> -							filter_param);
> +							(void *)dma_ch);
>  }

This still looks wrong to me, because the pl330_filter function now tkes
a struct dma_pl330_filter_args pointer argument, not dma_ch name.

	Arnd

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Arnd Bergmann]

On Monday 04 February 2013, Padmavathi Venna wrote:
> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> index 71d58dd..ec0d731 100644
> --- a/arch/arm/plat-samsung/dma-ops.c
> +++ b/arch/arm/plat-samsung/dma-ops.c
> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>  				struct device *dev, char *ch_name)
>  {
>  	dma_cap_mask_t mask;
> -	void *filter_param;
>  
>  	dma_cap_zero(mask);
>  	dma_cap_set(param->cap, mask);
>  
> -	/*
> -	 * If a dma channel property of a device node from device tree is
> -	 * specified, use that as the fliter parameter.
> -	 */
> -	filter_param = (dma_ch == DMACH_DT_PROP) ?
> -		(void *)param->dt_dmach_prop : (void *)dma_ch;
> -
>  	if (dev->of_node)
>  		return (unsigned)dma_request_slave_channel(dev, ch_name);
>  	else
>  		return (unsigned)dma_request_channel(mask, pl330_filter,
> -							filter_param);
> +							(void *)dma_ch);
>  }

This still looks wrong to me, because the pl330_filter function now tkes
a struct dma_pl330_filter_args pointer argument, not dma_ch name.

	Arnd

Re: [PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padma Venkat]

Hi Arnd,

On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 04 February 2013, Padmavathi Venna wrote:
>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>> index 71d58dd..ec0d731 100644
>> --- a/arch/arm/plat-samsung/dma-ops.c
>> +++ b/arch/arm/plat-samsung/dma-ops.c
>> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>>                               struct device *dev, char *ch_name)
>>  {
>>       dma_cap_mask_t mask;
>> -     void *filter_param;
>>
>>       dma_cap_zero(mask);
>>       dma_cap_set(param->cap, mask);
>>
>> -     /*
>> -      * If a dma channel property of a device node from device tree is
>> -      * specified, use that as the fliter parameter.
>> -      */
>> -     filter_param = (dma_ch == DMACH_DT_PROP) ?
>> -             (void *)param->dt_dmach_prop : (void *)dma_ch;
>> -
>>       if (dev->of_node)
>>               return (unsigned)dma_request_slave_channel(dev, ch_name);
>>       else
>>               return (unsigned)dma_request_channel(mask, pl330_filter,
>> -                                                     filter_param);
>> +                                                     (void *)dma_ch);
>>  }
>
> This still looks wrong to me, because the pl330_filter function now tkes
> a struct dma_pl330_filter_args pointer argument, not dma_ch name.

Below is my understanding about generic dma and our discussion on
previous versions of my patches.

I can’t pass single dma channel number(may be not dma_ch name in your
comment above) as void* argument to pl330_filter.  Because I also need
to compare against the dma controller device node, as my requested
channel can belong to any of the available dma controller on SoC.  So
I either need to pass pointer to dma_spec as void* argument which
holds the dma controller node and required channel number or I can
pass pointer to dma_pl330_filter_args as per your dw_dmac patches.

If I pass pointer to dma_spec I can have a check like below in my
filter function
return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]))

Or if I pass dma_pl330_filter_args I can have a check like below.
return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id ==
fargs->chan_id));

I modified  the pl330_filter function based on your dw_dmac patches.
Indeed I don’t need to pass pointer to pdmac object as 3rd arg in
of_dma_controller_register .  Even I pass NULL here works for me.
Can I pass NULL here as the third argument in of_dma_controller_register?
Please clarify me which is best way of doing this and correct me if my
understanding is wrong.

>
>         Arnd
Thanks
Padma

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padma Venkat]

Hi Arnd,

On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 04 February 2013, Padmavathi Venna wrote:
>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>> index 71d58dd..ec0d731 100644
>> --- a/arch/arm/plat-samsung/dma-ops.c
>> +++ b/arch/arm/plat-samsung/dma-ops.c
>> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>>                               struct device *dev, char *ch_name)
>>  {
>>       dma_cap_mask_t mask;
>> -     void *filter_param;
>>
>>       dma_cap_zero(mask);
>>       dma_cap_set(param->cap, mask);
>>
>> -     /*
>> -      * If a dma channel property of a device node from device tree is
>> -      * specified, use that as the fliter parameter.
>> -      */
>> -     filter_param = (dma_ch == DMACH_DT_PROP) ?
>> -             (void *)param->dt_dmach_prop : (void *)dma_ch;
>> -
>>       if (dev->of_node)
>>               return (unsigned)dma_request_slave_channel(dev, ch_name);
>>       else
>>               return (unsigned)dma_request_channel(mask, pl330_filter,
>> -                                                     filter_param);
>> +                                                     (void *)dma_ch);
>>  }
>
> This still looks wrong to me, because the pl330_filter function now tkes
> a struct dma_pl330_filter_args pointer argument, not dma_ch name.

Below is my understanding about generic dma and our discussion on
previous versions of my patches.

I can?t pass single dma channel number(may be not dma_ch name in your
comment above) as void* argument to pl330_filter.  Because I also need
to compare against the dma controller device node, as my requested
channel can belong to any of the available dma controller on SoC.  So
I either need to pass pointer to dma_spec as void* argument which
holds the dma controller node and required channel number or I can
pass pointer to dma_pl330_filter_args as per your dw_dmac patches.

If I pass pointer to dma_spec I can have a check like below in my
filter function
return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]))

Or if I pass dma_pl330_filter_args I can have a check like below.
return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id ==
fargs->chan_id));

I modified  the pl330_filter function based on your dw_dmac patches.
Indeed I don?t need to pass pointer to pdmac object as 3rd arg in
of_dma_controller_register .  Even I pass NULL here works for me.
Can I pass NULL here as the third argument in of_dma_controller_register?
Please clarify me which is best way of doing this and correct me if my
understanding is wrong.

>
>         Arnd
Thanks
Padma

Re: [PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Arnd Bergmann]

On Tuesday 05 February 2013, Padma Venkat wrote:
> On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 04 February 2013, Padmavathi Venna wrote:
> >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> >> index 71d58dd..ec0d731 100644
> >> --- a/arch/arm/plat-samsung/dma-ops.c
> >> +++ b/arch/arm/plat-samsung/dma-ops.c
> >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
> >>                               struct device *dev, char *ch_name)
> >>  {
> >>       dma_cap_mask_t mask;
> >> -     void *filter_param;
> >>
> >>       dma_cap_zero(mask);
> >>       dma_cap_set(param->cap, mask);
> >>
> >> -     /*
> >> -      * If a dma channel property of a device node from device tree is
> >> -      * specified, use that as the fliter parameter.
> >> -      */
> >> -     filter_param = (dma_ch == DMACH_DT_PROP) ?
> >> -             (void *)param->dt_dmach_prop : (void *)dma_ch;
> >> -
> >>       if (dev->of_node)
> >>               return (unsigned)dma_request_slave_channel(dev, ch_name);
> >>       else
> >>               return (unsigned)dma_request_channel(mask, pl330_filter,
> >> -                                                     filter_param);
> >> +                                                     (void *)dma_ch);
> >>  }
> >
> > This still looks wrong to me, because the pl330_filter function now tkes
> > a struct dma_pl330_filter_args pointer argument, not dma_ch name.
> 
> Below is my understanding about generic dma and our discussion on
> previous versions of my patches.
> 
> I can’t pass single dma channel number(may be not dma_ch name in your
> comment above) as void* argument to pl330_filter.  Because I also need
> to compare against the dma controller device node, as my requested
> channel can belong to any of the available dma controller on SoC.  So
> I either need to pass pointer to dma_spec as void* argument which
> holds the dma controller node and required channel number or I can
> pass pointer to dma_pl330_filter_args as per your dw_dmac patches.

Right.

> If I pass pointer to dma_spec I can have a check like below in my
> filter function
> return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]))
> 
> Or if I pass dma_pl330_filter_args I can have a check like below.
> return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id ==
> fargs->chan_id));
> 
> I modified  the pl330_filter function based on your dw_dmac patches.
> Indeed I don’t need to pass pointer to pdmac object as 3rd arg in
> of_dma_controller_register .  Even I pass NULL here works for me.
> Can I pass NULL here as the third argument in of_dma_controller_register?

These are all not the issues I am referring to in my comment above.
I think it works either way, even if you pass NULL to
of_dma_controller_register, although using it for the pdmac object
seems cleaner to me.

> Please clarify me which is best way of doing this and correct me if my
> understanding is wrong.

My point was that in the samsung_dmadev_request quoted above, you
refer to the same pl330_filter filter function, but the argument there
is a pointer to 'enum dma_ch', which is not compatible with any of
the methods you list, neither the dma_pl330_filter_args nor the
raw property.

Also, if you change the calling conventions for the pl330_filter
function, you should change both the caller and the function in the
same patch.

	Arnd

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Arnd Bergmann]

On Tuesday 05 February 2013, Padma Venkat wrote:
> On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 04 February 2013, Padmavathi Venna wrote:
> >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> >> index 71d58dd..ec0d731 100644
> >> --- a/arch/arm/plat-samsung/dma-ops.c
> >> +++ b/arch/arm/plat-samsung/dma-ops.c
> >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
> >>                               struct device *dev, char *ch_name)
> >>  {
> >>       dma_cap_mask_t mask;
> >> -     void *filter_param;
> >>
> >>       dma_cap_zero(mask);
> >>       dma_cap_set(param->cap, mask);
> >>
> >> -     /*
> >> -      * If a dma channel property of a device node from device tree is
> >> -      * specified, use that as the fliter parameter.
> >> -      */
> >> -     filter_param = (dma_ch == DMACH_DT_PROP) ?
> >> -             (void *)param->dt_dmach_prop : (void *)dma_ch;
> >> -
> >>       if (dev->of_node)
> >>               return (unsigned)dma_request_slave_channel(dev, ch_name);
> >>       else
> >>               return (unsigned)dma_request_channel(mask, pl330_filter,
> >> -                                                     filter_param);
> >> +                                                     (void *)dma_ch);
> >>  }
> >
> > This still looks wrong to me, because the pl330_filter function now tkes
> > a struct dma_pl330_filter_args pointer argument, not dma_ch name.
> 
> Below is my understanding about generic dma and our discussion on
> previous versions of my patches.
> 
> I can?t pass single dma channel number(may be not dma_ch name in your
> comment above) as void* argument to pl330_filter.  Because I also need
> to compare against the dma controller device node, as my requested
> channel can belong to any of the available dma controller on SoC.  So
> I either need to pass pointer to dma_spec as void* argument which
> holds the dma controller node and required channel number or I can
> pass pointer to dma_pl330_filter_args as per your dw_dmac patches.

Right.

> If I pass pointer to dma_spec I can have a check like below in my
> filter function
> return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]))
> 
> Or if I pass dma_pl330_filter_args I can have a check like below.
> return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id ==
> fargs->chan_id));
> 
> I modified  the pl330_filter function based on your dw_dmac patches.
> Indeed I don?t need to pass pointer to pdmac object as 3rd arg in
> of_dma_controller_register .  Even I pass NULL here works for me.
> Can I pass NULL here as the third argument in of_dma_controller_register?

These are all not the issues I am referring to in my comment above.
I think it works either way, even if you pass NULL to
of_dma_controller_register, although using it for the pdmac object
seems cleaner to me.

> Please clarify me which is best way of doing this and correct me if my
> understanding is wrong.

My point was that in the samsung_dmadev_request quoted above, you
refer to the same pl330_filter filter function, but the argument there
is a pointer to 'enum dma_ch', which is not compatible with any of
the methods you list, neither the dma_pl330_filter_args nor the
raw property.

Also, if you change the calling conventions for the pl330_filter
function, you should change both the caller and the function in the
same patch.

	Arnd

Re: [PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padma Venkat]

Hi Arnd,

On Tue, Feb 5, 2013 at 4:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 05 February 2013, Padma Venkat wrote:
>> On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Monday 04 February 2013, Padmavathi Venna wrote:
>> >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>> >> index 71d58dd..ec0d731 100644
>> >> --- a/arch/arm/plat-samsung/dma-ops.c
>> >> +++ b/arch/arm/plat-samsung/dma-ops.c
>> >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>> >>                               struct device *dev, char *ch_name)
>> >>  {
>> >>       dma_cap_mask_t mask;
>> >> -     void *filter_param;
>> >>
>> >>       dma_cap_zero(mask);
>> >>       dma_cap_set(param->cap, mask);
>> >>
>> >> -     /*
>> >> -      * If a dma channel property of a device node from device tree is
>> >> -      * specified, use that as the fliter parameter.
>> >> -      */
>> >> -     filter_param = (dma_ch == DMACH_DT_PROP) ?
>> >> -             (void *)param->dt_dmach_prop : (void *)dma_ch;
>> >> -
>> >>       if (dev->of_node)
>> >>               return (unsigned)dma_request_slave_channel(dev, ch_name);
>> >>       else
>> >>               return (unsigned)dma_request_channel(mask, pl330_filter,
>> >> -                                                     filter_param);
>> >> +                                                     (void *)dma_ch);
>> >>  }
>> >
>> > This still looks wrong to me, because the pl330_filter function now tkes
>> > a struct dma_pl330_filter_args pointer argument, not dma_ch name.
>>
>> Below is my understanding about generic dma and our discussion on
>> previous versions of my patches.
>>
>> I can’t pass single dma channel number(may be not dma_ch name in your
>> comment above) as void* argument to pl330_filter.  Because I also need
>> to compare against the dma controller device node, as my requested
>> channel can belong to any of the available dma controller on SoC.  So
>> I either need to pass pointer to dma_spec as void* argument which
>> holds the dma controller node and required channel number or I can
>> pass pointer to dma_pl330_filter_args as per your dw_dmac patches.
>
> Right.
>
>> If I pass pointer to dma_spec I can have a check like below in my
>> filter function
>> return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]))
>>
>> Or if I pass dma_pl330_filter_args I can have a check like below.
>> return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id ==
>> fargs->chan_id));
>>
>> I modified  the pl330_filter function based on your dw_dmac patches.
>> Indeed I don’t need to pass pointer to pdmac object as 3rd arg in
>> of_dma_controller_register .  Even I pass NULL here works for me.
>> Can I pass NULL here as the third argument in of_dma_controller_register?
>
> These are all not the issues I am referring to in my comment above.
> I think it works either way, even if you pass NULL to
> of_dma_controller_register, although using it for the pdmac object
> seems cleaner to me.
>
>> Please clarify me which is best way of doing this and correct me if my
>> understanding is wrong.
>
> My point was that in the samsung_dmadev_request quoted above, you
> refer to the same pl330_filter filter function, but the argument there
> is a pointer to 'enum dma_ch', which is not compatible with any of
> the methods you list, neither the dma_pl330_filter_args nor the
> raw property.
>
> Also, if you change the calling conventions for the pl330_filter
> function, you should change both the caller and the function in the
> same patch.

In none of my patches I have changed the pl330_filter args.  This
function always takes the same argument void*. In non-DT case 'enum
dma_ch' was typecasted to void* and in DT case I am passing a pointer
to dma_pl330_filter_args and in pl330_filter function they are
converted back. In both cases it finally comes down to
dma_request_channel which takes them as void* which in turn calls the
pl330_filter.

I think this is what you are pointing to. Please let me know if I am
still wrong :( .

>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thanks
Padma

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padma Venkat]

Hi Arnd,

On Tue, Feb 5, 2013 at 4:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 05 February 2013, Padma Venkat wrote:
>> On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Monday 04 February 2013, Padmavathi Venna wrote:
>> >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>> >> index 71d58dd..ec0d731 100644
>> >> --- a/arch/arm/plat-samsung/dma-ops.c
>> >> +++ b/arch/arm/plat-samsung/dma-ops.c
>> >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>> >>                               struct device *dev, char *ch_name)
>> >>  {
>> >>       dma_cap_mask_t mask;
>> >> -     void *filter_param;
>> >>
>> >>       dma_cap_zero(mask);
>> >>       dma_cap_set(param->cap, mask);
>> >>
>> >> -     /*
>> >> -      * If a dma channel property of a device node from device tree is
>> >> -      * specified, use that as the fliter parameter.
>> >> -      */
>> >> -     filter_param = (dma_ch == DMACH_DT_PROP) ?
>> >> -             (void *)param->dt_dmach_prop : (void *)dma_ch;
>> >> -
>> >>       if (dev->of_node)
>> >>               return (unsigned)dma_request_slave_channel(dev, ch_name);
>> >>       else
>> >>               return (unsigned)dma_request_channel(mask, pl330_filter,
>> >> -                                                     filter_param);
>> >> +                                                     (void *)dma_ch);
>> >>  }
>> >
>> > This still looks wrong to me, because the pl330_filter function now tkes
>> > a struct dma_pl330_filter_args pointer argument, not dma_ch name.
>>
>> Below is my understanding about generic dma and our discussion on
>> previous versions of my patches.
>>
>> I can?t pass single dma channel number(may be not dma_ch name in your
>> comment above) as void* argument to pl330_filter.  Because I also need
>> to compare against the dma controller device node, as my requested
>> channel can belong to any of the available dma controller on SoC.  So
>> I either need to pass pointer to dma_spec as void* argument which
>> holds the dma controller node and required channel number or I can
>> pass pointer to dma_pl330_filter_args as per your dw_dmac patches.
>
> Right.
>
>> If I pass pointer to dma_spec I can have a check like below in my
>> filter function
>> return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]))
>>
>> Or if I pass dma_pl330_filter_args I can have a check like below.
>> return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id ==
>> fargs->chan_id));
>>
>> I modified  the pl330_filter function based on your dw_dmac patches.
>> Indeed I don?t need to pass pointer to pdmac object as 3rd arg in
>> of_dma_controller_register .  Even I pass NULL here works for me.
>> Can I pass NULL here as the third argument in of_dma_controller_register?
>
> These are all not the issues I am referring to in my comment above.
> I think it works either way, even if you pass NULL to
> of_dma_controller_register, although using it for the pdmac object
> seems cleaner to me.
>
>> Please clarify me which is best way of doing this and correct me if my
>> understanding is wrong.
>
> My point was that in the samsung_dmadev_request quoted above, you
> refer to the same pl330_filter filter function, but the argument there
> is a pointer to 'enum dma_ch', which is not compatible with any of
> the methods you list, neither the dma_pl330_filter_args nor the
> raw property.
>
> Also, if you change the calling conventions for the pl330_filter
> function, you should change both the caller and the function in the
> same patch.

In none of my patches I have changed the pl330_filter args.  This
function always takes the same argument void*. In non-DT case 'enum
dma_ch' was typecasted to void* and in DT case I am passing a pointer
to dma_pl330_filter_args and in pl330_filter function they are
converted back. In both cases it finally comes down to
dma_request_channel which takes them as void* which in turn calls the
pl330_filter.

I think this is what you are pointing to. Please let me know if I am
still wrong :( .

>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thanks
Padma

Re: [PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Arnd Bergmann]

On Tuesday 05 February 2013, Padma Venkat wrote:
> In none of my patches I have changed the pl330_filter args.  This
> function always takes the same argument void*. In non-DT case 'enum
> dma_ch' was typecasted to void* and in DT case I am passing a pointer
> to dma_pl330_filter_args and in pl330_filter function they are
> converted back. In both cases it finally comes down to
> dma_request_channel which takes them as void* which in turn calls the
> pl330_filter.
> 
> I think this is what you are pointing to. Please let me know if I am
> still wrong :( .

I think I see the misunderstanding now. The pl330_filter function
you have actually interprets the void* argument differently, based
on whether the pl330 device was instantiated from device tree or not.
I failed to see this, and you are probably right that this works
correctly.

It is however a rather unusual interface, and it would be safer and
easier to understand if you used separate filter functions for
the two cases, like this:

bool pl330_filter(struct dma_chan *chan, void *param)
{
        u8 *peri_id;

        if (chan->device->dev->driver != &pl330_driver.drv)
                return false;

        peri_id = chan->private;
        return *peri_id == (unsigned)param;
}
EXPORT_SYMBOL(pl330_filter);

static bool pl330_dt_filter(struct dma_chan *chan, void *param)
{
        struct dma_pl330_filter_args *fargs = param;

        if (chan->device != &fargs->pdmac->ddma)
                return false;

        return (chan->chan_id == fargs->chan_id);
}

So this is not a correctness issue, but more one of readability. I would
assume however that if I was misunderstanding the code, the next person
also wouldn't know what is going on here if you have one filter function
that performs two completely different tasks.

	Arnd

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Arnd Bergmann]

On Tuesday 05 February 2013, Padma Venkat wrote:
> In none of my patches I have changed the pl330_filter args.  This
> function always takes the same argument void*. In non-DT case 'enum
> dma_ch' was typecasted to void* and in DT case I am passing a pointer
> to dma_pl330_filter_args and in pl330_filter function they are
> converted back. In both cases it finally comes down to
> dma_request_channel which takes them as void* which in turn calls the
> pl330_filter.
> 
> I think this is what you are pointing to. Please let me know if I am
> still wrong :( .

I think I see the misunderstanding now. The pl330_filter function
you have actually interprets the void* argument differently, based
on whether the pl330 device was instantiated from device tree or not.
I failed to see this, and you are probably right that this works
correctly.

It is however a rather unusual interface, and it would be safer and
easier to understand if you used separate filter functions for
the two cases, like this:

bool pl330_filter(struct dma_chan *chan, void *param)
{
        u8 *peri_id;

        if (chan->device->dev->driver != &pl330_driver.drv)
                return false;

        peri_id = chan->private;
        return *peri_id == (unsigned)param;
}
EXPORT_SYMBOL(pl330_filter);

static bool pl330_dt_filter(struct dma_chan *chan, void *param)
{
        struct dma_pl330_filter_args *fargs = param;

        if (chan->device != &fargs->pdmac->ddma)
                return false;

        return (chan->chan_id == fargs->chan_id);
}

So this is not a correctness issue, but more one of readability. I would
assume however that if I was misunderstanding the code, the next person
also wouldn't know what is going on here if you have one filter function
that performs two completely different tasks.

	Arnd

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padmavathi Venna]

This patch removes the usage of DMACH_DT_PROP and dt_dmach_prop
from dma code as the new generic dma dt binding support has been
added.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-s3c24xx/include/mach/dma.h       |    1 -
 arch/arm/mach-s3c64xx/include/mach/dma.h       |    1 -
 arch/arm/plat-samsung/dma-ops.c                |   10 +---------
 arch/arm/plat-samsung/include/plat/dma-ops.h   |    1 -
 arch/arm/plat-samsung/include/plat/dma-pl330.h |    1 -
 5 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/include/mach/dma.h b/arch/arm/mach-s3c24xx/include/mach/dma.h
index 6b72d5a..b55da1d 100644
--- a/arch/arm/mach-s3c24xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c24xx/include/mach/dma.h
@@ -24,7 +24,6 @@
 */
 
 enum dma_ch {
-	DMACH_DT_PROP = -1,	/* not yet supported, do not use */
 	DMACH_XD0 = 0,
 	DMACH_XD1,
 	DMACH_SDI,
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 57b1ff4..fe1a98c 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -21,7 +21,6 @@
  */
 enum dma_ch {
 	/* DMA0/SDMA0 */
-	DMACH_DT_PROP = -1, /* not yet supported, do not use */
 	DMACH_UART0 = 0,
 	DMACH_UART0_SRC2,
 	DMACH_UART1,
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index 71d58dd..ec0d731 100644
--- a/arch/arm/plat-samsung/dma-ops.c
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
 				struct device *dev, char *ch_name)
 {
 	dma_cap_mask_t mask;
-	void *filter_param;
 
 	dma_cap_zero(mask);
 	dma_cap_set(param->cap, mask);
 
-	/*
-	 * If a dma channel property of a device node from device tree is
-	 * specified, use that as the fliter parameter.
-	 */
-	filter_param = (dma_ch == DMACH_DT_PROP) ?
-		(void *)param->dt_dmach_prop : (void *)dma_ch;
-
 	if (dev->of_node)
 		return (unsigned)dma_request_slave_channel(dev, ch_name);
 	else
 		return (unsigned)dma_request_channel(mask, pl330_filter,
-							filter_param);
+							(void *)dma_ch);
 }
 
 static int samsung_dmadev_release(unsigned ch, void *param)
diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h
index 1141782..ce6d763 100644
--- a/arch/arm/plat-samsung/include/plat/dma-ops.h
+++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
@@ -18,7 +18,6 @@
 
 struct samsung_dma_req {
 	enum dma_transaction_type cap;
-	struct property *dt_dmach_prop;
 	struct s3c2410_dma_client *client;
 };
 
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index d384a80..abe07fa 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -21,7 +21,6 @@
  * use these just as IDs.
  */
 enum dma_ch {
-	DMACH_DT_PROP = -1,
 	DMACH_UART0_RX = 0,
 	DMACH_UART0_TX,
 	DMACH_UART1_RX,
-- 
1.7.4.4

[PATCH] ARM: SAMSUNG: dma: Remove unnecessary code

[Padmavathi Venna]

This patch removes the usage of DMACH_DT_PROP and dt_dmach_prop
from dma code as the new generic dma dt binding support has been
added.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-s3c24xx/include/mach/dma.h       |    1 -
 arch/arm/mach-s3c64xx/include/mach/dma.h       |    1 -
 arch/arm/plat-samsung/dma-ops.c                |   10 +---------
 arch/arm/plat-samsung/include/plat/dma-ops.h   |    1 -
 arch/arm/plat-samsung/include/plat/dma-pl330.h |    1 -
 5 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/include/mach/dma.h b/arch/arm/mach-s3c24xx/include/mach/dma.h
index 6b72d5a..b55da1d 100644
--- a/arch/arm/mach-s3c24xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c24xx/include/mach/dma.h
@@ -24,7 +24,6 @@
 */
 
 enum dma_ch {
-	DMACH_DT_PROP = -1,	/* not yet supported, do not use */
 	DMACH_XD0 = 0,
 	DMACH_XD1,
 	DMACH_SDI,
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 57b1ff4..fe1a98c 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -21,7 +21,6 @@
  */
 enum dma_ch {
 	/* DMA0/SDMA0 */
-	DMACH_DT_PROP = -1, /* not yet supported, do not use */
 	DMACH_UART0 = 0,
 	DMACH_UART0_SRC2,
 	DMACH_UART1,
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index 71d58dd..ec0d731 100644
--- a/arch/arm/plat-samsung/dma-ops.c
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
 				struct device *dev, char *ch_name)
 {
 	dma_cap_mask_t mask;
-	void *filter_param;
 
 	dma_cap_zero(mask);
 	dma_cap_set(param->cap, mask);
 
-	/*
-	 * If a dma channel property of a device node from device tree is
-	 * specified, use that as the fliter parameter.
-	 */
-	filter_param = (dma_ch == DMACH_DT_PROP) ?
-		(void *)param->dt_dmach_prop : (void *)dma_ch;
-
 	if (dev->of_node)
 		return (unsigned)dma_request_slave_channel(dev, ch_name);
 	else
 		return (unsigned)dma_request_channel(mask, pl330_filter,
-							filter_param);
+							(void *)dma_ch);
 }
 
 static int samsung_dmadev_release(unsigned ch, void *param)
diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h
index 1141782..ce6d763 100644
--- a/arch/arm/plat-samsung/include/plat/dma-ops.h
+++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
@@ -18,7 +18,6 @@
 
 struct samsung_dma_req {
 	enum dma_transaction_type cap;
-	struct property *dt_dmach_prop;
 	struct s3c2410_dma_client *client;
 };
 
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index d384a80..abe07fa 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -21,7 +21,6 @@
  * use these just as IDs.
  */
 enum dma_ch {
-	DMACH_DT_PROP = -1,
 	DMACH_UART0_RX = 0,
 	DMACH_UART0_TX,
 	DMACH_UART1_RX,
-- 
1.7.4.4