Re: [PATCH 4/9] powerpc: BestComm core support for Freescale MPC5200

On Saturday 12 May 2007, Sylvain Munaut wrote:

This patch adds support for the core of the BestComm API
for the Freescale MPC5200(b). The BestComm engine is a
microcode-controlled / tasks-based DMA used by several
of the onchip devices.

Have you tried if the DMA engine API that is now in drivers/dma
fits the hardware interfaces? Could they perhaps be extended to
do so?

+struct bcom_engine *bcom = NULL;

Doe this need to be a global? In most cases, you get a cleaner driver by
passing around pointers through function calls and your own device specific
data structures.

If you can't make that work, at least give it a longer name to make sure
it doesn't conflict with other global variables.

+/* ======================================================================== */
+/* Public and private API                                                   */
+/* ======================================================================== */
+
+/* Debug Dump */
+
+#define BCOM_DPRINTK(a,b...) printk(KERN_DEBUG DRIVER_NAME ": " a, ## b)

just use pr_debug in new code instead of defining your own

+static int __init
+mpc52xx_bcom_init(void)
+{
+	struct device_node *ofn_bcom, *ofn_sram;
+	struct resource res_bcom;
+
+	int rv;
+
+	/* Find the bestcomm node. If none, fails 'silently' since
+	 * we may just be on another platform */
+	ofn_bcom = of_find_compatible_node(
+			NULL, "dma-controller", "mpc5200-bestcomm");
+	if (!ofn_bcom)
+		return -ENODEV;

I know, my usual rant is getting old, but why is this one not an
of_platform_driver? It's not shared with arch/ppc or with arch/mips,
and it's not needed before module_init() time.

+
+#ifdef MODULE
+module_init(mpc52xx_bcom_init);
+module_exit(mpc52xx_bcom_exit);
+#endif
+
+/* If we're not a module, we must make sure everything is setup before anyone */
+/* tries to use us ... */
+#ifndef MODULE
+subsys_initcall(mpc52xx_bcom_init);
+#endif

You can make it subsys_initcall() unconditionally. If the driver gets built
as a module, it will turn into module_init() by itself.

+MODULE_DESCRIPTION("Freescale MPC52xx BestComm DMA");
+MODULE_AUTHOR("Sylvain Munaut [off-list ref]");
+MODULE_AUTHOR("Andrey Volkov [off-list ref]");
+MODULE_AUTHOR("Dale Farnsworth [off-list ref]");
+MODULE_LICENSE("GPL v2");

I didn't know that you could have multiple MODULE_AUTHOR statements.
Does this do the right thing with /bin/modinfo?

+EXPORT_SYMBOL(bcom);
+EXPORT_SYMBOL(bcom_dump_status);
+EXPORT_SYMBOL(bcom_dump_task);
+EXPORT_SYMBOL(bcom_dump_bdring);
+EXPORT_SYMBOL(bcom_task_alloc);
+EXPORT_SYMBOL(bcom_task_release);
+EXPORT_SYMBOL(bcom_load_image);
+EXPORT_SYMBOL(bcom_set_initiator);
+EXPORT_SYMBOL(bcom_enable);
+EXPORT_SYMBOL(bcom_disable);

Please put every EXPORT_SYMBOL right after the definition of the symbol
you are exporting.

Why don't you use EXPORT_SYMBOL_GPL?

+
+EXPORT_SYMBOL(bcom_sram);
+
+EXPORT_SYMBOL(bcom_sram_init);
+EXPORT_SYMBOL(bcom_sram_cleanup);
+EXPORT_SYMBOL(bcom_sram_alloc);
+EXPORT_SYMBOL(bcom_sram_free);
+

Same comment as above.

	Arnd <><