[PATCHv3 0/7] dw_dmac: split the driver and introduce PCI part

[PATCHv3 0/7] dw_dmac: split the driver and introduce PCI part

[Andy Shevchenko]

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%)

-- 
1.7.10.4

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

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c4b0eb3..0b88ced 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -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");
-- 
1.7.10.4

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

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 0b88ced..bbb2a82 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -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)");
-- 
1.7.10.4

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

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/Kconfig |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 677cd6e..df32537 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -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
-- 
1.7.10.4

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

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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 <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index bbb2a82..9f0129d 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -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;
 }
 
-- 
1.7.10.4

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

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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 <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index df32537..a5c7f64 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -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
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 7428fea..15eef5f 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -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
diff --git a/drivers/dma/dw_dmac_pci.c b/drivers/dma/dw_dmac_pci.c
new file mode 100644
index 0000000..b7ad37f
--- /dev/null
+++ b/drivers/dma/dw_dmac_pci.c
@@ -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.
+ */
+
+#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,
+};
+
+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;
+	static int instance;
+	int ret;
+
+	ret = pci_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_set_master(pdev);
+	pci_try_set_mwi(pdev);
+
+	ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+	if (ret)
+		goto err0;
+
+	ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
+	if (ret)
+		goto err0;
+
+	pd = platform_device_alloc("dw_dmac", instance);
+	if (!pd) {
+		dev_err(&pdev->dev, "can't allocate dw_dmac platform device\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
+	memset(r, 0, sizeof(r));
+
+	r[0].start = pci_resource_start(pdev, 0);
+	r[0].end = pci_resource_end(pdev, 0);
+	r[0].flags = IORESOURCE_MEM;
+
+	r[1].start = pdev->irq;
+	r[1].flags = IORESOURCE_IRQ;
+
+	ret = platform_device_add_resources(pd, r, ARRAY_SIZE(r));
+	if (ret) {
+		dev_err(&pdev->dev, "can't add resources to platform device\n");
+		goto err1;
+	}
+
+	ret = platform_device_add_data(pd, driver, sizeof(*driver));
+	if (ret)
+		goto err1;
+
+	dma_set_coherent_mask(&pd->dev, pdev->dev.coherent_dma_mask);
+	pd->dev.dma_mask = pdev->dev.dma_mask;
+	pd->dev.dma_parms = pdev->dev.dma_parms;
+	pd->dev.parent = &pdev->dev;
+
+	pci_set_drvdata(pdev, pd);
+
+	ret = platform_device_add(pd);
+	if (ret) {
+		dev_err(&pdev->dev, "platform_device_add failed\n");
+		goto err1;
+	}
+
+	instance++;
+	return 0;
+
+err1:
+	platform_device_put(pd);
+err0:
+	pci_disable_device(pdev);
+
+	return ret;
+}
+
+static void __devexit dw_pci_remove(struct pci_dev *pdev)
+{
+	struct platform_device *pd = pci_get_drvdata(pdev);
+
+	platform_device_unregister(pd);
+	pci_set_drvdata(pdev, NULL);
+	pci_disable_device(pdev);
+}
+
+static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = {
+	{ PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&pdata },
+	{ PCI_VDEVICE(INTEL, 0x0830), (kernel_ulong_t)&pdata },
+	{ PCI_VDEVICE(INTEL, 0x0f06), (kernel_ulong_t)&pdata },
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, dw_pci_id_table);
+
+static struct pci_driver dw_pci_driver = {
+	.name		= "dw_dmac_pci",
+	.id_table	= dw_pci_id_table,
+	.probe		= dw_pci_probe,
+	.remove		= __devexit_p(dw_pci_remove),
+};
+
+module_pci_driver(dw_pci_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("DesignWare DMAC PCI driver");
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
-- 
1.7.10.4

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

[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 <andriy.shevchenko@linux.intel.com>
---
 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%)

diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 15eef5f..122a48a 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -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
diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile
new file mode 100644
index 0000000..2edfb24
--- /dev/null
+++ b/drivers/dma/dw/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_DW_DMAC) += dw_dmac.o
+obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw/dw_dmac.c
similarity index 99%
rename from drivers/dma/dw_dmac.c
rename to drivers/dma/dw/dw_dmac.c
index 9f0129d..fa0471a 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw/dw_dmac.c
@@ -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",
diff --git a/drivers/dma/dw_dmac_pci.c b/drivers/dma/dw/dw_dmac_pci.c
similarity index 100%
rename from drivers/dma/dw_dmac_pci.c
rename to drivers/dma/dw/dw_dmac_pci.c
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw/dw_dmac_regs.h
similarity index 100%
rename from drivers/dma/dw_dmac_regs.h
rename to drivers/dma/dw/dw_dmac_regs.h
-- 
1.7.10.4

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 MAINTAINERS |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7dfd0eb..b87cbb1d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5999,10 +5999,10 @@ F:	drivers/tty/serial
 
 SYNOPSYS DESIGNWARE DMAC DRIVER
 M:	Viresh Kumar <viresh.linux@gmail.com>
+M:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 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 <johnstul@us.ibm.com>
-- 
1.7.10.4

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On Thu, Sep 27, 2012 at 10:31:55AM +0300, Andy Shevchenko wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 

commit log would be great.

> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index c4b0eb3..0b88ced 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -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.

>  	.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");
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

Hi,

On Thu, Sep 27, 2012 at 10:31:57AM +0300, Andy Shevchenko wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This driver could be used on different platforms. Thus, the HAVE_CLK dependency
> is dropped away.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/Kconfig |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 677cd6e..df32537 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On Thu, Sep 27, 2012 at 10:31:58AM +0300, Andy Shevchenko wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> 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 <heikki.krogerus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index bbb2a82..9f0129d 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]

On Thu, Sep 27, 2012 at 10:31:59AM +0300, Andy Shevchenko wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> 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 <heikki.krogerus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index df32537..a5c7f64 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -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
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 7428fea..15eef5f 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -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
> diff --git a/drivers/dma/dw_dmac_pci.c b/drivers/dma/dw_dmac_pci.c
> new file mode 100644
> index 0000000..b7ad37f
> --- /dev/null
> +++ b/drivers/dma/dw_dmac_pci.c
> @@ -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.
> + */
> +
> +#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.

> +	.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.

> +
> +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

> +	static int instance;

this could be a IDA instead.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]

On Thu, Sep 27, 2012 at 10:32:00AM +0300, Andy Shevchenko wrote:
> The dw_dmac driver contains multiple files. To make a managment of them more

typo: 'management'

> convenient move it to an own directory.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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%)
> 
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 15eef5f..122a48a 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -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
> diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile
> new file mode 100644
> index 0000000..2edfb24
> --- /dev/null
> +++ b/drivers/dma/dw/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_DW_DMAC) += dw_dmac.o
> +obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw/dw_dmac.c
> similarity index 99%
> rename from drivers/dma/dw_dmac.c
> rename to drivers/dma/dw/dw_dmac.c
> index 9f0129d..fa0471a 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw/dw_dmac.c
> @@ -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",
> diff --git a/drivers/dma/dw_dmac_pci.c b/drivers/dma/dw/dw_dmac_pci.c
> similarity index 100%
> rename from drivers/dma/dw_dmac_pci.c
> rename to drivers/dma/dw/dw_dmac_pci.c
> diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw/dw_dmac_regs.h
> similarity index 100%
> rename from drivers/dma/dw_dmac_regs.h
> rename to drivers/dma/dw/dw_dmac_regs.h
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Thu, Sep 27, 2012 at 10:32:01AM +0300, Andy Shevchenko wrote:
> Append myself to the mail entry of the section as well.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  MAINTAINERS |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7dfd0eb..b87cbb1d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5999,10 +5999,10 @@ F:	drivers/tty/serial
>  
>  SYNOPSYS DESIGNWARE DMAC DRIVER
>  M:	Viresh Kumar <viresh.linux@gmail.com>
> +M:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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 ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

Hi,

On Thu, Sep 27, 2012 at 11:04:54AM +0300, Andy Shevchenko wrote:
> On Thu, Sep 27, 2012 at 10:42 AM, Felipe Balbi <balbi@ti.com> wrote:
> >> -     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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Andy Shevchenko]

On Thu, Sep 27, 2012 at 10:42 AM, Felipe Balbi <balbi@ti.com> wrote:
>> -     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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Sep 27, 2012 at 10:49 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Sep 27, 2012 at 10:31:59AM +0300, Andy Shevchenko wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This is the PCI part of the DesignWare DMAC driver. The controller is usually
>> used in the Intel hardware such as Medfield.


>> --- /dev/null
>> +++ b/drivers/dma/dw_dmac_pci.c
>> @@ -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.
>> + */
>> +
>> +#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.

>> +     .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.

>> +     static int instance;
> this could be a IDA instead.
Will look at it, thanks.

-- 
With Best Regards,
Andy Shevchenko

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Hi,

On Thu, Sep 27, 2012 at 11:08:12AM +0300, Andy Shevchenko wrote:
> >> +     .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 ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[viresh kumar]

On Thu, Sep 27, 2012 at 1:21 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Sep 27, 2012 at 10:32:01AM +0300, Andy Shevchenko wrote:
>> Append myself to the mail entry of the section as well.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  MAINTAINERS |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7dfd0eb..b87cbb1d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5999,10 +5999,10 @@ F:    drivers/tty/serial
>>
>>  SYNOPSYS DESIGNWARE DMAC DRIVER
>>  M:   Viresh Kumar <viresh.linux@gmail.com>
>> +M:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> 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

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

[viresh kumar]

On Thu, Sep 27, 2012 at 1:10 PM, Felipe Balbi <balbi@ti.com> wrote:
>> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

>> @@ -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.

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Thu, Sep 27, 2012 at 01:46:17PM +0530, viresh kumar wrote:
> On Thu, Sep 27, 2012 at 1:10 PM, Felipe Balbi <balbi@ti.com> wrote:
> >> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> 
> >> @@ -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.

