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.
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.
+extern int __fsl_can_use_dr_usb(void); +#define fsl_can_use_dr_usb __fsl_can_use_dr_usb
+static inline int __fsl_can_use_esdhc(void) { return !fsl_can_use_dr_usb(); } +#define fsl_can_use_esdhc __fsl_can_use_esdhc
Do we really need such cryptic code? Please clean up!
Best regards,
Wolfgang Denk