Re: [PATCH] dmaengine: rcar-dmac: clear channel register when error
Hi Laurent
> > // This patch started to use C++ style comment out
> > // because it is recent Linus request
>
> I fear this will generate lots of frustration :-/ While I strongly prefer the
> traditional C style, I'm fine leaving the choice to driver authors. However,
> I
> think that mixing different styles in the same file only hinders readability.
> Would you like to convert the whole file ? ;-)
I already posted v2 patch ;P
> > chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> > if (chcr & RCAR_DMACHCR_CAE) {
> > - rcar_dmac_chan_halt(chan);
> > + struct rcar_dmac *dmac =
> > dev_get_drvdata(chan->chan.device->dev);
>
> This could be simplified with
>
> struct rcar_dmac *dmac = to_rcar_dmac(chan->chan.device);
Nice to know.
Will post v3 patch
Best regards
---
Kuninori Morimoto
Re: [PATCH] dmaengine: rcar-dmac: clear channel register when error
Hi Morimoto-san,
Thank you for the patch.
On Monday, 2 July 2018 04:07:17 EEST Kuninori Morimoto wrote:
> From: Kuninori Morimoto
>
> We need to clear channel register in error case as recovery.
> The channel is already stopped in such case, thus we don't need to call
> rcar_dmac_chan_halt() before clearing.
>
> rcar_dmac_chan_halt() will clear register and confirm DE bit.
> But it will be failed because channel is already stopped in error case.
> In other words, we shouldn't call it then.
>
> // This patch started to use C++ style comment out
> // because it is recent Linus request
I fear this will generate lots of frustration :-/ While I strongly prefer the
traditional C style, I'm fine leaving the choice to driver authors. However, I
think that mixing different styles in the same file only hinders readability.
Would you like to convert the whole file ? ;-)
> Reported-by: Hiroki Negishi
> Signed-off-by: Kuninori Morimoto
> Reviewed-by: Hiroki Negishi
> ---
> drivers/dma/sh/rcar-dmac.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 279c930..35d7a16 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1525,7 +1525,13 @@ static irqreturn_t rcar_dmac_isr_channel(int irq,
> void *dev)
>
> chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> if (chcr & RCAR_DMACHCR_CAE) {
> - rcar_dmac_chan_halt(chan);
> + struct rcar_dmac *dmac =
> dev_get_drvdata(chan->chan.device->dev);
This could be simplified with
struct rcar_dmac *dmac = to_rcar_dmac(chan->chan.device);
> +
> + // We don't need to call rcar_dmac_chan_halt()
> + // because channel is already stopped in error case.
> + // We need to clear register and check DE bit as recovery.
Is it also a request from Linus to wrap text much before the 80 characters
limit ? :-)
> + rcar_dmac_write(dmac, RCAR_DMACHCLR, 1 << chan->index);
> + rcar_dmac_chcr_de_barrier(chan);
> reinit = true;
> goto spin_lock_end;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH] dmaengine: rcar-dmac: clear channel register when error
Hi Fabio > > // This patch started to use C++ style comment out > > // because it is recent Linus request > > This is valid for the SPDX part only. > > For code you should still use the standard /* style. Wow ! Thanks, I will post v2 patch Best regards --- Kuninori Morimoto
Re: [PATCH] dmaengine: rcar-dmac: clear channel register when error
Hi Kuninori-san, On Sun, Jul 1, 2018 at 10:07 PM, Kuninori Morimoto wrote: > > From: Kuninori Morimoto > > We need to clear channel register in error case as recovery. > The channel is already stopped in such case, thus we don't need to call > rcar_dmac_chan_halt() before clearing. > > rcar_dmac_chan_halt() will clear register and confirm DE bit. > But it will be failed because channel is already stopped in error case. > In other words, we shouldn't call it then. > > // This patch started to use C++ style comment out > // because it is recent Linus request This is valid for the SPDX part only. For code you should still use the standard /* style.
[PATCH] dmaengine: rcar-dmac: clear channel register when error
From: Kuninori Morimoto
We need to clear channel register in error case as recovery.
The channel is already stopped in such case, thus we don't need to call
rcar_dmac_chan_halt() before clearing.
rcar_dmac_chan_halt() will clear register and confirm DE bit.
But it will be failed because channel is already stopped in error case.
In other words, we shouldn't call it then.
// This patch started to use C++ style comment out
// because it is recent Linus request
Reported-by: Hiroki Negishi
Signed-off-by: Kuninori Morimoto
Reviewed-by: Hiroki Negishi
---
drivers/dma/sh/rcar-dmac.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 279c930..35d7a16 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1525,7 +1525,13 @@ static irqreturn_t rcar_dmac_isr_channel(int irq, void
*dev)
chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
if (chcr & RCAR_DMACHCR_CAE) {
- rcar_dmac_chan_halt(chan);
+ struct rcar_dmac *dmac =
dev_get_drvdata(chan->chan.device->dev);
+
+ // We don't need to call rcar_dmac_chan_halt()
+ // because channel is already stopped in error case.
+ // We need to clear register and check DE bit as recovery.
+ rcar_dmac_write(dmac, RCAR_DMACHCLR, 1 << chan->index);
+ rcar_dmac_chcr_de_barrier(chan);
reinit = true;
goto spin_lock_end;
}
--
2.7.4
