From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: viresh kumar <viresh.kumar@linaro.org>
Cc: Vinod Koul <vinod.koul@intel.com>,
linux-kernel@vger.kernel.org,
Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: Re: [PATCHv1 2/6] dmaengine: dw_dmac: add driver for Atmel AT32
Date: Wed, 26 Sep 2012 09:47:48 +0300 [thread overview]
Message-ID: <1348642068.13371.135.camel@smile> (raw)
In-Reply-To: <CAOh2x=kEDfY_DaBdbeY+CREiRJDH5yeNLUUuSpGTZcdWtZEd7g@mail.gmail.com>
On Wed, 2012-09-26 at 09:20 +0530, viresh kumar wrote:
> On Tue, Sep 25, 2012 at 5:43 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > This driver should be usable on all platforms that depend on clk API.
>
> This is not what you mentioned in subject :(
>
> > 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.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
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.
>
> > 4 files changed, 162 insertions(+), 21 deletions(-)
> > create mode 100644 drivers/dma/dw_dmac_at32.c
> >
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index df32537..e47cc90 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_AT32
> > + tristate "Synopsys DesignWare AHB DMA support for Atmel"
>
> :(
>
> > + depends on HAVE_CLK
>
> Even this is not a must for all users of platform driver. So can leave this.
>
> > + 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
>
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > index d9344a7..9c8a500 100644
> > --- a/drivers/dma/dw_dmac.c
> > +++ b/drivers/dma/dw_dmac.c
> > @@ -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)
> > -{
> > - 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?
>
> >
> > MODULE_LICENSE("GPL v2");
> > MODULE_DESCRIPTION("Synopsys DesignWare DMA Controller driver");
> > diff --git a/drivers/dma/dw_dmac_at32.c b/drivers/dma/dw_dmac_at32.c
> > new file mode 100644
> > index 0000000..7bc7ac4
> > --- /dev/null
> > +++ b/drivers/dma/dw_dmac_at32.c
> > @@ -0,0 +1,150 @@
> > +/*
> > + * Driver for the Synopsys DesignWare DMA Controller
>
> How is this file different from dw_dmac.c?
>
> > + *
> > + * 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);
> > + 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?
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
next prev parent reply other threads:[~2012-09-26 6:47 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-25 12:13 [PATCHv1 0/6] dw_dmac: split the driver and introduce PCI part Andy Shevchenko
2012-09-25 12:13 ` [PATCHv1 1/6] dmaengine: dw_dmac: Remove clk API dependency Andy Shevchenko
2012-09-26 3:42 ` viresh kumar
2012-09-26 12:33 ` Andy Shevchenko
2012-09-25 12:13 ` [PATCHv1 2/6] dmaengine: dw_dmac: add driver for Atmel AT32 Andy Shevchenko
2012-09-26 3:50 ` viresh kumar
2012-09-26 6:47 ` Andy Shevchenko [this message]
2012-09-26 6:51 ` viresh kumar
2012-09-26 6:56 ` Andy Shevchenko
2012-09-26 3:52 ` viresh kumar
2012-09-25 12:13 ` [PATCHv1 3/6] dmaengine: dw_dmac: Add PCI part of the driver Andy Shevchenko
2012-09-26 4:00 ` viresh kumar
2012-09-26 7:48 ` Andy Shevchenko
2012-09-25 12:13 ` [PATCHv1 4/6] avr32: at32ap700x: rename DMA controller Andy Shevchenko
2012-09-25 12:13 ` [PATCHv1 5/6] MAINTAINERS: fix indentation for Viresh Kumar Andy Shevchenko
2012-09-26 3:36 ` viresh kumar
2012-09-26 7:10 ` Andy Shevchenko
2012-09-25 12:13 ` [PATCHv1 6/6] MAINTAINERS: add recently created files to dw_dmac section Andy Shevchenko
2012-09-25 13:19 ` Joe Perches
2012-09-25 13:37 ` Andy Shevchenko
2012-09-25 15:33 ` Vinod Koul
2012-09-25 16:57 ` Joe Perches
2012-09-26 6:44 ` Andy Shevchenko
2012-09-26 3:39 ` viresh kumar
2012-09-26 12:40 ` [PATCHv2 0/4] dw_dmac: split the driver and introduce PCI part Andy Shevchenko
2012-09-26 12:40 ` [PATCHv2 1/4] dmaengine: dw_dmac: convert to platform driver Andy Shevchenko
2012-09-26 14:13 ` viresh kumar
2012-09-26 18:00 ` Andy Shevchenko
2012-09-27 3:47 ` viresh kumar
2012-09-26 12:40 ` [PATCHv2 2/4] dmaengine: dw_dmac: Add PCI part of the driver Andy Shevchenko
2012-09-26 14:33 ` viresh kumar
2012-09-26 17:55 ` Andy Shevchenko
2012-09-26 19:41 ` Arnd Bergmann
2012-09-27 3:53 ` viresh kumar
2012-09-27 7:41 ` Arnd Bergmann
2012-09-27 14:22 ` Vinod Koul
2012-09-27 14:42 ` Arnd Bergmann
2012-09-26 12:40 ` [PATCHv2 3/4] dma: move dw_dmac driver to an own directory Andy Shevchenko
2012-09-26 14:53 ` viresh kumar
2012-09-26 17:50 ` Andy Shevchenko
2012-09-26 12:40 ` [PATCHv2 4/4] MAINTAINERS: add recently created files to dw_dmac section Andy Shevchenko
2012-09-26 14:45 ` viresh kumar
2012-09-26 14:48 ` viresh kumar
2012-09-26 17:49 ` Andy Shevchenko
2012-09-27 6:38 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1348642068.13371.135.camel@smile \
--to=andriy.shevchenko@linux.intel.com \
--cc=heikki.krogerus@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=vinod.koul@intel.com \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).