Ken.Fuchs@bench.com wrote:
U-Boot already has too many preprocessor constants and the addition of another (perhaps) dubious one merits more debate.
You omitted the context of this statement and hence most of its meaning.
Haavard Skinnemoen wrote:
I don't completely agree. U-Boot has too many #ifdefs, which isn't necessarily the same as too many #defines. And I don't think CONFIG_AT91 is dubious at all.
Perhaps the CONFIG_* symbols should be defined as TRUE or FALSE rather than defined or not defined.
I didn't say CONFIG_AT91 was dubious. I stated that defining CONFIG_<SOMETHING> where it isn't clear what <SOMETHING> should be is dubious. If one has to think too hard about what meaningful phrase <SOMETHING> should be, perhaps CONFIG_<SOMETHING> should not be defined at all. I'm complaining about defining new preprocessor definitions to be specific aggregations of other definitions just so a developer can type the conditions with fewer keystrokes without any improvement in code readability and maintainability. Sometimes an idiom should be left as an idiom rather than replaced by a preprocessor constant.
I have nothing against defining CONFIG_AT91 if it is useful, unique and makes code that uses it easier to understand and maintain.
But since we already have a CONFIG_AVR32 #define, we can clean up the mess in macb.c by simply reversing the logic.
If CONFIG_AVR32 can be used in macb.c without ofuscation, why is CONFIG_AT91 needed here? However, "simply reversing the logic" may be too much ofuscation though; we want clear rather than clever code after all.
An example of what I'd be opposed to is defining CONFIG_AT91SAM9260_OR_AT91SAM9263 where it is TRUE if either CONFIG_AT91SAM9260 or CONFIG_AT91SAM9263 are TRUE. Can you see where this might be used in the macb.c code?
Sincerely,
As I see it, CONFIG_AT91 would mean that you have a a certain class of peripherals which is developed for the AT91 range of processors (and is used by the AVR32 as well) The name is probably slightly misleading, but convenient.
Not having this definition, will soon mean that you have statements like the following:
#if defined(CONFIG_AT91RM9200) || \ defined(CONFIG_AT91SAM9260) || \ defined(CONFIG_AT91SAM9261) || \ defined(CONFIG_AT91SAM9263) || \ defined(CONFIG_AT91SAM9G10) || \ defined(CONFIG_AT91SAM9G15) || \ defined(CONFIG_AT91SAM9G20) || \ defined(CONFIG_AT91SAM9G41) || \ defined(CONFIG_AT91SAM9M10) || \ defined(CONFIG_AT91SAM9M11) || \ defined(CONFIG_AT91CAP9) || \ defined(CONFIG_AT572D940)
This statement will have to be updated every time a new MCU is released.
Having the statment #if defined(CONFIG_AT91)
should definitely reduce the maintenance of drivers.
Best Regards Ulf Samuelsson