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:

quoted

--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig

quoted

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

quoted

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

quoted

--- /dev/null
+++ b/drivers/dma/st_fdma.c

quoted

+#include <linux/module.h>

Needed?

quoted

+void *st_fdma_seg_to_mem(struct st_fdma_dev *fdev, u64 da, int len)

static?

Fixed in v2.

quoted

+{
+	[...]
+}

quoted

+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);
+		[...]
+}

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("STMicroelectronics FDMA engine driver");
+MODULE_AUTHOR("Ludovic.barre [off-list ref]");

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html