dw_dmac: split the driver and introduce PCI part

From: Heikki Krogerus <[email protected]>

Not all platforms support clk API.

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

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

---
 drivers/dma/Kconfig        |    1 -
 drivers/dma/dw_dmac.c      |   18 +++---------------
 drivers/dma/dw_dmac_regs.h |    1 -
 3 files changed, 3 insertions(+), 17 deletions(-)

drivers/dma/Kconfig +0 -0

index 677cd6e..df32537 100644

drivers/dma/dw_dmac.c +0 -0

index 9316d03..d9344a7 100644
@@ -1,9 +1,9 @@
 /*

drivers/dma/dw_dmac.c +1 -13

@@ -12,7 +12,6 @@
 #define VERBOSE_DEBUG
 #define DEBUG
 #include <linux/bitops.h>
-#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>

@@ -224,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);

@@ -1508,11 +1506,6 @@ static int __devinit dw_probe(struct platform_device *pdev)
        if (!dw)
                return -ENOMEM;
 
-       dw->clk = devm_clk_get(&pdev->dev, "hclk");
-       if (IS_ERR(dw->clk))
-               return PTR_ERR(dw->clk);
-       clk_prepare_enable(dw->clk);
-
        dw->regs = regs;
 
        /* get hardware configuration parameters */

@@ -1657,19 +1650,14 @@ static int __devexit dw_remove(struct platform_device *pdev)
 
 static void dw_shutdown(struct platform_device *pdev)
 {
-       struct dw_dma   *dw = platform_get_drvdata(pdev);
-
        dw_dma_off(platform_get_drvdata(pdev));
-       clk_disable_unprepare(dw->clk);
 }
 
 static int dw_suspend_noirq(struct device *dev)
 {
        struct platform_device *pdev = to_platform_device(dev);
-       struct dw_dma   *dw = platform_get_drvdata(pdev);
 
        dw_dma_off(platform_get_drvdata(pdev));
-       clk_disable_unprepare(dw->clk);
 
        return 0;
 }

@@ -1679,8 +1667,8 @@ static int dw_resume_noirq(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_dma   *dw = platform_get_drvdata(pdev);
 
-       clk_prepare_enable(dw->clk);
        dma_writel(dw, CFG, DW_CFG_DMA_EN);
+
        return 0;
 }
 

drivers/dma/dw_dmac_regs.h +0 -0

index ff39fa6..50e0b63 100644

Quoted 2 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]> Not all platforms support clk API.

But not these. There are dummy clk routines available now for platforms which don't support clk framework. So, even if your platform doesn't support clk framework, you should be able to compile your code.

From: Heikki Krogerus <[email protected]>

This driver should be usable on all platforms that depend on clk API.

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.c      |   23 +------
 drivers/dma/dw_dmac_at32.c |  150 ++++++++++++++++++++++++++++++++++++++++++++

drivers/dma/Kconfig +6 -0

+       select DW_DMAC
+       default y if CPU_AT32AP7000
+       help
+         Support the Synopsys DesignWare AHB DMA controller in the chips
+         such as the Atmel AT32ap7000.
+
 config AT_HDMAC
        tristate "Atmel AHB DMA support"
        depends on ARCH_AT91

drivers/dma/dw_dmac.c +1 -12

index d9344a7..9c8a500 100644
@@ -18,7 +18,6 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/of.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>

@@ -1681,35 +1680,17 @@ static const struct dev_pm_ops dw_dev_pm_ops = {
        .poweroff_noirq = dw_suspend_noirq,
 };
 
-#ifdef CONFIG_OF
-static const struct of_device_id dw_dma_id_table[] = {
-       { .compatible = "snps,dma-spear1340" },
-       {}
-};
-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 = {
                .name   = "dw_dmac",
                .pm     = &dw_dev_pm_ops,
-               .of_match_table = of_match_ptr(dw_dma_id_table),
        },
 };
 
