On Thu, Feb 19, 2009 at 08:56:50PM +0100, Wolfgang Denk wrote:
Dear Anton Vorontsov,
In message 20090219154545.GB26618@oksana.dev.rtsoft.ru you wrote:
So far it's used for specifying whether we want to use FSL DR USB or FSL eSDHC devices on MPC837X processors.
There are two more candidates for future use:
- USB ULPI PHY vs. TSEC1 on MPC8315E-RDB boards;
- Marvell vs. Vitesse PHYs on MPC8313E-RDB boards.
If you know that might need to be extended, than better plan for it right from the beginning.
OK. Done.
diff --git a/board/freescale/common/fsl_can_use.c b/board/freescale/common/fsl_can_use.c new file mode 100644 index 0000000..17cc20f --- /dev/null +++ b/board/freescale/common/fsl_can_use.c
That's a very strange name for a function, IMHO. I would expect that it has something to do with using a CAN controller...
+int __fsl_can_use_dr_usb(void)
If you plan to make this extendable, please use a more generic name. For example: fsl_hwconfig() [note that leading __ is reserved].
- const char *usb_or_esdhc = getenv("usb_or_esdhc");
Please make it extendable, use a more generic name for one (and only one) environment variable. It makes littel sense to pollyte the envrionment with N additional variables. For example, call it "hwconfig". Allow that this variable holds a list of config settings, separated for example by comma or colon or ...
- if (!usb_or_esdhc || !strcmp(usb_or_esdhc, "usb"))
return 1;- if (!strcmp(usb_or_esdhc, "esdhc"))
return 0;This doesn't scale as well. Use a table of known strings and (static inline) function pointers - this is much easier to extend when new options need to get added.
Once we've reached this point, we might even lean back and think which part of this is FSL specific. None, tight? So make this a generic feature and look around which other code can be replaced by it.
We can probably define both the content of option name / handler function pointer table and the respective handler functions in a board specific file - eventually even the board config file.
That would make for a flexible solution.
Here it is, patches on the way. It's very simple. Just an environment variable, but it should be enough to do what we need currently. I should write some documentation though.
Thanksб