after this change you can also add:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Thu, Sep 27, 2012 at 01:46:57PM +0530, viresh kumar wrote:
> On Thu, Sep 27, 2012 at 1:01 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > It's good to have a quasistatic name for the platform driver.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dw_dmac.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > index 0b88ced..bbb2a82 100644
> > --- a/drivers/dma/dw_dmac.c
> > +++ b/drivers/dma/dw_dmac.c
> > @@ -1712,6 +1712,7 @@ static struct platform_driver dw_driver = {
> >
> >  module_platform_driver(dw_driver);
> >
> > +MODULE_ALIAS("platform:dw_dmac");
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[viresh kumar]

On Thu, Sep 27, 2012 at 1:01 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> It's good to have a quasistatic name for the platform driver.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index 0b88ced..bbb2a82 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -1712,6 +1712,7 @@ static struct platform_driver dw_driver = {
>
>  module_platform_driver(dw_driver);
>
> +MODULE_ALIAS("platform:dw_dmac");

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Thu, Sep 27, 2012 at 01:47:58PM +0530, viresh kumar wrote:
> On Thu, Sep 27, 2012 at 1:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Thu, Sep 27, 2012 at 10:31:57AM +0300, Andy Shevchenko wrote:
> >> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>
> >> This driver could be used on different platforms. Thus, the HAVE_CLK dependency
> >> is dropped away.
> >>
> >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> ---
> >>  drivers/dma/Kconfig |    1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> >> index 677cd6e..df32537 100644
> >> --- a/drivers/dma/Kconfig
> >> +++ b/drivers/dma/Kconfig
> >> @@ -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.
> 
> No, it will not due to following commit id:
> 
> commit 93abe8e4b13ae9a0428ce940a8a03ac72a7626f1
> Author: Viresh Kumar <viresh.kumar@st.com>
> Date:   Mon Jul 30 14:39:27 2012 -0700
> 
>     clk: add non CONFIG_HAVE_CLK routines
> 
> 
> @Andy:
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[viresh kumar]

On Thu, Sep 27, 2012 at 1:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Sep 27, 2012 at 10:31:57AM +0300, Andy Shevchenko wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This driver could be used on different platforms. Thus, the HAVE_CLK dependency
>> is dropped away.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/dma/Kconfig |    1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index 677cd6e..df32537 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -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.

No, it will not due to following commit id:

commit 93abe8e4b13ae9a0428ce940a8a03ac72a7626f1
Author: Viresh Kumar <viresh.kumar@st.com>
Date:   Mon Jul 30 14:39:27 2012 -0700

    clk: add non CONFIG_HAVE_CLK routines


@Andy:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

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

[viresh kumar]

On Thu, Sep 27, 2012 at 1:15 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Sep 27, 2012 at 10:31:58AM +0300, Andy Shevchenko wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> 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 <heikki.krogerus@linux.intel.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/dma/dw_dmac.c |    7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>> index bbb2a82..9f0129d 100644
>> --- a/drivers/dma/dw_dmac.c
>> +++ b/drivers/dma/dw_dmac.c
>> @@ -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

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

[viresh kumar]

On Thu, Sep 27, 2012 at 1:02 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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 <viresh.kumar@linaro.org>
(After fixing Felipe's comment)

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

[Andy Shevchenko]

On Thu, Sep 27, 2012 at 10:51 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Sep 27, 2012 at 10:32:01AM +0300, Andy Shevchenko wrote:
>> Append myself to the mail entry of the section as well.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  MAINTAINERS |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7dfd0eb..b87cbb1d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5999,10 +5999,10 @@ F:    drivers/tty/serial
>>
>>  SYNOPSYS DESIGNWARE DMAC DRIVER
>>  M:   Viresh Kumar <viresh.linux@gmail.com>
>> +M:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Sep 27, 2012 at 11:19 AM, viresh kumar <viresh.kumar@linaro.org> wrote:
> On Thu, Sep 27, 2012 at 1:15 PM, Felipe Balbi <balbi@ti.com> wrote:
>> On Thu, Sep 27, 2012 at 10:31:58AM +0300, Andy Shevchenko wrote:
>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>
>>> 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 <heikki.krogerus@linux.intel.com>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>  drivers/dma/dw_dmac.c |    7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>>> index bbb2a82..9f0129d 100644
>>> --- a/drivers/dma/dw_dmac.c
>>> +++ b/drivers/dma/dw_dmac.c
>>> @@ -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 :)
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.

> @Vinod: Can we have some inputs from you here?
Agree, it's good to have an additional opinion here.

-- 
With Best Regards,
Andy Shevchenko

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

On Thu, Sep 27, 2012 at 01:52:24PM +0530, viresh kumar wrote:
> On Thu, Sep 27, 2012 at 1:02 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 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 <viresh.kumar@linaro.org>
> (After fixing Felipe's comment)

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

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vinod Koul]

On Thu, 2012-09-27 at 10:31 +0300, Andy Shevchenko wrote:
> +
> +#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

> +
> +static int __devinit dw_pci_probe(struct pci_dev *pdev,
> +				  const struct pci_device_id *id)
> +{
...
> +
> +	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?
> +	if (!pd) {
> +		dev_err(&pdev->dev, "can't allocate dw_dmac platform device\n");
> +		ret = -ENOMEM;
> +		goto err0;
> +	}

-- 
~Vinod

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

[Andy Shevchenko]

On Thu, 2012-09-27 at 14:14 +0530, Vinod Koul wrote: 
> > +static int __devinit dw_pci_probe(struct pci_dev *pdev,
> > +				  const struct pci_device_id *id)
> > +{
> ...
> > +
> > +	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?
I'm afraid I didn't get you comment.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Vinod Koul]

On Thu, 2012-09-27 at 12:43 +0300, Andy Shevchenko wrote:
> On Thu, 2012-09-27 at 14:14 +0530, Vinod Koul wrote: 
> > > +static int __devinit dw_pci_probe(struct pci_dev *pdev,
> > > +				  const struct pci_device_id *id)
> > > +{
> > ...
> > > +
> > > +	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?
> 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.

-- 
~Vinod

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

[viresh kumar]

On Thu, Sep 27, 2012 at 3:31 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
> 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

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

[Vinod Koul]

On Thu, 2012-09-27 at 15:41 +0530, viresh kumar wrote:
> On Thu, Sep 27, 2012 at 3:31 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
> > 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*...
Yes that IMHO would be simpler approach :)


-- 
~Vinod

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

[Andy Shevchenko]

On Thu, 2012-09-27 at 10:40 +0300, Felipe Balbi wrote: 
> On Thu, Sep 27, 2012 at 10:31:55AM +0300, Andy Shevchenko wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> 
> commit log would be great.
Will do.

> >  static struct platform_driver dw_driver = {
> > +	.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?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

Hi,

On Mon, Oct 01, 2012 at 01:47:17PM +0300, Andy Shevchenko wrote:
> On Thu, 2012-09-27 at 10:40 +0300, Felipe Balbi wrote: 
> > On Thu, Sep 27, 2012 at 10:31:55AM +0300, Andy Shevchenko wrote:
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > 
> > commit log would be great.
> Will do.
> 
> > >  static struct platform_driver dw_driver = {
> > > +	.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?

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

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Andy Shevchenko]

On Thu, Sep 27, 2012 at 1:32 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
> On Thu, 2012-09-27 at 15:41 +0530, viresh kumar wrote:
>> On Thu, Sep 27, 2012 at 3:31 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
>> > 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*...
> Yes that IMHO would be simpler approach :)
Oh, thanks, I got the idea. Will implement soon.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, 2012-09-27 at 15:41 +0530, viresh kumar wrote: 
> On Thu, Sep 27, 2012 at 3:31 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
> > 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 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. 


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Viresh Kumar]

On 8 October 2012 15:47, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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.

> 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.

> 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?

> 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

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

[Andy Shevchenko]

On Mon, 2012-10-08 at 16:19 +0530, Viresh Kumar wrote: 
> On 8 October 2012 15:47, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 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.

> > 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?

> > 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.

> 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.

> 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.



-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Viresh Kumar]

On 8 October 2012 18:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2012-10-08 at 16:19 +0530, Viresh Kumar wrote:
>> > 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?

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.

>> 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.
>
>> 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

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

[Vinod Koul]

On Mon, 2012-10-08 at 18:57 +0530, Viresh Kumar wrote:
> 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?
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.


-- 
Vinod Koul
Intel Corp.

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

[Andy Shevchenko]

On Mon, 2012-10-08 at 18:57 +0530, Viresh Kumar wrote: 
> On 8 October 2012 18:16, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, 2012-10-08 at 16:19 +0530, Viresh Kumar wrote:
> >> > 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?
> 
> 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. 

> >> 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.

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

> 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.

> 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.

> - 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.

> - 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?).

> - 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.

> 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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Viresh Kumar]

On 30 October 2012 20:35, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2012-10-08 at 18:57 +0530, Viresh Kumar wrote:

>> 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.

>> 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.

>> 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.

>> - 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 :)

>> - 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.

>> - 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.

> 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

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

[Andy Shevchenko]

On Tue, Oct 30, 2012 at 5:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 30 October 2012 20:35, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> 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.

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

-- 
With Best Regards,
Andy Shevchenko