From: Sylvain Munaut <tnt@246tNt•com>
To: Arnd Bergmann <arnd@arndb•de>
Cc: linuxppc-dev@ozlabs•org
Subject: Re: [PATCH 4/9] powerpc: BestComm core support for Freescale MPC5200
Date: Sun, 13 May 2007 01:49:41 +0200 [thread overview]
Message-ID: <46465295.90309@246tNt.com> (raw)
In-Reply-To: <200705130127.08761.arnd@arndb.de>
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@246tNt•com>");
>> +MODULE_AUTHOR("Andrey Volkov <avolkov@varma-el•com>");
>> +MODULE_AUTHOR("Dale Farnsworth <dfarnsworth@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@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@246tNt•com>
author: Andrey Volkov <avolkov@varma-el•com>
author: Dale Farnsworth <dfarnsworth@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
next prev parent reply other threads:[~2007-05-12 23:52 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-05-12 20:31 [PATCH 0/9] BestComm : better late than never heh ;) Sylvain Munaut
2007-05-12 20:31 ` [PATCH 1/9] powerpc: exports rheap symbol to modules Sylvain Munaut
2007-05-12 20:31 ` [PATCH 2/9] powerpc: Changes the config mechanism for rheap Sylvain Munaut
2007-05-12 20:31 ` [PATCH 3/9] powerpc/ppc32: Update mpc52xx_psc structure with B revision changes Sylvain Munaut
2007-05-12 20:31 ` [PATCH 4/9] powerpc: BestComm core support for Freescale MPC5200 Sylvain Munaut
2007-05-12 20:31 ` [PATCH 5/9] powerpc: BestcComm ATA task support Sylvain Munaut
2007-05-12 20:31 ` [PATCH 6/9] powerpc: BestcComm FEC " Sylvain Munaut
2007-05-12 20:31 ` [PATCH 7/9] powerpc: BestcComm GenBD " Sylvain Munaut
2007-05-12 20:31 ` [PATCH 8/9] drivers/net: Add support for Freescale MPC5200 SoC internal FEC Sylvain Munaut
2007-05-12 20:31 ` [PATCH 9/9] sound: Add support for Freescale MPC5200 AC97 interface Sylvain Munaut
2007-05-12 23:30 ` [PATCH 5/9] powerpc: BestcComm ATA task support Arnd Bergmann
2007-05-12 23:27 ` [PATCH 4/9] powerpc: BestComm core support for Freescale MPC5200 Arnd Bergmann
2007-05-12 23:49 ` Sylvain Munaut [this message]
2007-05-13 0:24 ` Arnd Bergmann
2007-05-13 7:17 ` Sylvain Munaut
2007-05-13 23:29 ` Matt Sealey
2007-05-14 5:15 ` Sylvain Munaut
2007-05-13 3:36 ` Dale Farnsworth
2007-05-15 21:37 ` Kumar Gala
2007-05-15 22:27 ` Sylvain Munaut
2007-05-13 23:46 ` [PATCH 3/9] powerpc/ppc32: Update mpc52xx_psc structure with B revision changes Matt Sealey
2007-05-14 5:27 ` Sylvain Munaut
2007-05-15 10:59 ` Raquel Velasco and Bill Buck
2007-05-15 21:20 ` [PATCH 2/9] powerpc: Changes the config mechanism for rheap Kumar Gala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=46465295.90309@246tNt.com \
--to=tnt@246tnt$(echo .)com \
--cc=arnd@arndb$(echo .)de \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox