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@xxxxxx>");
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
