dw_dmac: split the driver and introduce PCI part

PATCH In review Andy Shevchenko 6 participants 46 messages Sep 27, 2012 – Oct 30, 2012

Subsystems (2)

dmaengine Synopsys DesignWare DMAC driver

lore

Share & tools

lore

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #0 lore raw

v3 cover dw_dmac: split the driver and introduce PCI part

This patchset is dedicated to support different platform devices via the same core driver. In our case the dw_dmac could be used as a PCI device, regular embedded device or something else. This split allows to support the controller connected to any bus by adding a little piece of code without duplicating a core driver functionality.

Since v2:
- use git format-patch -C -M to avoid long useless patches
- substitute DRIVER macro to a normal structure definition in the dw_dmac_pci.c
- split patch 1/4 to few independent pieces with their own descriptions

Andy Shevchenko (2):
dma: move dw_dmac driver to an own directory
MAINTAINERS: add recently created files to dw_dmac section

Heikki Krogerus (5):
dmaengine: dw_dmac: use helper macro module_platform_driver()
dmaengine: dw_dmac: add module alias
dmaengine: dw_dmac: remove CLK dependency
dmaengine: dw_dmac: amend description and indentation
dmaengine: dw_dmac: add PCI part of the driver

 MAINTAINERS                         |    4 +-
 drivers/dma/Kconfig                 |   10 ++-
 drivers/dma/Makefile                |    2 +-
 drivers/dma/dw/Makefile             |    2 +
 drivers/dma/{ => dw}/dw_dmac.c      |   23 +++----
 drivers/dma/dw/dw_dmac_pci.c        |  126 +++++++++++++++++++++++++++++++++++
 drivers/dma/{ => dw}/dw_dmac_regs.h |    0
 7 files changed, 148 insertions(+), 19 deletions(-)
 create mode 100644 drivers/dma/dw/Makefile
 rename drivers/dma/{ => dw}/dw_dmac.c (99%)
 create mode 100644 drivers/dma/dw/dw_dmac_pci.c
 rename drivers/dma/{ => dw}/dw_dmac_regs.h (100%)

Signature · 1 line

1.7.10.4

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #1 lore raw

PATCH v3 1/7 dmaengine: dw_dmac: use helper macro module_platform_driver()

From: Heikki Krogerus <[email protected]>

Signed-off-by Heikki Krogerus <[email protected]>

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

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

drivers/dma/dw_dmac.c +2 ~~-11~~

