Messages in this thread | | | Date | Fri, 28 Aug 2015 18:59:22 +0100 | From | Peter Griffin <> | Subject | Re: [PATCH 3/7] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support |
| |
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.
| |