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

[Sylvain Munaut]

Hi Arnd,

Arnd Bergmann wrote:
> 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?
BestComm has some pretty specific needs and I think trying to
somehow extends a generic interface to fit those needs would just
pollute it.

However, you could totally implement a "DMA devices" that would just
use a simple "copy from there to there" task using this BestComm driver.
So other part of the kernel (like network) could use that interface to
use the dma engine ...

>> +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.
Yes, it kinda needs to be global. It's used in many many places.
I didn't like it much but I finally resigned because it ends up being a
lot easier, and more readable code to have it global.

I can rename it bcom_eng, quite unlikely to conflict.


>> +/* ======================================================================== */
>> +/* 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
>
Noted, will be fixed.

>> +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.
It needs to be initialized before _any_ other driver that uses bestcomm.
When compiled as module that could be an of_platform_driver but when
built-in there is apparently no way to ensure it's going to be probed
first. (At least no clean way ... )

>> +
>> +#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.
Oh, great, I didn't know that. I guess the module_exit still has to be
conditionnal.

>> +MODULE_DESCRIPTION("Freescale MPC52xx BestComm DMA");
>> +MODULE_AUTHOR("Sylvain Munaut <tnt at 246tNt.com>");
>> +MODULE_AUTHOR("Andrey Volkov <avolkov at varma-el.com>");
>> +MODULE_AUTHOR("Dale Farnsworth <dfarnsworth at mvista.com>");
>> +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?
I wasn't sure before I tried it myself. And it worked, so I used it ;)

tnt at efika:~$ /sbin/modinfo bestcomm-core
filename:      
/lib/modules/2.6.21-efika-g4f18984a-dirty/kernel/arch/powerpc/sysdev/bestcomm/bestcomm-core.ko
license:        GPL v2
author:         Sylvain Munaut <tnt at 246tNt.com>
author:         Andrey Volkov <avolkov at varma-el.com>
author:         Dale Farnsworth <dfarnsworth at mvista.com>
description:    Freescale MPC52xx BestComm DMA
depends:       
vermagic:       2.6.21-efika-g4f18984a-dirty mod_unload

>> +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.
Ok, I've seen both style I didn't know which to use.
> Why don't you use EXPORT_SYMBOL_GPL?
Why would I ? Is it mandatory now ?
I don't really have an objection to non-gpl modules to use the exported
functions ...
If Dale or Andrey have, they're free to post a patch to change to _GPL.



    Sylvain