Re: [U-Boot-Users] [PATCH 1/1] Add support for ATMELAT91SAM9G20EK board
On Fri, 25 Jul 2008 17:44:52 -0500 [EMAIL PROTECTED] wrote: 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. Reversing the logic wouldn't obfuscate anything. The code in question is a simple matter of AT91 needs to do it this way, AVR32 needs to do it another way. Whether we check for AT91 or AVR32 is completely arbitrary. 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. I completely agreee with this. Can you see where this might be used in the macb.c code? Not really, no. The processors aren't _that_ different. It's a bit unfortunate that the USRIO register ended up with different behaviour on AT91 and AVR32, but fixing the silicon at this point would just make things worse, and IMO it's not a huge deal. From a CPP abuse point of view, the macb driver is IMO exceptionally clean compared to a lot of other code in u-boot. Haavard - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
Re: [U-Boot-Users] [PATCH 1/1] Add support for ATMELAT91SAM9G20EK board
On Thu, 24 Jul 2008 13:14:02 -0500 [EMAIL PROTECTED] wrote: U-Boot already has too many preprocessor constants and the addition of another (perhaps) dubious one merits more debate. 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. But since we already have a CONFIG_AVR32 #define, we can clean up the mess in macb.c by simply reversing the logic. Haavard - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
Re: [U-Boot-Users] [PATCH 1/1] Add support for ATMELAT91SAM9G20EK board
[EMAIL PROTECTED] 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, Ken Fuchs - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
Re: [U-Boot-Users] [PATCH 1/1] Add support for ATMELAT91SAM9G20EK board
Jean-Christophe PLAGNIOL-VILLARD wrote: /* choose RMII or MII mode. This depends on the board */ #ifdef CONFIG_RMII #if defined(CONFIG_AT91CAP9) || defined(CONFIG_AT91SAM9260) || \ -defined(CONFIG_AT91SAM9263) +defined(CONFIG_AT91SAM9263) || defined(CONFIG_AT91SAM9G20) macb_writel(macb, USRIO, MACB_BIT(RMII) | MACB_BIT(CLKEN)); #else macb_writel(macb, USRIO, 0); #endif Same comment as Haavard, Could please create a CONFIG_SOMETHING for each of this tree block of ifdef (not necessarily the same CONFIG) What do you suggest for SOMETHING in CONFIG_SOMETHING? Surely you aren't suggesting a literal CONFIG_SOMETHING. The fact that you didn't easily define a meaningful SOMETHING suggests that there isn't one that is sufficiently meaningful and that perhaps the #if condition as written above should remain as is. Code should be written for readability, not for terseness alone. Do we have to define yet another preprocessor constant for yet another purpose? U-Boot already has too many preprocessor constants and the addition of another (perhaps) dubious one merits more debate. Sincerely, Ken Fuchs - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