index c4b0eb3..0b88ced 100644
@@ -1700,6 +1700,7 @@ MODULE_DEVICE_TABLE(of, dw_dma_id_table);
 #endif
 
 static struct platform_driver dw_driver = {
+	.probe		= dw_probe,
 	.remove		= __devexit_p(dw_remove),
 	.shutdown	= dw_shutdown,
 	.driver = {

@@ -1709,17 +1710,7 @@ static struct platform_driver dw_driver = {
 	},
 };
 
-static int __init dw_init(void)
-{
-	return platform_driver_probe(&dw_driver, dw_probe);
-}
-subsys_initcall(dw_init);
-
-static void __exit dw_exit(void)
-{
-	platform_driver_unregister(&dw_driver);
-}
-module_exit(dw_exit);
+module_platform_driver(dw_driver);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Synopsys DesignWare DMA Controller driver");

Signature · 1 line

1.7.10.4

Felipe Balbi

2012-09-27 07:45 UTC #2 lore raw

Re: [PATCHv3 1/7] dmaengine: dw_dmac: use helper macro module_platform_driver()

Quoted 2 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>

commit log would be great.

Quoted 11 lines

Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/dw_dmac.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
drivers/dma/dw_dmac.c +0 -0
index c4b0eb3..0b88ced 100644

drivers/dma/dw_dmac.c MODULE_DEVICE_TABLE(of, dw_dma_id_table); · line 1703

Quoted 4 lines

drivers/dma/dw_dmac.c +1 -0

@@ -1700,6 +1700,7 @@ MODULE_DEVICE_TABLE(of, dw_dma_id_table);
 #endif
 
 static struct platform_driver dw_driver = {
+	.probe		= dw_probe,

probe's in __init section. This is wrong. You need to change probe __devinit.

Quoted 30 lines

drivers/dma/dw_dmac.c +1 ~~-11~~

 	.remove		= __devexit_p(dw_remove),
 	.shutdown	= dw_shutdown,
 	.driver = {

@@ -1709,17 +1710,7 @@ static struct platform_driver dw_driver = {
 	},
 };
 
-static int __init dw_init(void)
-{
-	return platform_driver_probe(&dw_driver, dw_probe);
-}
-subsys_initcall(dw_init);
-
-static void __exit dw_exit(void)
-{
-	platform_driver_unregister(&dw_driver);
-}
-module_exit(dw_exit);
+module_platform_driver(dw_driver);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Synopsys DesignWare DMA Controller driver");

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signature · 1 line

balbi

viresh kumar

2012-09-27 08:16 UTC #3 lore raw

Re: [PATCHv3 1/7] dmaengine: dw_dmac: use helper macro module_platform_driver()

Quoted 10 lines · Felipe Balbi

drivers/dma/dw_dmac.c MODULE_DEVICE_TABLE(of, dw_dma_id_table); · line 1703
Quoted 7 lines
drivers/dma/dw_dmac.c +1 -0
@@ -1700,6 +1700,7 @@ MODULE_DEVICE_TABLE(of, dw_dma_id_table);
 #endif
 
 static struct platform_driver dw_driver = {
+     .probe          = dw_probe,
probe's in __init section. This is wrong. You need to change probe __devinit.

Good one. How can i miss it. :(

@Andy: Please add my Acked-by: after this change.

Felipe Balbi

Reviewed-by

2012-09-27 08:21 UTC #4 lore raw

Re: [PATCHv3 1/7] dmaengine: dw_dmac: use helper macro module_platform_driver()

Quoted 12 lines · viresh kumar

Quoted 10 lines · Felipe Balbi
drivers/dma/dw_dmac.c MODULE_DEVICE_TABLE(of, dw_dma_id_table); · line 1703
Quoted 7 lines
drivers/dma/dw_dmac.c +1 -0
@@ -1700,6 +1700,7 @@ MODULE_DEVICE_TABLE(of, dw_dma_id_table);
 #endif
 
 static struct platform_driver dw_driver = {
+     .probe          = dw_probe,
probe's in __init section. This is wrong. You need to change probe __devinit.

Quoted 3 lines

Good one. How can i miss it. :(
@Andy: Please add my Acked-by: after this change.

after this change you can also add:

Reviewed-by Felipe Balbi <[email protected]>

Signature · 1 line

balbi

Andy Shevchenko

author reviewer

2012-10-01 10:47 UTC #5 lore raw

Re: [PATCHv3 1/7] dmaengine: dw_dmac: use helper macro module_platform_driver()

In reply to Felipe Balbi

Quoted 5 lines · Felipe Balbi

Quoted 2 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
commit log would be great.

Will do.

Quoted 5 lines

drivers/dma/dw_dmac.c MODULE_DEVICE_TABLE(of, dw_dma_id_table); · line 1703
Quoted 2 lines
static struct platform_driver dw_driver = {
drivers/dma/dw_dmac.c +1 -0
+	.probe		= dw_probe,
probe's in __init section. This is wrong. You need to change probe __devinit.

Have you checked this one
0272e93f364eac1a30f2831adcaca3dd633d5f14

Or might be I missed something?

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Felipe Balbi

Reviewed-by

2012-10-01 13:10 UTC #6 lore raw

Re: [PATCHv3 1/7] dmaengine: dw_dmac: use helper macro module_platform_driver()

Hi,

Quoted 13 lines · Andy Shevchenko

Quoted 5 lines · Felipe Balbi
Quoted 2 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
commit log would be great.
Will do.
Quoted 5 lines
drivers/dma/dw_dmac.c MODULE_DEVICE_TABLE(of, dw_dma_id_table); · line 1703
Quoted 2 lines
static struct platform_driver dw_driver = {
drivers/dma/dw_dmac.c +1 -0
+	.probe		= dw_probe,
probe's in __init section. This is wrong. You need to change probe __devinit.

Quoted 4 lines

Have you checked this one
0272e93f364eac1a30f2831adcaca3dd633d5f14
Or might be I missed something?

My bad, missed that one. Looks like I need to update my local branch.

Reviewed-by Felipe Balbi <[email protected]>

Signature · 1 line

balbi

Andy Shevchenko

author reviewer

2012-09-27 07:33 UTC #7 lore raw

PATCH v3 2/7 dmaengine: dw_dmac: add module alias

In reply to Andy Shevchenko

From: Heikki Krogerus <[email protected]>

It's good to have a quasistatic name for the platform driver.

Signed-off-by Heikki Krogerus <[email protected]>

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

---
 drivers/dma/dw_dmac.c |    1 +
 1 file changed, 1 insertion(+)

drivers/dma/dw_dmac.c +1 -0

index 0b88ced..bbb2a82 100644
@@ -1712,6 +1712,7 @@ static struct platform_driver dw_driver = {
 
 module_platform_driver(dw_driver);
 
+MODULE_ALIAS("platform:dw_dmac");
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Synopsys DesignWare DMA Controller driver");
 MODULE_AUTHOR("Haavard Skinnemoen (Atmel)");

Signature · 1 line

1.7.10.4

viresh kumar

Acked-by on v3 2/7

2012-09-27 08:16 UTC #8 lore raw

Re: [PATCHv3 2/7] dmaengine: dw_dmac: add module alias

Quoted 15 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>
It's good to have a quasistatic name for the platform driver.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/dw_dmac.c |    1 +
 1 file changed, 1 insertion(+)
drivers/dma/dw_dmac.c +0 -0
index 0b88ced..bbb2a82 100644

drivers/dma/dw_dmac.c static struct platform_driver dw_driver = { · line 1715

Quoted 4 lines

drivers/dma/dw_dmac.c +1 -0

@@ -1712,6 +1712,7 @@ static struct platform_driver dw_driver = {
 
 module_platform_driver(dw_driver);
 
+MODULE_ALIAS("platform:dw_dmac");

Acked-by Viresh Kumar <[email protected]>

Felipe Balbi

Reviewed-by

2012-09-27 08:21 UTC #9 lore raw

Re: [PATCHv3 2/7] dmaengine: dw_dmac: add module alias

Quoted 18 lines · viresh kumar

Quoted 16 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
It's good to have a quasistatic name for the platform driver.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/dw_dmac.c |    1 +
 1 file changed, 1 insertion(+)
drivers/dma/dw_dmac.c +0 -0
index 0b88ced..bbb2a82 100644
@@ -1712,6 +1712,7 @@ static struct platform_driver dw_driver = {
 

Quoted 5 lines

drivers/dma/dw_dmac.c static struct platform_driver dw_driver = { · line 1715
Quoted 3 lines
drivers/dma/dw_dmac.c +1 -0
 module_platform_driver(dw_driver);
 
+MODULE_ALIAS("platform:dw_dmac");
Acked-by Viresh Kumar <[email protected]>

Reviewed-by Felipe Balbi <[email protected]>

Signature · 1 line

balbi

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #10 lore raw

PATCH v3 3/7 dmaengine: dw_dmac: remove CLK dependency

In reply to Andy Shevchenko

From: Heikki Krogerus <[email protected]>

This driver could be used on different platforms. Thus, the HAVE_CLK dependency is dropped away.

Signed-off-by Heikki Krogerus <[email protected]>

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

---
 drivers/dma/Kconfig |    1 -
 1 file changed, 1 deletion(-)

drivers/dma/Kconfig +0 -1

index 677cd6e..df32537 100644
@@ -83,7 +83,6 @@ config INTEL_IOP_ADMA
 
 config DW_DMAC
 	tristate "Synopsys DesignWare AHB DMA support"
-	depends on HAVE_CLK
 	select DMA_ENGINE
 	default y if CPU_AT32AP7000
 	help

Signature · 1 line

1.7.10.4

Felipe Balbi

2012-09-27 07:47 UTC #11 lore raw

Re: [PATCHv3 3/7] dmaengine: dw_dmac: remove CLK dependency

Hi,

Quoted 16 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>
This driver could be used on different platforms. Thus, the HAVE_CLK dependency is dropped away.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/Kconfig |    1 -
 1 file changed, 1 deletion(-)
drivers/dma/Kconfig +0 -0
index 677cd6e..df32537 100644

drivers/dma/Kconfig config INTEL_IOP_ADMA · line 86

Quoted 4 lines

drivers/dma/Kconfig +0 -1

@@ -83,7 +83,6 @@ config INTEL_IOP_ADMA
 
 config DW_DMAC
 	tristate "Synopsys DesignWare AHB DMA support"
-	depends on HAVE_CLK

as is, this will break compilation of any arch which doesn't set HAVE_CLK.

Signature · 1 line

balbi

Andy Shevchenko

2012-09-27 08:04 UTC #12 lore raw

Re: [PATCHv3 3/7] dmaengine: dw_dmac: remove CLK dependency

Quoted 4 lines · Felipe Balbi

Quoted 1 line
- depends on HAVE_CLK
as is, this will break compilation of any arch which doesn't set HAVE_CLK.

As Viresh suggested and I checked it's not. He wrote nice patch that adds stubs for such case.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Felipe Balbi

2012-09-27 08:13 UTC #13 lore raw

Re: [PATCHv3 3/7] dmaengine: dw_dmac: remove CLK dependency

Hi,

Quoted 7 lines · Andy Shevchenko

Quoted 4 lines · Felipe Balbi
Quoted 1 line
- depends on HAVE_CLK
as is, this will break compilation of any arch which doesn't set HAVE_CLK.
As Viresh suggested and I checked it's not. He wrote nice patch that adds stubs for such case.

Great, that finally reached mainline. I was waiting for that patch for a long time, actually :-)

thanks for the note

Signature · 1 line

balbi

viresh kumar

Acked-by

2012-09-27 08:18 UTC #14 lore raw

Re: [PATCHv3 3/7] dmaengine: dw_dmac: remove CLK dependency

In reply to Felipe Balbi

Quoted 20 lines · Felipe Balbi

Hi,
Quoted 17 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
This driver could be used on different platforms. Thus, the HAVE_CLK dependency is dropped away.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/Kconfig |    1 -
 1 file changed, 1 deletion(-)
drivers/dma/Kconfig +0 -0
index 677cd6e..df32537 100644
@@ -83,7 +83,6 @@ config INTEL_IOP_ADMA
 

Quoted 6 lines

drivers/dma/Kconfig config INTEL_IOP_ADMA · line 86
Quoted 3 lines
drivers/dma/Kconfig +0 -1
 config DW_DMAC
      tristate "Synopsys DesignWare AHB DMA support"
-     depends on HAVE_CLK
as is, this will break compilation of any arch which doesn't set HAVE_CLK.

No, it will not due to following commit id:

commit 93abe8e4b13ae9a0428ce940a8a03ac72a7626f1

Author: Viresh Kumar <[email protected]>
Date:   Mon Jul 30 14:39:27 2012 -0700

clk: add non CONFIG_HAVE_CLK routines

@Andy:

Acked-by Viresh Kumar <[email protected]>

Felipe Balbi

Reviewed-by

2012-09-27 08:22 UTC #15 lore raw

Re: [PATCHv3 3/7] dmaengine: dw_dmac: remove CLK dependency

Quoted 34 lines · viresh kumar

Quoted 20 lines · Felipe Balbi
Hi,
Quoted 17 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
This driver could be used on different platforms. Thus, the HAVE_CLK dependency is dropped away.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/Kconfig |    1 -
 1 file changed, 1 deletion(-)
drivers/dma/Kconfig +0 -0
index 677cd6e..df32537 100644
@@ -83,7 +83,6 @@ config INTEL_IOP_ADMA
 
Quoted 6 lines
drivers/dma/Kconfig config INTEL_IOP_ADMA · line 86
Quoted 3 lines
drivers/dma/Kconfig +0 -1
 config DW_DMAC
      tristate "Synopsys DesignWare AHB DMA support"
-     depends on HAVE_CLK
as is, this will break compilation of any arch which doesn't set HAVE_CLK.
No, it will not due to following commit id:
commit 93abe8e4b13ae9a0428ce940a8a03ac72a7626f1
Author: Viresh Kumar <[email protected]>
Date:   Mon Jul 30 14:39:27 2012 -0700

Quoted 6 lines

clk: add non CONFIG_HAVE_CLK routines
@Andy:
Acked-by Viresh Kumar <[email protected]>

Reviewed-by Felipe Balbi <[email protected]>

Signature · 1 line

balbi

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #16 lore raw

PATCH v3 4/7 dmaengine: dw_dmac: amend description and indentation

In reply to Andy Shevchenko

From: Heikki Krogerus <[email protected]>

The driver will be used as a core part for various implementations of the DesignWare DMA device. The patch adjusts description on the top and corrects paragraph indentation in few places across the code.

Signed-off-by Heikki Krogerus <[email protected]>

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

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

drivers/dma/dw_dmac.c +4 -3

index bbb2a82..9f0129d 100644
@@ -1,14 +1,15 @@
 /*
- * Driver for the Synopsys DesignWare DMA Controller (aka DMACA on
- * AVR32 systems.)
+ * Core driver for the Synopsys DesignWare DMA Controller
  *
  * Copyright (C) 2007-2008 Atmel Corporation
  * Copyright (C) 2010-2011 ST Microelectronics
+ * Copyright (C) 2012 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>

@@ -222,7 +223,6 @@ static inline void dwc_dump_chan_regs(struct dw_dma_chan *dwc)
 		channel_readl(dwc, CTL_LO));
 }
 
-
 static inline void dwc_chan_disable(struct dw_dma *dw, struct dw_dma_chan *dwc)
 {
 	channel_clear_bit(dw, CH_EN, dwc->mask);

@@ -1679,6 +1679,7 @@ static int dw_resume_noirq(struct device *dev)
 
 	clk_prepare_enable(dw->clk);
 	dma_writel(dw, CFG, DW_CFG_DMA_EN);
+
 	return 0;
 }

Signature · 1 line

1.7.10.4

Felipe Balbi

2012-09-27 07:57 UTC #17 lore raw

Re: [PATCHv3 4/7] dmaengine: dw_dmac: amend description and indentation

Quoted 17 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>
The driver will be used as a core part for various implementations of the DesignWare DMA device. The patch adjusts description on the top and corrects paragraph indentation in few places across the code.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/dw_dmac.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/dma/dw_dmac.c +0 -0
index bbb2a82..9f0129d 100644

drivers/dma/dw_dmac.c line 6

Quoted 8 lines

drivers/dma/dw_dmac.c +2 -2

@@ -1,14 +1,15 @@
 /*
- * Driver for the Synopsys DesignWare DMA Controller (aka DMACA on
- * AVR32 systems.)
+ * Core driver for the Synopsys DesignWare DMA Controller
  *
  * Copyright (C) 2007-2008 Atmel Corporation
  * Copyright (C) 2010-2011 ST Microelectronics
+ * Copyright (C) 2012 Intel Corporation

I'm not a lawyer, but I'm not sure the few changes done to this driver is enough for Intel to hold a copyright here... dunno, though.

Signature · 1 line

balbi

viresh kumar

2012-09-27 08:19 UTC #18 lore raw

Re: [PATCHv3 4/7] dmaengine: dw_dmac: amend description and indentation

Quoted 13 lines · Felipe Balbi

Quoted 12 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
The driver will be used as a core part for various implementations of the DesignWare DMA device. The patch adjusts description on the top and corrects paragraph indentation in few places across the code.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/dw_dmac.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Quoted 16 lines

drivers/dma/dw_dmac.c line 6
Quoted 13 lines
drivers/dma/dw_dmac.c +2 -2
index bbb2a82..9f0129d 100644
@@ -1,14 +1,15 @@
 /*
- * Driver for the Synopsys DesignWare DMA Controller (aka DMACA on
- * AVR32 systems.)
+ * Core driver for the Synopsys DesignWare DMA Controller
  *
  * Copyright (C) 2007-2008 Atmel Corporation
  * Copyright (C) 2010-2011 ST Microelectronics
+ * Copyright (C) 2012 Intel Corporation
I'm not a lawyer, but I'm not sure the few changes done to this driver is enough for Intel to hold a copyright here... dunno, though.

Neither do i :)

@Vinod: Can we have some inputs from you here?

--
viresh

Andy Shevchenko

2012-09-27 08:37 UTC #19 lore raw

Re: [PATCHv3 4/7] dmaengine: dw_dmac: amend description and indentation

Quoted 31 lines · viresh kumar

Quoted 13 lines · Felipe Balbi
Quoted 12 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
The driver will be used as a core part for various implementations of the DesignWare DMA device. The patch adjusts description on the top and corrects paragraph indentation in few places across the code.
Signed-off-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/dma/dw_dmac.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Quoted 16 lines
drivers/dma/dw_dmac.c line 6
Quoted 13 lines
drivers/dma/dw_dmac.c +2 -2
index bbb2a82..9f0129d 100644
@@ -1,14 +1,15 @@
 /*
- * Driver for the Synopsys DesignWare DMA Controller (aka DMACA on
- * AVR32 systems.)
+ * Core driver for the Synopsys DesignWare DMA Controller
  *
  * Copyright (C) 2007-2008 Atmel Corporation
  * Copyright (C) 2010-2011 ST Microelectronics
+ * Copyright (C) 2012 Intel Corporation
I'm not a lawyer, but I'm not sure the few changes done to this driver is enough for Intel to hold a copyright here... dunno, though.

Quoted 1 line

Neither do i :)

I only have one precedent where was a collision between two versions of the as3645a driver.
In the result Nokia's version was modified accordingly to my comments and few patches.

Quoted 1 line

@Vinod: Can we have some inputs from you here?

Agree, it's good to have an additional opinion here.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #20 lore raw

PATCH v3 5/7 dmaengine: dw_dmac: add PCI part of the driver

In reply to Andy Shevchenko

From: Heikki Krogerus <[email protected]>

This is the PCI part of the DesignWare DMAC driver. The controller is usually used in the Intel hardware such as Medfield.

Signed-off-by Heikki Krogerus <[email protected]>

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

---
 drivers/dma/Kconfig       |    9 ++++
 drivers/dma/Makefile      |    1 +
 drivers/dma/dw_dmac_pci.c |  126 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+)
 create mode 100644 drivers/dma/dw_dmac_pci.c

drivers/dma/Kconfig +9 -0

index df32537..a5c7f64 100644
@@ -89,6 +89,15 @@ config DW_DMAC
 	  Support the Synopsys DesignWare AHB DMA controller.  This
 	  can be integrated in chips such as the Atmel AT32ap7000.
 
+config DW_DMAC_PCI
+	tristate "Synopsys DesignWare AHB DMA support (PCI bus)"
+	depends on PCI
+	select DW_DMAC
+	help
+	  Support the Synopsys DesignWare AHB DMA controller on the platfroms
+	  that provide it as a PCI device. For example, Intel Medfield has
+	  integrated this GPDMA controller.
+
 config AT_HDMAC
 	tristate "Atmel AHB DMA support"
 	depends on ARCH_AT91

drivers/dma/Makefile +1 -0

index 7428fea..15eef5f 100644
@@ -12,6 +12,7 @@ obj-$(CONFIG_FSL_DMA) += fsldma.o
 obj-$(CONFIG_MPC512X_DMA) += mpc512x_dma.o
 obj-$(CONFIG_MV_XOR) += mv_xor.o
 obj-$(CONFIG_DW_DMAC) += dw_dmac.o
+obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o
 obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o

drivers/dma/dw_dmac_pci.c new file +126 -0

126 lines not on this page · raw message

Signature · 1 line

1.7.10.4

Felipe Balbi

2012-09-27 07:54 UTC #21 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 58 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>

This is the PCI part of the DesignWare DMAC driver. The controller is usually used in the Intel hardware such as Medfield.

Signed-off-by Heikki Krogerus <[email protected]>

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

---
 drivers/dma/Kconfig       |    9 ++++
 drivers/dma/Makefile      |    1 +
 drivers/dma/dw_dmac_pci.c |  126 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+)
 create mode 100644 drivers/dma/dw_dmac_pci.c

drivers/dma/Kconfig +9 -0

index df32537..a5c7f64 100644
@@ -89,6 +89,15 @@ config DW_DMAC
 	  Support the Synopsys DesignWare AHB DMA controller.  This
 	  can be integrated in chips such as the Atmel AT32ap7000.
 
+config DW_DMAC_PCI
+	tristate "Synopsys DesignWare AHB DMA support (PCI bus)"
+	depends on PCI
+	select DW_DMAC
+	help
+	  Support the Synopsys DesignWare AHB DMA controller on the platfroms
+	  that provide it as a PCI device. For example, Intel Medfield has
+	  integrated this GPDMA controller.
+
 config AT_HDMAC
 	tristate "Atmel AHB DMA support"
 	depends on ARCH_AT91

drivers/dma/Makefile +1 -0

index 7428fea..15eef5f 100644
@@ -12,6 +12,7 @@ obj-$(CONFIG_FSL_DMA) += fsldma.o
 obj-$(CONFIG_MPC512X_DMA) += mpc512x_dma.o
 obj-$(CONFIG_MV_XOR) += mv_xor.o
 obj-$(CONFIG_DW_DMAC) += dw_dmac.o
+obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o
 obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o

drivers/dma/dw_dmac_pci.c new file +6 -0

new file mode 100644
index 0000000..b7ad37f
@@ -0,0 +1,126 @@
+/*
+ * PCI driver for the Synopsys DesignWare DMA Controller
+ *
+ * Copyright (C) 2012 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify

drivers/dma/dw_dmac_pci.c line 16

Quoted 10 lines

drivers/dma/dw_dmac_pci.c new file +10 -0

+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/dw_dmac.h>
+
+static struct dw_dma_platform_data pdata = {

"pdata" looks like it wants a prefix.

drivers/dma/dw_dmac_pci.c line 20

Quoted 4 lines

drivers/dma/dw_dmac_pci.c new file +4 -0

+	.is_private = 1,
+	.chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
+	.chan_priority = CHAN_PRIORITY_ASCENDING,
+};

This is the same for all of the PCI IDs listed below, looks like it's best to just add it as platform_data of the dwc_dmac device directly, rather than passing a pointer to this via driver->data.

drivers/dma/dw_dmac_pci.c line 27

Quoted 7 lines

drivers/dma/dw_dmac_pci.c new file +7 -0

+
+static int __devinit dw_pci_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
+	struct platform_device *pd;
+	struct resource r[2];
+	struct dw_dma_platform_data *driver = (void *)id->driver_data;

missing space after the cast... but it looks unnecessary

drivers/dma/dw_dmac_pci.c line 28

Quoted 1 line

drivers/dma/dw_dmac_pci.c new file +1 -0
+	static int instance;

this could be a IDA instead.

Signature · 1 line

balbi

Andy Shevchenko

2012-09-27 08:08 UTC #22 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 20 lines · Felipe Balbi

Quoted 19 lines · Andy Shevchenko
From: Heikki Krogerus <[email protected]>
This is the PCI part of the DesignWare DMAC driver. The controller is usually used in the Intel hardware such as Medfield.
drivers/dma/dw_dmac_pci.c new file +10 -0
@@ -0,0 +1,126 @@
+/*
+ * PCI driver for the Synopsys DesignWare DMA Controller
+ *
+ * Copyright (C) 2012 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+

Quoted 8 lines

drivers/dma/dw_dmac_pci.c line 16
Quoted 6 lines
drivers/dma/dw_dmac_pci.c new file +6 -0
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/dw_dmac.h>
+
+static struct dw_dma_platform_data pdata = {
"pdata" looks like it wants a prefix.

Okay.

Quoted 8 lines

drivers/dma/dw_dmac_pci.c line 20
Quoted 4 lines
drivers/dma/dw_dmac_pci.c new file +4 -0
+     .is_private = 1,
+     .chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
+     .chan_priority = CHAN_PRIORITY_ASCENDING,
+};
This is the same for all of the PCI IDs listed below, looks like it's best to just add it as platform_data of the dwc_dmac device directly, rather than passing a pointer to this via driver->data.

It potentially could be altered. I prefer to leave it as structure in the pci driver.

Quoted 2 lines

drivers/dma/dw_dmac_pci.c line 28
Quoted 1 line
drivers/dma/dw_dmac_pci.c new file +1 -0
+     static int instance;
this could be a IDA instead.

Will look at it, thanks.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Felipe Balbi

2012-09-27 08:14 UTC #23 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Hi,

Quoted 10 lines · Andy Shevchenko

Quoted 8 lines
drivers/dma/dw_dmac_pci.c line 20
Quoted 4 lines
drivers/dma/dw_dmac_pci.c new file +4 -0
+     .is_private = 1,
+     .chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
+     .chan_priority = CHAN_PRIORITY_ASCENDING,
+};
This is the same for all of the PCI IDs listed below, looks like it's best to just add it as platform_data of the dwc_dmac device directly, rather than passing a pointer to this via driver->data.
It potentially could be altered. I prefer to leave it as structure in the pci driver.

fair enough ;-)

Signature · 1 line

balbi

Vinod Koul

2012-09-27 08:50 UTC #24 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

In reply to Andy Shevchenko

drivers/dma/dw_dmac_pci.c line 20

Quoted 11 lines · Andy Shevchenko

drivers/dma/dw_dmac_pci.c new file +11 -0

+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/dw_dmac.h>
+
+static struct dw_dma_platform_data pdata = {
+	.is_private = 1,
+	.chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
+	.chan_priority = CHAN_PRIORITY_ASCENDING,
+};

pdata could use a prefix/suffix

Quoted 4 lines

drivers/dma/dw_dmac_pci.c new file +4 -0

+
+static int __devinit dw_pci_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	pd = platform_device_alloc("dw_dmac", instance);

Why can't the core driver library be agnostic. Why do we care if the device is platform, pci or something else.
There is nothing in dma API callbacks which driver implements that warrants it to be some device.

You have already taken care of dma controller accesses be arch independent so I don't see a reason why this should be done?

Quoted 5 lines

drivers/dma/dw_dmac_pci.c new file +5 -0

+	if (!pd) {
+		dev_err(&pdev->dev, "can't allocate dw_dmac platform device\n");
+		ret = -ENOMEM;
+		goto err0;
+	}

Signature · 1 line

~Vinod

Andy Shevchenko

author reviewer

2012-09-27 09:43 UTC #25 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 11 lines · Vinod Koul

Quoted 3 lines
drivers/dma/dw_dmac_pci.c new file +3 -0
+static int __devinit dw_pci_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
...
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	pd = platform_device_alloc("dw_dmac", instance);
Why can't the core driver library be agnostic. Why do we care if the device is platform, pci or something else.
There is nothing in dma API callbacks which driver implements that warrants it to be some device.

Quoted 2 lines

You have already taken care of dma controller accesses be arch independent so I don't see a reason why this should be done?

I'm afraid I didn't get you comment.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Vinod Koul

2012-09-27 10:07 UTC #26 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 12 lines · Andy Shevchenko

Quoted 11 lines · Vinod Koul
Quoted 3 lines
drivers/dma/dw_dmac_pci.c new file +3 -0
+static int __devinit dw_pci_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
...
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	pd = platform_device_alloc("dw_dmac", instance);
Why can't the core driver library be agnostic. Why do we care if the device is platform, pci or something else.
There is nothing in dma API callbacks which driver implements that warrants it to be some device.

Quoted 3 lines

Quoted 2 lines
You have already taken care of dma controller accesses be arch independent so I don't see a reason why this should be done?
I'm afraid I didn't get you comment.

Let me try again.

what does it take to do platform and PCI driver for this:
1. make dma h/w access (read/write) platform independent. which you have already done
2. Device registration: Create two probes, or use common probe. You smartly chose the second one BUT by creating another device. If you look closely at the probe then I would say it would be easy to create library for probe which can be used across both pci and platform driver probes. The probe library which initializes driver and registers with dmaengine needs device struct and resources can be provided by each probe.

Signature · 1 line

~Vinod

viresh kumar

2012-09-27 10:11 UTC #27 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 12 lines · Vinod Koul

Let me try again.
what does it take to do platform and PCI driver for this:
1. make dma h/w access (read/write) platform independent. which you have already done
2. Device registration: Create two probes, or use common probe. You smartly chose the second one BUT by creating another device. If you look closely at the probe then I would say it would be easy to create library for probe which can be used across both pci and platform driver probes. The probe library which initializes driver and registers with dmaengine needs device struct and resources can be provided by each probe.

Or in other words... create three files
- dw_dmac.c
- dw_dmac-pltfm.c
- dw_dmac-pci.c...

Don't do anything specific to platform or pci in dw_dmac.c... Keep pltfm and pci files to smallest possible size, and keep as much of common part in dmac.c...

Similar is done in drivers/mmc/host/sdhci*...

This is what i was thinking when i questioned multiple device creation...

--
viresh

Vinod Koul

2012-09-27 10:38 UTC #28 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 19 lines · viresh kumar

Quoted 12 lines · Vinod Koul
Let me try again.
what does it take to do platform and PCI driver for this:
1. make dma h/w access (read/write) platform independent. which you have already done
2. Device registration: Create two probes, or use common probe. You smartly chose the second one BUT by creating another device. If you look closely at the probe then I would say it would be easy to create library for probe which can be used across both pci and platform driver probes. The probe library which initializes driver and registers with dmaengine needs device struct and resources can be provided by each probe.
Or in other words... create three files
- dw_dmac.c
- dw_dmac-pltfm.c
- dw_dmac-pci.c...

Quoted 5 lines

Don't do anything specific to platform or pci in dw_dmac.c... Keep pltfm and pci files to smallest possible size, and keep as much of common part in dmac.c...
Similar is done in drivers/mmc/host/sdhci*...

Yes that IMHO would be simpler approach :)

Signature · 1 line

~Vinod

Andy Shevchenko

2012-10-03 13:40 UTC #29 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 20 lines · Vinod Koul

Quoted 19 lines · viresh kumar
Quoted 12 lines · Vinod Koul
Let me try again.
what does it take to do platform and PCI driver for this:
1. make dma h/w access (read/write) platform independent. which you have already done
2. Device registration: Create two probes, or use common probe. You smartly chose the second one BUT by creating another device. If you look closely at the probe then I would say it would be easy to create library for probe which can be used across both pci and platform driver probes. The probe library which initializes driver and registers with dmaengine needs device struct and resources can be provided by each probe.
Or in other words... create three files
- dw_dmac.c
- dw_dmac-pltfm.c
- dw_dmac-pci.c...

Quoted 6 lines

Quoted 5 lines
Don't do anything specific to platform or pci in dw_dmac.c... Keep pltfm and pci files to smallest possible size, and keep as much of common part in dmac.c...
Similar is done in drivers/mmc/host/sdhci*...
Yes that IMHO would be simpler approach :)

Oh, thanks, I got the idea. Will implement soon.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author reviewer

2012-10-08 10:17 UTC #30 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

In reply to viresh kumar

Quoted 19 lines · viresh kumar

Quoted 12 lines · Vinod Koul
Let me try again.
what does it take to do platform and PCI driver for this:
1. make dma h/w access (read/write) platform independent. which you have already done
2. Device registration: Create two probes, or use common probe. You smartly chose the second one BUT by creating another device. If you look closely at the probe then I would say it would be easy to create library for probe which can be used across both pci and platform driver probes. The probe library which initializes driver and registers with dmaengine needs device struct and resources can be provided by each probe.
Or in other words... create three files
- dw_dmac.c
- dw_dmac-pltfm.c
- dw_dmac-pci.c...

Quoted 5 lines

Don't do anything specific to platform or pci in dw_dmac.c... Keep pltfm and pci files to smallest possible size, and keep as much of common part in dmac.c...
Similar is done in drivers/mmc/host/sdhci*...

This approach has the significant differences to proposed before.

First of all it will export IP-block relevant functions to the kernel namespace. I think it is not a good idea to pollute kernel more. Moreover the API dependencies disallow transparent build and usage of the drivers. For example, you can't built-in platform driver and leave core part as a module. And there is at least one device, Intel Medfield, where DMA controller is exposed as PCI device on fake PCI bus. In that case the initialization sequence matters and the easier way is to provide independent drivers for platform device.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Viresh Kumar

2012-10-08 10:49 UTC #31 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 1 line · Andy Shevchenko

This approach has the significant differences to proposed before.

I am afraid i didn't get your mail completely. Still i will try based on my understanding.

Quoted 2 lines

First of all it will export IP-block relevant functions to the kernel namespace. I think it is not a good idea to pollute kernel more.

So, few routines which are required to be called from probe, suspend/resume and exit would be made non-static... This is what you wanted to say? Hopefully most of the routines would still be declared static in the core file. So IMHO, the simplicity or clarity that new approach gives has more advantages than this aspect.

Quoted 3 lines

Moreover the API dependencies disallow transparent build and usage of the drivers. For example, you can't built-in platform driver and leave core part as a module.

Obviously... Should be done this way only... What if core driver isn't inserted and platform's probe is already called. That's why depends-on should not allow you to make core part as module alone...

I couldn't get the issue completely. What's the problem in this approach?

Quoted 4 lines

And there is at least one device, Intel Medfield,
where DMA controller is exposed as PCI device on fake PCI bus. In that case the initialization sequence matters and the easier way is to provide independent drivers for platform device.

Couldn't get this one :(

--
viresh

Andy Shevchenko

author reviewer

2012-10-08 12:46 UTC #32 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 6 lines · Viresh Kumar

Quoted 1 line · Andy Shevchenko
This approach has the significant differences to proposed before.
I am afraid i didn't get your mail completely. Still i will try based on my understanding.

Ok, let me try again.

Quoted 8 lines

Quoted 2 lines
First of all it will export IP-block relevant functions to the kernel namespace. I think it is not a good idea to pollute kernel more.
So, few routines which are required to be called from probe, suspend/resume and exit would be made non-static... This is what you wanted to say? Hopefully most of the routines would still be declared static in the core file. So IMHO, the simplicity or clarity that new approach gives has more advantages than this aspect.

The probe (core part), remove, shutdown, suspend, and resume will be exported. Practically each generic function which will be used in drivers outside of core. Why do we need them in the kernel namespace?

Quoted 4 lines

Quoted 3 lines
Moreover the API dependencies disallow transparent build and usage of the drivers. For example, you can't built-in platform driver and leave core part as a module.
Obviously... Should be done this way only...

I could not agree. I don't see any reason why this is the only way.

Quoted 2 lines

What if core driver isn't inserted
and platform's probe is already called.

Nothing will happen in my case until user loads all necessary bits.

Quoted 3 lines

That's why depends-on should not
allow you to make core part as module alone...
I couldn't get the issue completely. What's the problem in this approach?

Why we need to do this if we could avoid it? I see nothing to prevent us to build parts as modules, or otherwise, or mixed style.
In other words one approach provides weak dependency and the other - hard dependency between pieces of code.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Viresh Kumar

2012-10-08 13:27 UTC #33 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Viresh Kumar
Quoted 2 lines
First of all it will export IP-block relevant functions to the kernel namespace. I think it is not a good idea to pollute kernel more.

Quoted 8 lines

Quoted 5 lines
So, few routines which are required to be called from probe, suspend/resume and exit would be made non-static... This is what you wanted to say? Hopefully most of the routines would still be declared static in the core file. So IMHO, the simplicity or clarity that new approach gives has more advantages than this aspect.
The probe (core part), remove, shutdown, suspend, and resume will be exported. Practically each generic function which will be used in drivers outside of core. Why do we need them in the kernel namespace?

And these are generic kernel framework supporting routines, instead of routines that expose the hardware. Even then exposing them isn't that bad.

I do agree, we must have as less routines in kernel namespace as possible. But not at the price of over complexity of stuff which isn't required.

Quoted 4 lines

Quoted 2 lines
What if core driver isn't inserted
and platform's probe is already called.
Nothing will happen in my case until user loads all necessary bits.

Quoted 8 lines

Quoted 3 lines
That's why depends-on should not
allow you to make core part as module alone...
I couldn't get the issue completely. What's the problem in this approach?
Why we need to do this if we could avoid it? I see nothing to prevent us to build parts as modules, or otherwise, or mixed style.
In other words one approach provides weak dependency and the other - hard dependency between pieces of code.

I agree that there are some parts of your approach which might be having few advantages. But it is actually adding more complexity without much need of it. Logically speaking, we never had two devices for the same dma controller. We are adding them just to have pci over platform.. Which would mean the system become more and more complex..

So, during run time...
- there will be two device-driver binding loops.. Once for pci and then for platform
- In suspend/resume... two devices will get into suspend, instead of one..
- There might be other frameworks in kernel.. which react on struct device basis... they will get affected too..
- You have larger image size for pci case. as you compile platform too..

Just try to think from this perspective... we dont have two hardware devices in the system.... Ideally speaking there must be a struct device associated with a hardware device...

@Arnd/Vinod: Can you guys throw some more light here.. on the adv/disadv of both the approaches?

--
viresh

Vinod Koul

2012-10-24 11:27 UTC #34 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 14 lines · Viresh Kumar

I agree that there are some parts of your approach which might be having few advantages. But it is actually adding more complexity without much need of it. Logically speaking, we never had two devices for the same dma controller. We are adding them just to have pci over platform.. Which would mean the system become more and more complex..
So, during run time...
- there will be two device-driver binding loops.. Once for pci and then for platform
- In suspend/resume... two devices will get into suspend, instead of one..
- There might be other frameworks in kernel.. which react on struct device basis... they will get affected too..
- You have larger image size for pci case. as you compile platform too..

Quoted 6 lines

Just try to think from this perspective... we dont have two hardware devices in the system.... Ideally speaking there must be a struct device associated with a hardware device...
@Arnd/Vinod: Can you guys throw some more light here.. on the adv/disadv of both the approaches?

I am worried about those tow. Runtime PM handlers in case PCI devices make life a lot easier am not sure what support will be there for platform devices in other systems.
Also next version of this h/w on our systems is bringing subtle changes so having them separate seemed to me a better idea.

Signature · 2 lines

Vinod Koul
Intel Corp.

Andy Shevchenko

author reviewer

2012-10-30 15:05 UTC #35 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

In reply to Viresh Kumar

Quoted 15 lines · Viresh Kumar

Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Viresh Kumar
Quoted 2 lines
First of all it will export IP-block relevant functions to the kernel namespace. I think it is not a good idea to pollute kernel more.
Quoted 8 lines
Quoted 5 lines
So, few routines which are required to be called from probe, suspend/resume and exit would be made non-static... This is what you wanted to say? Hopefully most of the routines would still be declared static in the core file. So IMHO, the simplicity or clarity that new approach gives has more advantages than this aspect.
The probe (core part), remove, shutdown, suspend, and resume will be exported. Practically each generic function which will be used in drivers outside of core. Why do we need them in the kernel namespace?

Quoted 5 lines

And these are generic kernel framework supporting routines, instead of routines that expose the hardware. Even then exposing them isn't that bad.
I do agree, we must have as less routines in kernel namespace as possible. But not at the price of over complexity of stuff which isn't required.

I didn't see the complexity you are talking about.

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
That's why depends-on should not
allow you to make core part as module alone...
I couldn't get the issue completely. What's the problem in this approach?

Quoted 8 lines

Quoted 4 lines
Why we need to do this if we could avoid it? I see nothing to prevent us to build parts as modules, or otherwise, or mixed style.
In other words one approach provides weak dependency and the other - hard dependency between pieces of code.
I agree that there are some parts of your approach which might be having few advantages. But it is actually adding more complexity without much need of it.

Sorry, still didn't get where it is (complexity) and in what part.

Quoted 2 lines

Logically speaking, we never had two devices for the same dma controller.

What do you mean by this? Do we ever have the same IP block on different buses at the same time? I think it's potentially possible.

Quoted 7 lines

We are adding them just to have pci over platform.. Which would mean the system become more and more complex..
So, during run time...
- there will be two device-driver binding loops.. Once for pci and then for platform

Is this a real problem? We have dozens of the drivers on modern hw, part of them by the way use proposed approach.

Quoted 1 line

- In suspend/resume... two devices will get into suspend, instead of one..

True. But it allows to keep a potential to have the same devices to be attached to the different buses simultaneously.

Quoted 2 lines

- There might be other frameworks in kernel.. which react on struct device basis... they will get affected too..

I'm wondering if there any that affects us. I think you might know if there was any example of it (in history?).

Quoted 1 line

- You have larger image size for pci case. as you compile platform too..

How much? I didn't check this, but I believe it will consume one page at most.

Quoted 3 lines

Just try to think from this perspective... we dont have two hardware devices in the system.... Ideally speaking there must be a struct device associated with a hardware device...

I'm sorry, but I'm still not fully convinced by those arguments. For me it looks like both approaches have good and bad aspects on the similar level.

Actually sdhci seems for me as a not good example. In the mmc code they have already robust architecture (there is host controller, there is core part, there is card part and so on...) and enough of ->ops structures. We have a simpler driver.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Viresh Kumar

2012-10-30 15:19 UTC #36 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 6 lines · Andy Shevchenko

Quoted 2 lines · Viresh Kumar
I do agree, we must have as less routines in kernel namespace as possible. But not at the price of over complexity of stuff which isn't required.
I didn't see the complexity you are talking about.

Complexity not in the way that we need a genius to understand the code :) But creating devices layer which doesn't exist.

Quoted 5 lines

Quoted 2 lines
Logically speaking, we never had two devices for the same dma controller.
What do you mean by this? Do we ever have the same IP block on different buses at the same time? I think it's potentially possible.

One device is one instance of an IP. With your approach we will have two device structures for a single physical IP present.

Quoted 9 lines

Quoted 6 lines
We are adding them just to have pci over platform.. Which would mean the system become more and more complex..
So, during run time...
- there will be two device-driver binding loops.. Once for pci and then for platform
Is this a real problem? We have dozens of the drivers on modern hw, part of them by the way use proposed approach.

I am repeating this (probably said this earlier, if not sorry). If something already
exists in kernel, it doesn't meant that it is perfect and others should blindly follow
it.

The loops isn't a problem, but an unnecessary activity done.

Quoted 4 lines

Quoted 1 line
- In suspend/resume... two devices will get into suspend, instead of one..
True. But it allows to keep a potential to have the same devices to be attached to the different buses simultaneously.

I don't see why a single physical block would be connected to AMBA and PCI at the same time. And obviously that's not the case right now, so better leave that in current argument :)

Quoted 5 lines

Quoted 2 lines
- There might be other frameworks in kernel.. which react on struct device basis... they will get affected too..
I'm wondering if there any that affects us. I think you might know if there was any example of it (in history?).

There are many frameworks which work per device. regulator, clk, RTPM, etc. They will see a hardware hierarchy which doesn't exist.

Quoted 4 lines

Quoted 1 line
- You have larger image size for pci case. as you compile platform too..
How much? I didn't check this, but I believe it will consume one page at most.

Whatever amount it is. We can't waste it.

Quoted 4 lines

Actually sdhci seems for me as a not good example. In the mmc code they have already robust architecture (there is host controller, there is core part, there is card part and so on...) and enough of ->ops structures. We have a simpler driver.

I am not talking about SD/MMC as a whole. SDHCI targets a specific hardware configuration (manufactured by multiple vendors). It is very similar to our case here.

For me i don't agree with your approach. So, its a NACK. However if Arnd/Vinod or You can give me some strong points about your approach, i can consider it again :)

--
viresh

Andy Shevchenko

2012-10-30 15:39 UTC #37 lore raw

Re: [PATCHv3 5/7] dmaengine: dw_dmac: add PCI part of the driver

Quoted 5 lines · Viresh Kumar

Quoted 1 line · Andy Shevchenko
On Mon, 2012-10-08 at 18:57 +0530, Viresh Kumar wrote:
For me i don't agree with your approach. So, its a NACK.

Point taken.

Quoted 2 lines

However if Arnd/Vinod or You can give me some strong points about your approach, i can consider it again :)

Fair enough.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #38 lore raw

PATCH v3 6/7 dma: move dw_dmac driver to an own directory

In reply to Andy Shevchenko

The dw_dmac driver contains multiple files. To make a managment of them more convenient move it to an own directory.

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

---
 drivers/dma/Makefile                |    3 +--
 drivers/dma/dw/Makefile             |    2 ++
 drivers/dma/{ => dw}/dw_dmac.c      |    2 +-
 drivers/dma/{ => dw}/dw_dmac_pci.c  |    0
 drivers/dma/{ => dw}/dw_dmac_regs.h |    0
 5 files changed, 4 insertions(+), 3 deletions(-)
 create mode 100644 drivers/dma/dw/Makefile
 rename drivers/dma/{ => dw}/dw_dmac.c (99%)
 rename drivers/dma/{ => dw}/dw_dmac_pci.c (100%)
 rename drivers/dma/{ => dw}/dw_dmac_regs.h (100%)

drivers/dma/Makefile +1 -2

index 15eef5f..122a48a 100644
@@ -11,8 +11,7 @@ obj-$(CONFIG_INTEL_IOP_ADMA) += iop-adma.o
 obj-$(CONFIG_FSL_DMA) += fsldma.o
 obj-$(CONFIG_MPC512X_DMA) += mpc512x_dma.o
 obj-$(CONFIG_MV_XOR) += mv_xor.o
-obj-$(CONFIG_DW_DMAC) += dw_dmac.o
-obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o
+obj-$(CONFIG_DW_DMAC) += dw/
 obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o

drivers/dma/dw/Makefile new file +2 -0

new file mode 100644
index 0000000..2edfb24
@@ -0,0 +1,2 @@
+obj-$(CONFIG_DW_DMAC) += dw_dmac.o
+obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o

drivers/dma/dw/dw_dmac.c renamed from drivers/dma/dw_dmac.c +1 -1

similarity index 99%
rename from drivers/dma/dw_dmac.c
rename to drivers/dma/dw/dw_dmac.c
index 9f0129d..fa0471a 100644
@@ -24,8 +24,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#include "../dmaengine.h"
 #include "dw_dmac_regs.h"
-#include "dmaengine.h"
 
 /*
  * This supports the Synopsys "DesignWare AHB Central DMA Controller",

drivers/dma/dw/dw_dmac_pci.c renamed from drivers/dma/dw_dmac_pci.c +0 -0

similarity index 100%
rename from drivers/dma/dw_dmac_pci.c
rename to drivers/dma/dw/dw_dmac_pci.c

drivers/dma/dw/dw_dmac_regs.h renamed from drivers/dma/dw_dmac_regs.h +0 -0

similarity index 100%
rename from drivers/dma/dw_dmac_regs.h
rename to drivers/dma/dw/dw_dmac_regs.h

Signature · 1 line

1.7.10.4

Felipe Balbi

2012-09-27 07:55 UTC #39 lore raw

Re: [PATCHv3 6/7] dma: move dw_dmac driver to an own directory

Quoted 1 line · Andy Shevchenko

The dw_dmac driver contains multiple files. To make a managment of them more

typo: 'management'

Quoted 70 lines

convenient move it to an own directory.

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

---
 drivers/dma/Makefile                |    3 +--
 drivers/dma/dw/Makefile             |    2 ++
 drivers/dma/{ => dw}/dw_dmac.c      |    2 +-
 drivers/dma/{ => dw}/dw_dmac_pci.c  |    0
 drivers/dma/{ => dw}/dw_dmac_regs.h |    0
 5 files changed, 4 insertions(+), 3 deletions(-)
 create mode 100644 drivers/dma/dw/Makefile
 rename drivers/dma/{ => dw}/dw_dmac.c (99%)
 rename drivers/dma/{ => dw}/dw_dmac_pci.c (100%)
 rename drivers/dma/{ => dw}/dw_dmac_regs.h (100%)

drivers/dma/Makefile +1 -2

index 15eef5f..122a48a 100644
@@ -11,8 +11,7 @@ obj-$(CONFIG_INTEL_IOP_ADMA) += iop-adma.o
 obj-$(CONFIG_FSL_DMA) += fsldma.o
 obj-$(CONFIG_MPC512X_DMA) += mpc512x_dma.o
 obj-$(CONFIG_MV_XOR) += mv_xor.o
-obj-$(CONFIG_DW_DMAC) += dw_dmac.o
-obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o
+obj-$(CONFIG_DW_DMAC) += dw/
 obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o

drivers/dma/dw/Makefile new file +2 -0

new file mode 100644
index 0000000..2edfb24
@@ -0,0 +1,2 @@
+obj-$(CONFIG_DW_DMAC) += dw_dmac.o
+obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o

drivers/dma/dw/dw_dmac.c renamed from drivers/dma/dw_dmac.c +1 -1

similarity index 99%
rename from drivers/dma/dw_dmac.c
rename to drivers/dma/dw/dw_dmac.c
index 9f0129d..fa0471a 100644
@@ -24,8 +24,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#include "../dmaengine.h"
 #include "dw_dmac_regs.h"
-#include "dmaengine.h"
 
 /*
  * This supports the Synopsys "DesignWare AHB Central DMA Controller",

drivers/dma/dw/dw_dmac_pci.c renamed from drivers/dma/dw_dmac_pci.c +0 -0

similarity index 100%
rename from drivers/dma/dw_dmac_pci.c
rename to drivers/dma/dw/dw_dmac_pci.c

drivers/dma/dw/dw_dmac_regs.h renamed from drivers/dma/dw_dmac_regs.h +0 -0

similarity index 100%
rename from drivers/dma/dw_dmac_regs.h
rename to drivers/dma/dw/dw_dmac_regs.h

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signature · 1 line

balbi

viresh kumar

Acked-by on v3 6/7

2012-09-27 08:22 UTC #40 lore raw

Re: [PATCHv3 6/7] dma: move dw_dmac driver to an own directory

In reply to Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

The dw_dmac driver contains multiple files. To make a managment of them more convenient move it to an own directory.

Looks readable now :)

Acked-by Viresh Kumar <[email protected]>

(After fixing Felipe's comment)

Felipe Balbi

Reviewed-by

2012-09-27 08:44 UTC #41 lore raw

Re: [PATCHv3 6/7] dma: move dw_dmac driver to an own directory

Quoted 10 lines · viresh kumar

Quoted 3 lines · Andy Shevchenko
The dw_dmac driver contains multiple files. To make a managment of them more convenient move it to an own directory.
Looks readable now :)
Acked-by Viresh Kumar <[email protected]>
(After fixing Felipe's comment)

And here's mine (after fixing the typo):

Reviewed-by Felipe Balbi <[email protected]>

Signature · 1 line

balbi

Andy Shevchenko

author reviewer

2012-09-27 07:32 UTC #42 lore raw

PATCH v3 7/7 MAINTAINERS: add recently created files to dw_dmac section

In reply to Andy Shevchenko

Append myself to the mail entry of the section as well.

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

---
 MAINTAINERS |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

MAINTAINERS +2 -2

index 7dfd0eb..b87cbb1d 100644
@@ -5999,10 +5999,10 @@ F:	drivers/tty/serial
 
 SYNOPSYS DESIGNWARE DMAC DRIVER
 M:	Viresh Kumar <[email protected]>
+M:	Andy Shevchenko <[email protected]>
 S:	Maintained
 F:	include/linux/dw_dmac.h
-F:	drivers/dma/dw_dmac_regs.h
-F:	drivers/dma/dw_dmac.c
+F:	drivers/dma/dw/
 
 TIMEKEEPING, NTP
 M:	John Stultz <[email protected]>

Signature · 1 line

1.7.10.4

Felipe Balbi

2012-09-27 07:56 UTC #43 lore raw

Re: [PATCHv3 7/7] MAINTAINERS: add recently created files to dw_dmac section

Quoted 12 lines · Andy Shevchenko

Append myself to the mail entry of the section as well.
Signed-off-by Andy Shevchenko <[email protected]>
---
 MAINTAINERS |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
MAINTAINERS +0 -0
index 7dfd0eb..b87cbb1d 100644

MAINTAINERS F: drivers/tty/serial · line 6002

Quoted 4 lines

MAINTAINERS +1 -0

@@ -5999,10 +5999,10 @@ F:	drivers/tty/serial
 
 SYNOPSYS DESIGNWARE DMAC DRIVER
 M:	Viresh Kumar <[email protected]>
+M:	Andy Shevchenko <[email protected]>

this needs to be agreed with Viresh first, I guess. Not sure if it was done or not. If it was, sorry for the noise ;-)

Signature · 1 line

balbi

viresh kumar

2012-09-27 08:12 UTC #44 lore raw

Re: [PATCHv3 7/7] MAINTAINERS: add recently created files to dw_dmac section

Quoted 14 lines · Felipe Balbi

Quoted 13 lines · Andy Shevchenko
Append myself to the mail entry of the section as well.
Signed-off-by Andy Shevchenko <[email protected]>
---
 MAINTAINERS |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
MAINTAINERS +0 -0
index 7dfd0eb..b87cbb1d 100644
@@ -5999,10 +5999,10 @@ F:    drivers/tty/serial
 

Quoted 6 lines

MAINTAINERS F: drivers/tty/serial · line 6002
Quoted 3 lines
MAINTAINERS +1 -0
 SYNOPSYS DESIGNWARE DMAC DRIVER
 M:   Viresh Kumar <[email protected]>
+M:   Andy Shevchenko <[email protected]>
this needs to be agreed with Viresh first, I guess. Not sure if it was done or not. If it was, sorry for the noise ;-)

@Andy: It's not that i don't want anybody else to be a Maintainer of this module, but i am
a bit confused with what is written in description of MAINTAINERS file...

M: is not considered for Mail normally, but as Maintainer - who is bound to look and review
at all the patches, Has right to Ack or Nack something (Only if he is on the logical side :) )

Suppose, if there are 10-20 people from different companies, who want to have themselves
in cc for patches to a specific driver/subsystem, Will all of them add themselves to
MAINTAINERS file? I guess NO.

If you only want yourself to be in cc, then ideally after these patches you should be there
without these changes.

People must cc after running scripts/get_maintainers.pl and it should return your name too,
if i am not incorrect.

But if you want to be a MAINTAINER explicitly, which i believe you are not (looking at the commit
log), then this change is fine.

--
viresh

Andy Shevchenko

2012-09-27 08:24 UTC #45 lore raw

Re: [PATCHv3 7/7] MAINTAINERS: add recently created files to dw_dmac section

In reply to Felipe Balbi

Quoted 14 lines · Felipe Balbi

Quoted 13 lines · Andy Shevchenko
Append myself to the mail entry of the section as well.
Signed-off-by Andy Shevchenko <[email protected]>
---
 MAINTAINERS |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
MAINTAINERS +0 -0
index 7dfd0eb..b87cbb1d 100644
@@ -5999,10 +5999,10 @@ F:    drivers/tty/serial
 

Quoted 6 lines

MAINTAINERS F: drivers/tty/serial · line 6002
Quoted 3 lines
MAINTAINERS +1 -0
 SYNOPSYS DESIGNWARE DMAC DRIVER
 M:   Viresh Kumar <[email protected]>
+M:   Andy Shevchenko <[email protected]>
this needs to be agreed with Viresh first, I guess. Not sure if it was done or not. If it was, sorry for the noise ;-)

Indeed, it's under discussion. I just have a talk with colleagues, and they mostly convinced me in unnecessity of that change.

Signature · 2 lines

With Best Regards,
Andy Shevchenko