-static int __init dw_init(void)
-{

drivers/dma/dw_dmac_at32.c new file +33 -0

+ *
+ * The driver is based on the excerpts from the original dw_dmac.c. That's why
+ * the same copyright holders are mentioned here as well.
+ *
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/dw_dmac.h>
+
+struct dw_at32 {
+       struct platform_device  *pdev;
+       struct clk              *clk;
+};
+
+static int __init dw_at32_probe(struct platform_device *pdev)
+{
+       struct dw_at32                  *at32;
+       struct platform_device          *pd;
+       struct dw_dma_platform_data     *pdata = pdev->dev.platform_data;
+       static int                      instance;
+       int                             ret;
+
+       at32 = devm_kzalloc(&pdev->dev, sizeof(*at32), GFP_KERNEL);

Quoted 3 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>

This driver should be usable on all platforms that depend on clk API.

This is not what you mentioned in subject :(

Quoted 7 lines

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.c      |   23 +------
 drivers/dma/dw_dmac_at32.c |  150 ++++++++++++++++++++++++++++++++++++++++++++

I don't agree with the naming used here. There is nothing AT32 specific here. It is actively used
by SPEAr. It should be named dw_dmac_platform.c or *pltfm.c

Quoted 9 lines

 4 files changed, 162 insertions(+), 21 deletions(-)
 create mode 100644 drivers/dma/dw_dmac_at32.c

drivers/dma/Kconfig +0 -0

index df32537..e47cc90 100644

drivers/dma/Kconfig config DW_DMAC · line 93

Quoted 5 lines

drivers/dma/Kconfig +2 -0

@@ -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_AT32
+       tristate "Synopsys DesignWare AHB DMA support for Atmel"

:(

drivers/dma/Kconfig config DW_DMAC · line 94

Quoted 1 line

drivers/dma/Kconfig +1 -0

+       depends on HAVE_CLK

Even this is not a must for all users of platform driver. So can leave this.

Quoted 47 lines

drivers/dma/Kconfig +6 -0

+       select DW_DMAC
+       default y if CPU_AT32AP7000
+       help
+         Support the Synopsys DesignWare AHB DMA controller in the chips
+         such as the Atmel AT32ap7000.
+
 config AT_HDMAC
        tristate "Atmel AHB DMA support"
        depends on ARCH_AT91

drivers/dma/dw_dmac.c +1 -12

index d9344a7..9c8a500 100644
@@ -18,7 +18,6 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/of.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>

@@ -1681,35 +1680,17 @@ static const struct dev_pm_ops dw_dev_pm_ops = {
        .poweroff_noirq = dw_suspend_noirq,
 };
 
-#ifdef CONFIG_OF
-static const struct of_device_id dw_dma_id_table[] = {
-       { .compatible = "snps,dma-spear1340" },
-       {}
-};
-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 = {
                .name   = "dw_dmac",
                .pm     = &dw_dev_pm_ops,
-               .of_match_table = of_match_ptr(dw_dma_id_table),
        },
 };
 
-static int __init dw_init(void)
-{

drivers/dma/dw_dmac.c static const struct dev_pm_ops dw_dev_pm_ops = { · line 1693

Quoted 10 lines

drivers/dma/dw_dmac.c +1 -9

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

Shouldn't this be a separate patch?

drivers/dma/dw_dmac_at32.c line 2

Quoted 11 lines

drivers/dma/dw_dmac.c +0 -0

 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Synopsys DesignWare DMA Controller driver");

drivers/dma/dw_dmac_at32.c new file +2 -0

new file mode 100644
index 0000000..7bc7ac4
@@ -0,0 +1,150 @@
+/*
+ * Driver for the Synopsys DesignWare DMA Controller

How is this file different from dw_dmac.c?

Quoted 33 lines

drivers/dma/dw_dmac_at32.c new file +33 -0

+ *
+ * The driver is based on the excerpts from the original dw_dmac.c. That's why
+ * the same copyright holders are mentioned here as well.
+ *
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/dw_dmac.h>
+
+struct dw_at32 {
+       struct platform_device  *pdev;
+       struct clk              *clk;
+};
+
+static int __init dw_at32_probe(struct platform_device *pdev)
+{
+       struct dw_at32                  *at32;
+       struct platform_device          *pd;
+       struct dw_dma_platform_data     *pdata = pdev->dev.platform_data;
+       static int                      instance;
+       int                             ret;
+
+       at32 = devm_kzalloc(&pdev->dev, sizeof(*at32), GFP_KERNEL);

drivers/dma/dw_dmac_at32.c line 45

Quoted 10 lines

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

+       if (!at32) {
+               dev_err(&pdev->dev, "can't allocate memory\n");
+               return -ENOMEM;
+       }
+
+       pd = platform_device_alloc("dw_dmac", instance);
+       if (!pd) {
+               dev_err(&pdev->dev, "can't allocate dw_dmac platform device\n");
+               return -ENOMEM;
+       }

Why create another device? Why not simply call dw_probe() from here?

This separate driver makes no sense in case it is built properly without CLK framework. Let me check this and leave comments at patch 1/6.

Quoted 2 lines · Andy Shevchenko

This separate driver makes no sense in case it is built properly without CLK framework. Let me check this and leave comments at patch 1/6.

Following is the commit that introduced this change :)

commit 93abe8e4b13ae9a0428ce940a8a03ac72a7626f1

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

clk: add non CONFIG_HAVE_CLK routines

Many drivers are shared between architectures that may or may not have HAVE_CLK selected for them. To remove compilation errors for them we enclose clk_*() calls in these drivers within #ifdef CONFIG_HAVE_CLK, #endif.

This patch removes the need of these CONFIG_HAVE_CLK statements, by introducing dummy routines when HAVE_CLK is not selected by platforms. So, definition of these routines will always be available. These calls will return error for platforms that don't select HAVE_CLK.

Signed-off-by Viresh Kumar <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Jeff Garzik <[email protected]>

Cc Andrew Lunn <[email protected]>

Cc Bhupesh Sharma <[email protected]>

Cc Giuseppe Cavallaro <[email protected]>

Cc Russell King <[email protected]>

Cc Mike Turquette <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc viresh kumar <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

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

drivers/dma/dw_dmac_at32.c +0 -0

 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>

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

new file mode 100644
index 0000000..7490894
@@ -0,0 +1,130 @@
+/*
+ * 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>

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

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

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

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

drivers/dma/dw_dmac_pci.c new file +37 -1

+       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), DRIVER(1, 0, 0) },
+       { PCI_VDEVICE(INTEL, 0x0830), DRIVER(1, 0, 0) },
+       { PCI_VDEVICE(INTEL, 0x0f06), DRIVER(1, 0, 0) },
+       { 0, }
+};
+MODULE_DEVICE_TABLE(pci, dw_pci_id_table);
+
+static struct pci_driver dw_pci_driver = {
+       .name           = DW_DRIVER_NAME,
+       .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 <[email protected]>");
+MODULE_AUTHOR("Andy Shevchenko <[email protected]>");
--

1.7.10.4

drivers/dma/dw_dmac_at32.c line 15

Quoted 9 lines · Andy Shevchenko

drivers/dma/dw_dmac_at32.c +1 -0

index 7bc7ac4..5c9180e 100644
@@ -12,6 +12,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+

:(

Quoted 27 lines

drivers/dma/dw_dmac_at32.c +0 -0

 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>

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

new file mode 100644
index 0000000..7490894
@@ -0,0 +1,130 @@
+/*
+ * 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>
+
+#define DW_DRIVER_NAME "dw_dmac_pci"
+

drivers/dma/dw_dmac_pci.c line 23

Quoted 6 lines

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

+#define DRIVER(_is_private, _chan_order, _chan_pri)            \
+       ((kernel_ulong_t)&(struct dw_dma_platform_data) {       \
+               .is_private = (_is_private),                    \
+               .chan_allocation_order = (_chan_order),         \
+               .chan_priority = (_chan_pri),                   \
+       })

See if you can align "\" at the end of every line in one column

drivers/dma/dw_dmac_pci.c line 28

Quoted 6 lines

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

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

no need of multiple spaces before *pd.

drivers/dma/dw_dmac_pci.c line 32

Quoted 4 lines

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

+       struct resource                 r[2];
+       struct dw_dma_platform_data     *driver = (void *)id->driver_data;
+       static int                      instance;
+       int                             ret;

for all above lines too

Quoted 20 lines

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

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

drivers/dma/dw_dmac_pci.c line 61

Quoted 10 lines

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

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

ditto

drivers/dma/dw_dmac_pci.c line 64

Quoted 3 lines

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

+
+       r[1].start      = pdev->irq;
+       r[1].flags      = IORESOURCE_IRQ;

ditto

Quoted 23 lines

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

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

drivers/dma/dw_dmac_pci.c line 93

Quoted 6 lines

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

+
+       instance++;
+       return 0;
+
+err1:
+       pci_set_drvdata(pdev, NULL);

Is this required?

Quoted 40 lines

drivers/dma/dw_dmac_pci.c new file +37 -1

+       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), DRIVER(1, 0, 0) },
+       { PCI_VDEVICE(INTEL, 0x0830), DRIVER(1, 0, 0) },
+       { PCI_VDEVICE(INTEL, 0x0f06), DRIVER(1, 0, 0) },
+       { 0, }
+};
+MODULE_DEVICE_TABLE(pci, dw_pci_id_table);
+
+static struct pci_driver dw_pci_driver = {
+       .name           = DW_DRIVER_NAME,
+       .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 <[email protected]>");
+MODULE_AUTHOR("Andy Shevchenko <[email protected]>");
--

1.7.10.4

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

Cc Viresh Kumar <[email protected]>

---
 MAINTAINERS |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Quoted 5 lines · Andy Shevchenko

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

Cc Viresh Kumar <[email protected]>

---
 MAINTAINERS |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Acked-by Viresh Kumar <[email protected]>

Though i am not sure if this patch should be part of the set.

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

Quoted 1 line · Andy Shevchenko

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

You also added yourself as a maintainer.
Congrats/sympathies, etc...

MAINTAINERS

Quoted 1 line

MAINTAINERS +0 -0

[]

MAINTAINERS F: drivers/tty/serial · line 6016

Quoted 11 lines

MAINTAINERS +3 -0

@@ -6007,10 +6007,13 @@ 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_dmac_at32.c
+F:	drivers/dma/dw_dmac_pci.c

Perhaps these F: lines instead

F: include/linux/dw_dmac.h
F: drivers/dma/dw_dmac*

Quoted 4 lines

Perhaps these F: lines instead

F:    include/linux/dw_dmac.h
F:    drivers/dma/dw_dmac*

Might be. There is another idea to move them under drivers/dma/dw/ folder. So, Viresh, what do you think is better?

Quoted 2 lines · Joe Perches

You also added yourself as a maintainer.
Congrats/sympathies, etc...

Actually I prefer to be just a supporter, because I introduce few additional files. But if I understood correctly, there is no way to distinguish person statuses in one section.

Quoted 6 lines · Andy Shevchenko

Quoted 2 lines · Joe Perches

You also added yourself as a maintainer.
Congrats/sympathies, etc...

Actually I prefer to be just a supporter, because I introduce few additional files. But if I understood correctly, there is no way to distinguish person statuses in one section.

True, the status refers to the section, not the person.

Supported is a nominally "higher" level of stewardship than maintainer. Supported is supposed to mean you get paid to look after the code.

I think of these section blocks as one of the ways to get the appropriate people to look at patches and bug reports.

Quoted 5 lines · Joe Perches

Quoted 1 line · Andy Shevchenko

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

You also added yourself as a maintainer.
Congrats/sympathies, etc...

Actually I prefer to be just a supporter, because I introduce few additional files. But if I understood correctly, there is no way to distinguish person statuses in one section.

Quoted 16 lines

MAINTAINERS

Quoted 1 line

MAINTAINERS +0 -0

[]

MAINTAINERS F: drivers/tty/serial · line 6016

Quoted 11 lines

MAINTAINERS +3 -0

@@ -6007,10 +6007,13 @@ 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_dmac_at32.c
+F: drivers/dma/dw_dmac_pci.c

Perhaps these F: lines instead

Quoted 2 lines

F:    include/linux/dw_dmac.h
F:    drivers/dma/dw_dmac*

Might be. There is another idea to move them under drivers/dma/dw/ folder. So, Viresh, what do you think is better?

From: Heikki Krogerus <[email protected]>

This driver could be used on different platforms. Thus, we converted it to be a platform driver. The HAVE_CLK dependency is dropped away as well.

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

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

---
 drivers/dma/Kconfig   |    2 --
 drivers/dma/dw_dmac.c |   21 +++++++--------------
 2 files changed, 7 insertions(+), 16 deletions(-)

drivers/dma/Kconfig +0 -0

index 677cd6e..75cdcb5 100644

drivers/dma/Kconfig +0 -0

        help
          Support the Synopsys DesignWare AHB DMA controller.  This
          can be integrated in chips such as the Atmel AT32ap7000.

drivers/dma/dw_dmac.c +3 -2

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

drivers/dma/dw_dmac.c +2 -2

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

@@ -1700,6 +1701,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,18 +1711,9 @@ static struct platform_driver dw_driver = {
        },
 };
 
-static int __init dw_init(void)
-{

drivers/dma/dw_dmac.c +1 -1

+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

Quoted 4 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>

This driver could be used on different platforms. Thus, we converted it to be a platform driver. The HAVE_CLK dependency is dropped away as well.

Commit log and patch content don't match. :(
We aren't converting it to a platform driver, it is already one.

Quoted 12 lines

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

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

---
 drivers/dma/Kconfig   |    2 --
 drivers/dma/dw_dmac.c |   21 +++++++--------------
 2 files changed, 7 insertions(+), 16 deletions(-)

drivers/dma/Kconfig +0 -0

index 677cd6e..75cdcb5 100644

drivers/dma/Kconfig config INTEL_IOP_ADMA · line 86

Quoted 4 lines

drivers/dma/Kconfig +0 -1

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

This matches subject.

drivers/dma/Kconfig config INTEL_IOP_ADMA · line 88

Quoted 2 lines

drivers/dma/Kconfig +0 -1

        select DMA_ENGINE
-       default y if CPU_AT32AP7000

This doesn't. If you want this change, do this in a separate patch.

Quoted 30 lines

drivers/dma/Kconfig +0 -0

        help
          Support the Synopsys DesignWare AHB DMA controller.  This
          can be integrated in chips such as the Atmel AT32ap7000.

drivers/dma/dw_dmac.c +3 -2

index c4b0eb3..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));
 }
 

drivers/dma/dw_dmac.c static inline void dwc_dump_chan_regs(struct dw_dma_chan *dwc) · line 225

Quoted 1 line

drivers/dma/dw_dmac.c +0 -1

-

Above + and - also don't belong to this patch

Quoted 24 lines

drivers/dma/dw_dmac.c +2 -0

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

@@ -1700,6 +1701,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,18 +1711,9 @@ static struct platform_driver dw_driver = {
        },
 };
 

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

Quoted 12 lines

drivers/dma/dw_dmac.c +1 -11

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

This should be a separate patch as well

Quoted 4 lines

drivers/dma/dw_dmac.c +1 -0

+MODULE_ALIAS("platform:dw_dmac");
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Synopsys DesignWare DMA Controller driver");
 MODULE_AUTHOR("Haavard Skinnemoen (Atmel)");

Quoted 9 lines · viresh kumar

Quoted 4 lines · Andy Shevchenko

From: Heikki Krogerus <[email protected]>

This driver could be used on different platforms. Thus, we converted it to be a platform driver. The HAVE_CLK dependency is dropped away as well.

Commit log and patch content don't match. :(
We aren't converting it to a platform driver, it is already one.

Okay, any suggestion for this part?

drivers/dma/dw_dmac_pci.c +23 -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;
+       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;
+

drivers/dma/dw_dmac_pci.c +73 -1

+       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), DRIVER(1, 0, 0) },
+       { PCI_VDEVICE(INTEL, 0x0830), DRIVER(1, 0, 0) },
+       { PCI_VDEVICE(INTEL, 0x0f06), DRIVER(1, 0, 0) },
+       { 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 <[email protected]>");
+MODULE_AUTHOR("Andy Shevchenko <[email protected]>");
--

1.7.10.4

Forgot earlier, please keep spear-devel in cc... They will be going to use dw_dmac driver :)

drivers/dma/dw_dmac_pci.c line 20

Quoted 11 lines · Andy Shevchenko

drivers/dma/dw_dmac_pci.c +10 -0

+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/dw_dmac.h>
+
+#define DRIVER(_is_private, _chan_order, _chan_pri)            \
+       ((kernel_ulong_t)&(struct dw_dma_platform_data) {       \
+               .is_private = (_is_private),                    \
+               .chan_allocation_order = (_chan_order),         \
+               .chan_priority = (_chan_pri),                   \

I believe you don't need these braces around input variables on right side of "=". Even if there is something complex passed.

Did you try to align "\"? Sorry can't see it in gmail :(

drivers/dma/dw_dmac_pci.c line 53

Quoted 6 lines

drivers/dma/dw_dmac_pci.c +6 -0

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

Is this the correct approach? I doubt... We are creating a platform device from a
pci driver... Don't know if it can lead to some issues within kernel.

Lets call the specialist for his comments :)
@Arnd: Can you please help us here?

Quoted 16 lines · viresh kumar

drivers/dma/dw_dmac_pci.c line 20

Quoted 11 lines · Andy Shevchenko

drivers/dma/dw_dmac_pci.c +10 -0

+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/dw_dmac.h>
+
+#define DRIVER(_is_private, _chan_order, _chan_pri)            \
+       ((kernel_ulong_t)&(struct dw_dma_platform_data) {       \
+               .is_private = (_is_private),                    \
+               .chan_allocation_order = (_chan_order),         \
+               .chan_priority = (_chan_pri),                   \

I believe you don't need these braces around input variables on right side of "=". Even if there is something complex passed.

Hmm... Have no idea if anyone will use robust stuff as a parameter to that macro. I could remove them.

Quoted 12 lines

drivers/dma/dw_dmac_pci.c line 53

Quoted 7 lines

drivers/dma/dw_dmac_pci.c +6 -0

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

Is this the correct approach? I doubt... We are creating a platform device from a
pci driver... Don't know if it can lead to some issues within kernel.

Quoted 2 lines

Lets call the specialist for his comments :)
@Arnd: Can you please help us here?

This approach is used among different drivers in kernel. As first example that comes to my mind you could consider chipidea USB driver (drivers/usb/chipidea).

Quoted 6 lines · Andy Shevchenko

Quoted 6 lines

Quoted 5 lines

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

+#define DRIVER(_is_private, _chan_order, _chan_pri)            \
+       ((kernel_ulong_t)&(struct dw_dma_platform_data) {       \
+               .is_private = (_is_private),                    \
+               .chan_allocation_order = (_chan_order),         \
+               .chan_priority = (_chan_pri),                   \

Quoted 4 lines

Quoted 2 lines

I believe you don't need these braces around input variables on right side of "=". Even if there is something complex passed.

Hmm... Have no idea if anyone will use robust stuff as a parameter to that macro. I could remove them.

IMHO It's better to leave them in as general practice in case of a macro.

However, I would prefer not having this macro at all and just open-code the contents. If you use traditional struct initializers, the amount of code is almost exactly the same.

Quoted 11 lines

Quoted 11 lines

drivers/dma/dw_dmac_pci.c line 53

Quoted 6 lines

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

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

Is this the correct approach? I doubt... We are creating a platform device from a
pci driver... Don't know if it can lead to some issues within kernel.

Quoted 5 lines

Quoted 2 lines

Lets call the specialist for his comments :)
@Arnd: Can you please help us here?

This approach is used among different drivers in kernel. As first example that comes to my mind you could consider chipidea USB driver (drivers/usb/chipidea).

Yes, this is fine for a PCI device with multiple logical devices. You could also use the MFD framework if that results in smaller code. For a driver that can be either a platform device or a pci device with no other sub-devices, I would however recommend having a common initialization function for stuff that can be called from either bus_type but with no extra level of indirection.

I believe there is no common initialization part here, because PCI device in any case would be calling probe of platform device. :)

Quoted 2 lines · viresh kumar

I believe there is no common initialization part here, because PCI device in any case would be calling probe of platform device. :)

Looking at the driver more closely now. Right now, it only supports platform devices, and the dw_probe function contains some parts that are related to platform devices and some parts that are needed also in case of PCI.

My suggestion would be that you split the dw_probe function to separate out the bus specific parts and end up with three functions like

int dw_dmac_probe(struct dw_dma *dw);
int dw_dmac_platform_probe(struct platform_device *pdev); int dw_dmac_pci_probe(struct dw_dma *dw);

where the first one is called by the other two, depending on the bus type. This could be done either splitting the driver into multiple files so you can have the platform and pci parts in separate driver modules depending on the common module that does all the actual work, or everything can be put in a single file with an #ifdef CONFIG_PCI to disable the pci driver parts when they don't apply.

Does this make sense?

Arnd
--
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/

Quoted 6 lines

where the first one is called by the other two, depending on the bus type. This could be done either splitting the driver into multiple files so you can have the platform and pci parts in separate driver modules depending on the common module that does all the actual work, or everything can be put in a single file with an #ifdef CONFIG_PCI to disable the pci driver parts when they don't apply.

probe wont be called if PCI device is not loaded, so see no reason why CONFIG_xxx would be needed

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

Quoted 2 lines · Andy Shevchenko

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

git format-patch has a option -C or -M or both... that makes such changes easier to review.
If you create patches with these options, we will not get such long patches for renames....
instead a single line will tell "foo.c file is renamed as dw/foo.c"

So this patch will have only few lines after that.

Please resend with that... as its impossible to review it currently.

Append myself to mail subsection 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 6005

Quoted 8 lines

SYNOPSYS DESIGNWARE DMAC DRIVER
M: Viresh Kumar <[email protected]>

MAINTAINERS +2 -2

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

I can't see this change...

Quoted 10 lines · viresh kumar

MAINTAINERS F: drivers/tty/serial · line 6005

Quoted 8 lines

SYNOPSYS DESIGNWARE DMAC DRIVER
M: Viresh Kumar <[email protected]>

MAINTAINERS +2 -2

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

I can't see this change...

Sorry didn't saw your 3rd patch... :(

I don't know why, but when i get patches from you... they appear in the older threads..
i.e. v2 comes as reply to v1, v3 as reply to v2... so becomes difficult to track them..

You simply do git send-email *.patch?

Quoted 1 line · Andy Shevchenko

Append myself to mail subsection as well.

Mail subsection? You are adding yourself as Maintainer of dw_dmac...

Do you really want that.
I am asking this, because that's not the normal practice followed in kernel.

This isn't done until unless, somebody has done a MAJOR modification to the driver.