dw_dmac: few cleanups to the driver

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/dma/dw_dmac.c +0 -0

index c27c125..60b172a 100644

drivers/dma/dw_dmac.c +0 -0

 	dma_async_device_register(&dw->dma);
 

Signature · 1 line

1.7.10.4

Hi,

Quoted 11 lines · Andy Shevchenko

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/dma/dw_dmac.c +0 -0

index c27c125..60b172a 100644
@@ -456,9 +456,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 

drivers/dma/dw_dmac.c static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 460

Quoted 7 lines

drivers/dma/dw_dmac.c +2 -3

 static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
 {
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
-			lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
+	dev_crit(chan2dev(&dwc->chan), "  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
+		 lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);

is this really critical ? To me it looks more like a debugging message.

Quoted 5 lines

drivers/dma/dw_dmac.c +0 -0

 }
 
 static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 9 lines

drivers/dma/dw_dmac.c +2 -4

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

Quoted 8 lines

drivers/dma/dw_dmac.c +0 -0

 	dwc_dump_lli(dwc, &bad_desc->lli);
 	list_for_each_entry(child, &bad_desc->tx_list, desc_node)
 		dwc_dump_lli(dwc, &child->lli);

@@ -1625,8 +1622,8 @@ static int __devinit dw_probe(struct platform_device *pdev)
 
 	dma_writel(dw, CFG, DW_CFG_DMA_EN);
 

drivers/dma/dw_dmac.c static int __devinit dw_probe(struct platform_device *pdev) · line 1626

Quoted 4 lines

drivers/dma/dw_dmac.c +2 -2

-	printk(KERN_INFO "%s: DesignWare DMA Controller, %d channels\n",
-			dev_name(&pdev->dev), nr_channels);
+	pr_info("%s: DesignWare DMA Controller, %d channels\n",
+		dev_name(&pdev->dev), nr_channels);

you have a struct device available in platform_device, please use dev_info() or dev_dbg().

Quoted 14 lines · Felipe Balbi

Hi,

Quoted 11 lines · Andy Shevchenko

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/dma/dw_dmac.c +0 -0

index c27c125..60b172a 100644
@@ -456,9 +456,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 

Quoted 9 lines

drivers/dma/dw_dmac.c static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 460

Quoted 7 lines

drivers/dma/dw_dmac.c +2 -3

 static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
 {
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
-			lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
+	dev_crit(chan2dev(&dwc->chan), "  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
+		 lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);

is this really critical ? To me it looks more like a debugging message.

This one is used in two cases, where one is marked as "error", another - "critical"

Quoted 6 lines

Quoted 6 lines

drivers/dma/dw_dmac.c +0 -1

 static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),

Quoted 8 lines

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 5 lines

drivers/dma/dw_dmac.c +2 -3

-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

To this and upper comment, there is an explanation why it's critical. I guess the WARN_ONCE is not good enough, for example if we have more than one user making such noise.

Quoted 18 lines · Andy Shevchenko

Quoted 17 lines · Felipe Balbi

Hi,

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 12 lines

drivers/dma/dw_dmac.c +2 -4

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

Quoted 3 lines

To this and upper comment, there is an explanation why it's critical. I guess the WARN_ONCE is not good enough, for example if we have more than one user making such noise.

then use dev_WARN()

Quoted 19 lines · Felipe Balbi

Quoted 18 lines · Andy Shevchenko

Quoted 17 lines · Felipe Balbi

Hi,

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 12 lines

drivers/dma/dw_dmac.c +2 -4

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

Quoted 5 lines

Quoted 3 lines

To this and upper comment, there is an explanation why it's critical. I guess the WARN_ONCE is not good enough, for example if we have more than one user making such noise.

then use dev_WARN()

I can't see how dev_WARN could be more useful here than the dev_crit. In current message we have channel and cookie to link back to the user. What does WARN add meaningful?

Quoted 23 lines · Andy Shevchenko

Quoted 22 lines · Felipe Balbi

Quoted 20 lines · Andy Shevchenko

Quoted 15 lines · Felipe Balbi

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 12 lines

drivers/dma/dw_dmac.c +2 -4

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

To this and upper comment, there is an explanation why it's critical. I guess the WARN_ONCE is not good enough, for example if we have more than one user making such noise.

Quoted 4 lines

Quoted 1 line

then use dev_WARN()

I can't see how dev_WARN could be more useful here than the dev_crit. In current message we have channel and cookie to link back to the user. What does WARN add meaningful?

a dump_stack()

Quoted 29 lines · Felipe Balbi

Quoted 27 lines · Andy Shevchenko

Quoted 18 lines · Felipe Balbi

Quoted 17 lines · Andy Shevchenko

Quoted 15 lines · Felipe Balbi

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 12 lines

drivers/dma/dw_dmac.c +2 -4

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

Quoted 5 lines

Quoted 3 lines

To this and upper comment, there is an explanation why it's critical. I guess the WARN_ONCE is not good enough, for example if we have more than one user making such noise.

then use dev_WARN()

I can't see how dev_WARN could be more useful here than the dev_crit. In current message we have channel and cookie to link back to the user. What does WARN add meaningful?

Quoted 1 line

a dump_stack()

How could it be useful? The dwc_handle_error is called from a tasklet that is called from scheduler asynchronously. The tasklet is queued in interrupt handler.

Quoted 32 lines · Andy Shevchenko

Quoted 30 lines · Felipe Balbi

Quoted 19 lines · Andy Shevchenko

Quoted 18 lines · Felipe Balbi

Quoted 17 lines · Andy Shevchenko

Quoted 15 lines · Felipe Balbi

drivers/dma/dw_dmac.c static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc) · line 495

Quoted 12 lines

drivers/dma/dw_dmac.c +2 -4

@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
   * controller flagged an error instead of scribbling over
   * random memory locations.
   */
- dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-                 "Bad descriptor submitted for DMA!\n");
- dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-                 "  cookie: %d\n", bad_desc->txd.cookie);
+ dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+ dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so that it's noisy enough to catch the failing user.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

To this and upper comment, there is an explanation why it's critical. I guess the WARN_ONCE is not good enough, for example if we have more than one user making such noise.

then use dev_WARN()

I can't see how dev_WARN could be more useful here than the dev_crit. In current message we have channel and cookie to link back to the user. What does WARN add meaningful?

a dump_stack()

Quoted 3 lines

How could it be useful? The dwc_handle_error is called from a tasklet that is called from scheduler asynchronously. The tasklet is queued in interrupt handler.

even if it's not useful, it's a lot more verbose and more likely to get user's attention. If someone's passing broken DMA descriptor, it should be a really big fat warning so we can get user reports early enoough.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/dma/dw_dmac.c +2 -2

index 60b172a..7d2b438 100644
@@ -1653,7 +1653,7 @@ static void dw_shutdown(struct platform_device *pdev)
 {
 	struct dw_dma	*dw = platform_get_drvdata(pdev);
 
-	dw_dma_off(platform_get_drvdata(pdev));
+	dw_dma_off(dw);
 	clk_disable_unprepare(dw->clk);
 }
 

@@ -1662,7 +1662,7 @@ static int dw_suspend_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_dma	*dw = platform_get_drvdata(pdev);
 
-	dw_dma_off(platform_get_drvdata(pdev));
+	dw_dma_off(dw);
 	clk_disable_unprepare(dw->clk);
 
 	return 0;

Signature · 1 line

1.7.10.4

On Wed, Oct 17, 2012 at 01:31:18PM +0300, Andy Shevchenko wrote:

I would suggest adding a commit log, but if maintainer is fine without, I'm fine too :-p