[U-Boot] [PATCH 2/6] Add FSL "Can use" framework

[Wolfgang Denk]

Dear Anton Vorontsov,

In message <20090219154545.GB26618 at 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:
> 1. USB ULPI PHY vs. TSEC1 on MPC8315E-RDB boards;
> 2. 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
One difference between a man and a machine is that a machine is quiet
when well oiled.