From: Peter Griffin <peter.griffin@linaro.org>
To: Paul Bolle <pebolle@tiscali.nl>
Cc: lee.jones@linaro.org, devicetree@vger.kernel.org,
dmaengine@vger.kernel.org, ludovic.barre@st.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, srinivas.kandagatla@gmail.com,
maxime.coquelin@st.com, patrice.chotard@st.com,
vinod.koul@intel.com, dan.j.williams@intel.com
Subject: Re: [PATCH 3/7] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support
Date: Fri, 28 Aug 2015 18:59:22 +0100 [thread overview]
Message-ID: <20150828175922.GA2853@griffinp-ThinkPad-X1-Carbon-2nd> (raw)
In-Reply-To: <1436429872.20619.77.camel@tiscali.nl>
Hi Paul,
Thanks for reviewing.
On Thu, 09 Jul 2015, Paul Bolle wrote:
> On wo, 2015-07-08 at 17:11 +0100, Peter Griffin wrote:
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
>
> > +config ST_FDMA
> > + bool "ST FDMA dmaengine support"
> > + depends on ARCH_STI
> > + select DMA_ENGINE
> > + select FW_LOADER
> > + select FW_LOADER_USER_HELPER_FALLBACK
> > + select LIBELF_32
> > + select DMA_VIRTUAL_CHANNELS
> > + help
> > + Enable support for ST FDMA controller.
> > + It supports 16 independent DMA channels, accepts up to 32 DMA requests
> > +
> > + Say Y here if you have such a chipset.
> > + If unsure, say N.
>
> > --- a/drivers/dma/Makefile
> > +++ b/drivers/dma/Makefile
> >
> > +obj-$(CONFIG_ST_FDMA) += st_fdma.o
>
> ST_FDMA is a bool symbol, so st_fdma.o can only be built-in.
Yes good spot, that is a mistake it should be tristate. Will fix in v2.
>
> > --- /dev/null
> > +++ b/drivers/dma/st_fdma.c
>
> > +#include <linux/module.h>
>
> Needed?
>
> > +void *st_fdma_seg_to_mem(struct st_fdma_dev *fdev, u64 da, int len)
>
> static?
Fixed in v2.
>
> > +{
> > + [...]
> > +}
>
> > +static int
> > +st_fdma_elf_load_segments(struct st_fdma_dev *fdev, const struct
> > firmware *fw)
> > +{
> > + [...]
> > + dst = st_fdma_seg_to_mem(fdev, da, memsz);
> > + [...]
> > +}
>
> > +static const struct of_device_id st_fdma_match[] = {
> > + { .compatible = "st,fdma_mpe31", .data = &fdma_mpe31 },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, st_fdma_match);
>
> MODULE_DEVICE_TABLE() is preprocessed away for built-in only code.
>
> > +static int __exit st_fdma_remove(struct platform_device *pdev)
> > +{
> > + struct st_fdma_dev *fdev = platform_get_drvdata(pdev);
> > +
> > + wait_for_completion(&fdev->fw_ack);
> > +
> > + st_fdma_clk_disable(fdev);
> > +
> > + return 0;
> > +}
>
> Since this driver is built-in only this means st_fdma_remove() can never
> be used, right?
Will be capable of being a module in v2.
>
> > +static struct platform_driver st_fdma_platform_driver = {
> > + .driver = {
> > + .name = "st-fdma",
> > + .of_match_table = st_fdma_match,
> > + .pm = ST_FDMA_PM,
> > + },
> > + .probe = st_fdma_probe,
> > + .remove = st_fdma_remove,
> > +};
> > +module_platform_driver(st_fdma_platform_driver);
>
> So can .remove be dropped?
>
> Since v4.2-rc1 there's builtin_platform_driver() for built-in only code.
Thanks for the pointer, I wasn't aware of that function.
>
> > +bool st_fdma_filter_fn(struct dma_chan *chan, void *param)
> > +{
> > + [...]
> > +}
> > +EXPORT_SYMBOL(st_fdma_filter_fn);
>
> This series adds no users of this export. I suppose they will be added
> in another series. Is that correct?
No the export is not required. Will fix in v2.
>
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("STMicroelectronics FDMA engine driver");
> > +MODULE_AUTHOR("Ludovic.barre <Ludovic.barre@st.com>");
>
> These macros will, basically, be preprocessed away for built-in only
> code.
The driver will be capable of being a module in v2.
regards,
Peter.
next prev parent reply other threads:[~2015-08-28 17:59 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-08 16:11 [PATCH 0/7] Add support for FDMA DMA controller found on STi chipsets Peter Griffin
2015-07-08 16:11 ` [PATCH 1/7] dmaengine: st_fdma: Add STMicroelectronics FDMA DT binding documentation Peter Griffin
2015-08-19 15:06 ` Vinod Koul
2015-08-25 17:08 ` Peter Griffin
2015-08-26 7:12 ` Lee Jones
2015-09-03 9:18 ` Peter Griffin
2015-07-08 16:11 ` [PATCH 2/7] dmaengine: st_fdma: Add STMicroelectronics FDMA xbar " Peter Griffin
2015-07-08 16:11 ` [PATCH 3/7] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support Peter Griffin
2015-07-09 8:17 ` Paul Bolle
2015-08-28 17:59 ` Peter Griffin [this message]
2015-08-31 7:49 ` Maxime Coquelin
2015-08-31 8:08 ` Paul Bolle
2015-08-31 8:44 ` Maxime Coquelin
2015-08-19 15:40 ` Vinod Koul
2015-09-02 17:42 ` Peter Griffin
2015-07-08 16:11 ` [PATCH 4/7] dmaengine: st_fdma: Add xbar support Peter Griffin
2015-07-09 8:27 ` Paul Bolle
2015-07-08 16:11 ` [PATCH 5/7] ARM: STi: DT: STiH407: Add FDMA driver and xbar driver dt nodes Peter Griffin
2015-08-26 7:46 ` Lee Jones
2015-09-03 14:12 ` Peter Griffin
2015-07-08 16:11 ` [PATCH 6/7] MAINTAINERS: Add FDMA driver files to STi section Peter Griffin
2015-08-26 7:33 ` Lee Jones
2015-09-02 15:14 ` Peter Griffin
2015-07-08 16:11 ` [PATCH 7/7] ARM: multi_v7_defconfig: Enable STi FDMA driver Peter Griffin
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=20150828175922.GA2853@griffinp-ThinkPad-X1-Carbon-2nd \
--to=peter.griffin@linaro.org \
--cc=dan.j.williams@intel.com \
--cc=devicetree@vger.kernel.org \
--cc=dmaengine@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ludovic.barre@st.com \
--cc=maxime.coquelin@st.com \
--cc=patrice.chotard@st.com \
--cc=pebolle@tiscali.nl \
--cc=srinivas.kandagatla@gmail.com \
--cc=vinod.koul@intel.com \
/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).