[PATCH] fsldma: print correct IRQ on mpc83xx

[PATCH] fsldma: print correct IRQ on mpc83xx

[Peter Korsgaard]

The mpc83xx variant uses a shared IRQ for all channels, so the individual
channel nodes don't have an interrupt property. Fix the code to print the
controller IRQ instead if there isn't any for the channel.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 drivers/dma/fsldma.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index ca70a21..61b6e08 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -875,7 +875,8 @@ static int __devinit fsl_dma_chan_probe(struct fsl_dma_device *fdev,
 	}
 
 	dev_info(fdev->dev, "#%d (%s), irq %d\n", new_fsl_chan->id,
-				compatible, new_fsl_chan->irq);
+		 compatible,
+		 new_fsl_chan->irq != NO_IRQ ? new_fsl_chan->irq : fdev->irq);
 
 	return 0;
 
-- 
1.5.6.5

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Timur Tabi]

Peter Korsgaard wrote:

> @@ -875,7 +875,8 @@ static int __devinit fsl_dma_chan_probe(struct fsl_dma_device *fdev,
>  	}
>  
>  	dev_info(fdev->dev, "#%d (%s), irq %d\n", new_fsl_chan->id,
> -				compatible, new_fsl_chan->irq);
> +		 compatible,
> +		 new_fsl_chan->irq != NO_IRQ ? new_fsl_chan->irq : fdev->irq);

Wouldn't it be better to make sure that, on 83xx, new_fsl_chan->irq has the same
value as fdev->irq before we get here?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Peter Korsgaard]

>>>>> "Timur" == Timur Tabi <timur@freescale.com> writes:

 Timur> Peter Korsgaard wrote:
 >> @@ -875,7 +875,8 @@ static int __devinit fsl_dma_chan_probe(struct fsl_dma_device *fdev,
 >> }
 >> 
 >> dev_info(fdev->dev, "#%d (%s), irq %d\n", new_fsl_chan->id,
 >> -				compatible, new_fsl_chan->irq);
 >> +		 compatible,
 >> +		 new_fsl_chan->irq != NO_IRQ ? new_fsl_chan->irq : fdev->irq);

 Timur> Wouldn't it be better to make sure that, on 83xx,
 Timur> new_fsl_chan->irq has the same value as fdev->irq before we
 Timur> get here?

Why? Does it buy us anything to request_irq again for each channel?

Now we're at it, it seems like there's a check for != NO_IRQ missing
in fsl_dma_chan_remove().

-- 
Bye, Peter Korsgaard

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Timur Tabi]

Peter Korsgaard wrote:

> Why? Does it buy us anything to request_irq again for each channel?
> 
> Now we're at it, it seems like there's a check for != NO_IRQ missing
> in fsl_dma_chan_remove().

If the device tree doesn't specify an interrupts property in the device channel,
then I think that's an error.  All the device trees already do that.  So where
do you see this problem?


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Peter Korsgaard]

>>>>> "Timur" == Timur Tabi <timur@freescale.com> writes:

 Timur> Peter Korsgaard wrote:
 >> Why? Does it buy us anything to request_irq again for each channel?
 >> 
 >> Now we're at it, it seems like there's a check for != NO_IRQ missing
 >> in fsl_dma_chan_remove().

 Timur> If the device tree doesn't specify an interrupts property in
 Timur> the device channel, then I think that's an error.  All the
 Timur> device trees already do that.  So where do you see this
 Timur> problem?

Documentation/powerpc/dts-bindings/fsl/dma.txt and
The NO_IRQ check in fsldma.c:fsl_dma_chan_probe()

And it makes sense, there's no per-channel DMAC interrupts on mpc83xx.

-- 
Bye, Peter Korsgaard

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Timur Tabi]

Peter Korsgaard wrote:

