| Message ID | 1231944284-18296-1-git-send-email-jacmet@sunsite.dk (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Kumar Gala |
| Headers | show |
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" == 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().
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" == 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.
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" == 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.
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 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
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;
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(-)