[PATCH v3 1/3] dmaengine: shdma: modify the DMAC Address Error

[PATCH v3 1/3] dmaengine: shdma: modify the DMAC Address Error

[Shimoda, Yoshihiro]

The USB-DMAC/SUDMAC don't have the interrupt of DMAC Address Error.
So, only when the resource has a name and it is "error_irq", the driver
calls request_irq() for DMAC Address Error.

This patch is also useful for the generic DMAC which doesn't have
DMAC Address Error. So, we can get rid of the "CPU_SH4 || ARCH_SHMOBILE"
ifdefs.
This patch also changes the IRQF_DISABLED to 0.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 about v3:
  - changes the IRQF_DISABLED to 0.

 drivers/dma/shdma.c |   78 ++++++++++++++++++++++++++-------------------------
 1 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/drivers/dma/shdma.c b/drivers/dma/shdma.c
index 81809c2..55149ed 100644
--- a/drivers/dma/shdma.c
+++ b/drivers/dma/shdma.c
@@ -1149,12 +1149,12 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev)
 static int __init sh_dmae_probe(struct platform_device *pdev)
 {
 	struct sh_dmae_pdata *pdata = pdev->dev.platform_data;
-	unsigned long irqflags = IRQF_DISABLED,
+	unsigned long irqflags = 0,
 		chan_flag[SH_DMAC_MAX_CHANNELS] = {};
-	int errirq, chan_irq[SH_DMAC_MAX_CHANNELS];
+	int errirq = 0, chan_irq[SH_DMAC_MAX_CHANNELS];
 	int err, i, irq_cnt = 0, irqres = 0, irq_cap = 0;
 	struct sh_dmae_device *shdev;
-	struct resource *chan, *dmars, *errirq_res, *chanirq_res;
+	struct resource *chan, *dmars, *errirq_res, *irq_res, *chanirq_res;

 	/* get platform data */
 	if (!pdata || !pdata->channel_num)
@@ -1179,8 +1179,8 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
 	 *    specify IORESOURCE_IRQ_SHAREABLE in their resources, they will be
 	 *    requested with the IRQF_SHARED flag
 	 */
-	errirq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!chan || !errirq_res)
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!chan || !irq_res)
 		return -ENODEV;

 	if (!request_mem_region(chan->start, resource_size(chan), pdev->name)) {
@@ -1258,33 +1258,35 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
 	/* Default transfer size of 32 bytes requires 32-byte alignment */
 	shdev->common.copy_align = LOG2_DEFAULT_XFER_SIZE;

-#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
-	chanirq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
-
-	if (!chanirq_res)
-		chanirq_res = errirq_res;
-	else
-		irqres++;
-
-	if (chanirq_res = errirq_res ||
-	    (errirq_res->flags & IORESOURCE_BITS) = IORESOURCE_IRQ_SHAREABLE)
-		irqflags = IRQF_SHARED;
-
-	errirq = errirq_res->start;
-
-	err = request_irq(errirq, sh_dmae_err, irqflags,
-			  "DMAC Address Error", shdev);
-	if (err) {
-		dev_err(&pdev->dev,
-			"DMA failed requesting irq #%d, error %d\n",
-			errirq, err);
-		goto eirq_err;
+	errirq_res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
+						  "error_irq");
+	if (errirq_res) {
+		chanirq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+		if (!chanirq_res)
+			chanirq_res = errirq_res;
+		else
+			irqres++;
+
+		if (chanirq_res = errirq_res ||
+		    (errirq_res->flags & IORESOURCE_BITS) =
+		    IORESOURCE_IRQ_SHAREABLE)
+			irqflags = IRQF_SHARED;
+
+		errirq = errirq_res->start;
+
+		err = request_irq(errirq, sh_dmae_err, irqflags,
+				  "DMAC Address Error", shdev);
+		if (err) {
+			dev_err(&pdev->dev,
+				"DMA failed requesting irq #%d, error %d\n",
+				errirq, err);
+			goto eirq_err;
+		}
+	} else {
+		chanirq_res = irq_res;
 	}

-#else
-	chanirq_res = errirq_res;
-#endif /* CONFIG_CPU_SH4 || CONFIG_ARCH_SHMOBILE */
-
 	if (chanirq_res->start = chanirq_res->end &&
 	    !platform_get_resource(pdev, IORESOURCE_IRQ, 1)) {
 		/* Special case - all multiplexed */
@@ -1305,11 +1307,11 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
 					break;
 				}

-				if ((errirq_res->flags & IORESOURCE_BITS) =
+				if ((irq_res->flags & IORESOURCE_BITS) =
 				    IORESOURCE_IRQ_SHAREABLE)
 					chan_flag[irq_cnt] = IRQF_SHARED;
 				else
-					chan_flag[irq_cnt] = IRQF_DISABLED;
+					chan_flag[irq_cnt] = 0;
 				dev_dbg(&pdev->dev,
 					"Found IRQ %d for channel %d\n",
 					i, irq_cnt);
@@ -1345,10 +1347,9 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
 chan_probe_err:
 	sh_dmae_chan_remove(shdev);

-#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
-	free_irq(errirq, shdev);
+	if (errirq_res)
+		free_irq(errirq, shdev);
 eirq_err:
-#endif
 rst_err:
 	spin_lock_irq(&sh_dmae_lock);
 	list_del_rcu(&shdev->node);
@@ -1379,12 +1380,13 @@ static int __exit sh_dmae_remove(struct platform_device *pdev)
 {
 	struct sh_dmae_device *shdev = platform_get_drvdata(pdev);
 	struct resource *res;
-	int errirq = platform_get_irq(pdev, 0);
+	struct resource *errirq_res = platform_get_resource_byname(pdev,
+						IORESOURCE_IRQ, "error_irq");

 	dma_async_device_unregister(&shdev->common);

-	if (errirq > 0)
-		free_irq(errirq, shdev);
+	if (errirq_res)
+		free_irq(errirq_res->start, shdev);

 	spin_lock_irq(&sh_dmae_lock);
 	list_del_rcu(&shdev->node);
-- 
1.7.1

Re: [PATCH v3 1/3] dmaengine: shdma: modify the DMAC Address Error registration

[Paul Mundt]

On Tue, Jan 10, 2012 at 03:38:13PM +0900, Shimoda, Yoshihiro wrote:
> The USB-DMAC/SUDMAC don't have the interrupt of DMAC Address Error.
> So, only when the resource has a name and it is "error_irq", the driver
> calls request_irq() for DMAC Address Error.
> 
> This patch is also useful for the generic DMAC which doesn't have
> DMAC Address Error. So, we can get rid of the "CPU_SH4 || ARCH_SHMOBILE"
> ifdefs.
> This patch also changes the IRQF_DISABLED to 0.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  about v3:
>   - changes the IRQF_DISABLED to 0.
> 
>  drivers/dma/shdma.c |   78 ++++++++++++++++++++++++++-------------------------
>  1 files changed, 40 insertions(+), 38 deletions(-)
> 
The updated patches look fine to me, thanks for fixing them up. I'll let
Vinod pick them up.

Acked-by: Paul Mundt <lethal@linux-sh.org>

Re: [PATCH v3 1/3] dmaengine: shdma: modify the DMAC Address Error

[Guennadi Liakhovetski]

Hello Shimoda-san

Thanks for your work on this! Nice to see patches improving!

On Tue, 10 Jan 2012, Shimoda, Yoshihiro wrote:

> The USB-DMAC/SUDMAC don't have the interrupt of DMAC Address Error.
> So, only when the resource has a name and it is "error_irq", the driver
> calls request_irq() for DMAC Address Error.
> 
> This patch is also useful for the generic DMAC which doesn't have
> DMAC Address Error. So, we can get rid of the "CPU_SH4 || ARCH_SHMOBILE"
> ifdefs.
> This patch also changes the IRQF_DISABLED to 0.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  about v3:
>   - changes the IRQF_DISABLED to 0.
> 
>  drivers/dma/shdma.c |   78 ++++++++++++++++++++++++++-------------------------
>  1 files changed, 40 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/dma/shdma.c b/drivers/dma/shdma.c
> index 81809c2..55149ed 100644
> --- a/drivers/dma/shdma.c
> +++ b/drivers/dma/shdma.c
> @@ -1149,12 +1149,12 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev)
>  static int __init sh_dmae_probe(struct platform_device *pdev)
>  {
>  	struct sh_dmae_pdata *pdata = pdev->dev.platform_data;
> -	unsigned long irqflags = IRQF_DISABLED,
> +	unsigned long irqflags = 0,
>  		chan_flag[SH_DMAC_MAX_CHANNELS] = {};
> -	int errirq, chan_irq[SH_DMAC_MAX_CHANNELS];
> +	int errirq = 0, chan_irq[SH_DMAC_MAX_CHANNELS];
>  	int err, i, irq_cnt = 0, irqres = 0, irq_cap = 0;
>  	struct sh_dmae_device *shdev;
> -	struct resource *chan, *dmars, *errirq_res, *chanirq_res;
> +	struct resource *chan, *dmars, *errirq_res, *irq_res, *chanirq_res;
> 
>  	/* get platform data */
>  	if (!pdata || !pdata->channel_num)
> @@ -1179,8 +1179,8 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
>  	 *    specify IORESOURCE_IRQ_SHAREABLE in their resources, they will be
>  	 *    requested with the IRQF_SHARED flag
>  	 */
> -	errirq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> -	if (!chan || !errirq_res)
> +	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!chan || !irq_res)
>  		return -ENODEV;
> 
>  	if (!request_mem_region(chan->start, resource_size(chan), pdev->name)) {
> @@ -1258,33 +1258,35 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
>  	/* Default transfer size of 32 bytes requires 32-byte alignment */
>  	shdev->common.copy_align = LOG2_DEFAULT_XFER_SIZE;
> 
> -#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
> -	chanirq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
> -
> -	if (!chanirq_res)
> -		chanirq_res = errirq_res;
> -	else
> -		irqres++;
> -
> -	if (chanirq_res = errirq_res ||
> -	    (errirq_res->flags & IORESOURCE_BITS) = IORESOURCE_IRQ_SHAREABLE)
> -		irqflags = IRQF_SHARED;
> -
> -	errirq = errirq_res->start;
> -
> -	err = request_irq(errirq, sh_dmae_err, irqflags,
> -			  "DMAC Address Error", shdev);
> -	if (err) {
> -		dev_err(&pdev->dev,
> -			"DMA failed requesting irq #%d, error %d\n",
> -			errirq, err);
> -		goto eirq_err;
> +	errirq_res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> +						  "error_irq");

Wouldn't this break all platforms, providing error IRQ, because they don't 
yet have any IRQ resources with that name? Also, I think, it would be good 
to define macros for DMAC IRQ names, similar to SH_MOBILE_SDHI_IRQ_* 
macros in include/linux/mmc/sh_mobile_sdhi.h. To avoid breakages you, 
probably, want to

1. add IRQ resource name definition macros to the header
2. add IRQ resource names to platforms. This shouldn't break anything, you 
still can request those IRQs without name.
3. convert the driver. Here I would try to think of a scheme, where we 
wouldn't have to mix named and unnamed IRQ resources. So, maybe use all of 
them as named. So, how about

3.1. Request both "error irq" and "channel irq" IRQ resources
3.2. If none is supplied - error out
3.3. If an error IRQ is provided - request it
3.4. If no separate channel IRQs are provided - request error IRQ 
additional times, according to the channel count
3.5. If separate channel IRQs are provided - request them just like now

Would this make sense?

Thanks
Guennadi

> +	if (errirq_res) {
> +		chanirq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
> +
> +		if (!chanirq_res)
> +			chanirq_res = errirq_res;
> +		else
> +			irqres++;
> +
> +		if (chanirq_res = errirq_res ||
> +		    (errirq_res->flags & IORESOURCE_BITS) =
> +		    IORESOURCE_IRQ_SHAREABLE)
> +			irqflags = IRQF_SHARED;
> +
> +		errirq = errirq_res->start;
> +
> +		err = request_irq(errirq, sh_dmae_err, irqflags,
> +				  "DMAC Address Error", shdev);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"DMA failed requesting irq #%d, error %d\n",
> +				errirq, err);
> +			goto eirq_err;
> +		}
> +	} else {
> +		chanirq_res = irq_res;
>  	}
> 
> -#else
> -	chanirq_res = errirq_res;
> -#endif /* CONFIG_CPU_SH4 || CONFIG_ARCH_SHMOBILE */
> -
>  	if (chanirq_res->start = chanirq_res->end &&
>  	    !platform_get_resource(pdev, IORESOURCE_IRQ, 1)) {
>  		/* Special case - all multiplexed */
> @@ -1305,11 +1307,11 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
>  					break;
>  				}
> 
> -				if ((errirq_res->flags & IORESOURCE_BITS) =
> +				if ((irq_res->flags & IORESOURCE_BITS) =
>  				    IORESOURCE_IRQ_SHAREABLE)
>  					chan_flag[irq_cnt] = IRQF_SHARED;
>  				else
> -					chan_flag[irq_cnt] = IRQF_DISABLED;
> +					chan_flag[irq_cnt] = 0;
>  				dev_dbg(&pdev->dev,
>  					"Found IRQ %d for channel %d\n",
>  					i, irq_cnt);
> @@ -1345,10 +1347,9 @@ static int __init sh_dmae_probe(struct platform_device *pdev)
>  chan_probe_err:
>  	sh_dmae_chan_remove(shdev);
> 
> -#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
> -	free_irq(errirq, shdev);
> +	if (errirq_res)
> +		free_irq(errirq, shdev);
>  eirq_err:
> -#endif
>  rst_err:
>  	spin_lock_irq(&sh_dmae_lock);
>  	list_del_rcu(&shdev->node);
> @@ -1379,12 +1380,13 @@ static int __exit sh_dmae_remove(struct platform_device *pdev)
>  {
>  	struct sh_dmae_device *shdev = platform_get_drvdata(pdev);
>  	struct resource *res;
> -	int errirq = platform_get_irq(pdev, 0);
> +	struct resource *errirq_res = platform_get_resource_byname(pdev,
> +						IORESOURCE_IRQ, "error_irq");
> 
>  	dma_async_device_unregister(&shdev->common);
> 
> -	if (errirq > 0)
> -		free_irq(errirq, shdev);
> +	if (errirq_res)
> +		free_irq(errirq_res->start, shdev);
> 
>  	spin_lock_irq(&sh_dmae_lock);
>  	list_del_rcu(&shdev->node);
> -- 
> 1.7.1
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v3 1/3] dmaengine: shdma: modify the DMAC Address Error

[Shimoda, Yoshihiro]

Hello Guennadi-san,

Thank you very much for your review!

2012/01/10 18:48, Guennadi Liakhovetski wrote:
> Hello Shimoda-san
> 
> Thanks for your work on this! Nice to see patches improving!
> 
> On Tue, 10 Jan 2012, Shimoda, Yoshihiro wrote:
> 
>> +	errirq_res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
>> +						  "error_irq");
> 
> Wouldn't this break all platforms, providing error IRQ, because they don't 
> yet have any IRQ resources with that name? Also, I think, it would be good 
> to define macros for DMAC IRQ names, similar to SH_MOBILE_SDHI_IRQ_* 
> macros in include/linux/mmc/sh_mobile_sdhi.h. To avoid breakages you, 
> probably, want to
> 
> 1. add IRQ resource name definition macros to the header
> 2. add IRQ resource names to platforms. This shouldn't break anything, you 
> still can request those IRQs without name.
> 3. convert the driver. Here I would try to think of a scheme, where we 
> wouldn't have to mix named and unnamed IRQ resources. So, maybe use all of 
> them as named. So, how about
> 
> 3.1. Request both "error irq" and "channel irq" IRQ resources
> 3.2. If none is supplied - error out
> 3.3. If an error IRQ is provided - request it
> 3.4. If no separate channel IRQs are provided - request error IRQ 
> additional times, according to the channel count
> 3.5. If separate channel IRQs are provided - request them just like now
> 
> Would this make sense?

I think that it is very nice.
I will write such a code.

Best regards,
Yoshihiro Shimoda

> 
> Thanks
> Guennadi