> Documentation/powerpc/dts-bindings/fsl/dma.txt and
> The NO_IRQ check in fsldma.c:fsl_dma_chan_probe()
> 
> And it makes sense, there's no per-channel DMAC interrupts on mpc83xx.

But the device trees do have IRQs in the channels:

arch/powerpc/boot/dts/mpc836x_mds.dts:

dma@82a8 {
	...
	interrupt-parent = <&ipic>;
	interrupts = <71 8>;
	dma-channel@0 {
		compatible = "fsl,mpc8360-dma-channel", "fsl,elo-dma-channel";
		reg = <0 0x80>;
		cell-index = <0>;
		interrupt-parent = <&ipic>;
		interrupts = <71 8>;
	};
	...

So I don't see what bug you're trying to fix.  If you're saying that the
interrupts should not be specific DMA channels, then the proper fix is to have
the DMA driver pick up the interrupts from the DMA controller's node when
necessary.  Hacking up a printk doesn't fix anything.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Peter Korsgaard]

>>>>> "Timur" == Timur Tabi <timur@freescale.com> writes:

 Timur> Peter Korsgaard wrote:
 >> Documentation/powerpc/dts-bindings/fsl/dma.txt and
 >> The NO_IRQ check in fsldma.c:fsl_dma_chan_probe()
 >> 
 >> And it makes sense, there's no per-channel DMAC interrupts on mpc83xx.

 Timur> But the device trees do have IRQs in the channels:

 Timur> arch/powerpc/boot/dts/mpc836x_mds.dts:

 Timur> So I don't see what bug you're trying to fix.  If you're
 Timur> saying that the interrupts should not be specific DMA
 Timur> channels, then the proper fix is to have the DMA driver pick
 Timur> up the interrupts from the DMA controller's node when
 Timur> necessary.  Hacking up a printk doesn't fix anything.

Ok, let me try again. SOME device trees indeed have an interrupt
property per channel, but the bindings
(Documentation/powerpc/dts-bindings/fsl/dma.txt) and the probe code in
fsldma.c doesn't require it.

The only minor issues we have is the cosmetic problem of the driver
printing NO_IRQ (E.G. this patch), and the driver calling
free_irq(NO_IRQ) (E.G. the other patch I sent today) in the release
path.

-- 
Bye, Peter Korsgaard

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Timur Tabi]

Peter Korsgaard wrote:

> The only minor issues we have is the cosmetic problem of the driver
> printing NO_IRQ (E.G. this patch), and the driver calling
> free_irq(NO_IRQ) (E.G. the other patch I sent today) in the release
> path.

Ok, I understand.

ACK on the patches.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Dan Williams]

Timur Tabi wrote:
> Peter Korsgaard wrote:
> 
>> The only minor issues we have is the cosmetic problem of the driver
>> printing NO_IRQ (E.G. this patch), and the driver calling
>> free_irq(NO_IRQ) (E.G. the other patch I sent today) in the release
>> path.
> 
> Ok, I understand.
> 
> ACK on the patches.
> 

Ok, Li Yang and Zhang Wei have been silent recently on fsldma patches. 
Applied to async_tx.git/fixes

Thanks,
Dan

Re: [PATCH] fsldma: print correct IRQ on mpc83xx

[Li Yang]

On Thu, Jan 15, 2009 at 2:17 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> Timur Tabi wrote:
>>
>> Peter Korsgaard wrote:
>>
>>> The only minor issues we have is the cosmetic problem of the driver
>>> printing NO_IRQ (E.G. this patch), and the driver calling
>>> free_irq(NO_IRQ) (E.G. the other patch I sent today) in the release
>>> path.
>>
>> Ok, I understand.
>>
>> ACK on the patches.
>>
>
> Ok, Li Yang and Zhang Wei have been silent recently on fsldma patches.
> Applied to async_tx.git/fixes

Sorry for the delay, was trapped by an upcoming release.

For the two patches from Peter,

Acked-by: Li Yang <leoli@freescale.com>

- Leo