On 08/11/2010 10:37 AM, Nori, Sekhar wrote: > Hi Nishanth, > > On Wed, Aug 11, 2010 at 09:33:29, Nishanth Menon wrote: >> On 08/10/2010 06:39 AM, Sekhar Nori wrote: > >>> diff --git a/board/davinci/da8xxevm/da850evm.c >>> b/board/davinci/da8xxevm/da850evm.c >>> index 959b2c6..6a6d4fb 100644 >>> --- a/board/davinci/da8xxevm/da850evm.c >>> +++ b/board/davinci/da8xxevm/da850evm.c >>> @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { >>> { DAVINCI_LPSC_GPIO }, >>> }; >>> >>> +#ifndef CONFIG_DA850_EVM_MAX_SPEED >>> +#define CONFIG_DA850_EVM_MAX_SPEED 300000 >>> +#endif >>> + >>> +/* >>> + * get_board_rev() - setup to pass kernel board revision information >>> + * Returns: >>> + * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x >>> part >>> + * 0 - 300 MHz >>> + * 1 - 372 MHz >>> + * 2 - 408 MHz >>> + * 3 - 456 MHz >>> + */ >>> +u32 get_board_rev(void) >>> +{ >>> + char *s; >>> + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED; >>> + u32 rev = 0; >>> + >>> + s = getenv("maxspeed"); >>> + if (s) >>> + maxspeed = simple_strtoul(s, NULL, 10); >>> + >>> + switch (maxspeed) { >>> + case 456000: >>> + rev |= 3; >>> + break; >>> + case 408000: >>> + rev |= 2; >>> + break; >>> + case 372000: >>> + rev |= 1; >> wondering if the |= makes any sense... > > Yeah, I put it in there in case additional fields pop-up > in board revision. It doesn't make a lot of sense now so > I will remove it. thx.
> >>> + break; >>> + } >>> + >>> + return rev; >> >> IMHO, the logic could be simplified? >> >> option 1: >> u8 rev=0; >> s = simple_strtoul(s, NULL, 10); aarrg.. emailing with eyes half shut mistake should have been: s = getenv("maxspeed"); >> if (s) { >> switch (simple_strtoul(s, NULL, 10)) { >> case 456000: >> rev = 3; >> break; >> case 408000: >> rev = 2; >> break; >> case 372000: >> rev = 1; >> break; >> } >> } > > Not sure how this simplifies the logic. I'd argue multiple strtoul > calls are actually better avoided. How does it handle the case where > max speed is to be set using board configuration? > my bad, the above should explain.. >> >> option 2: >> if you think that the speeds could get added in the future, that switch >> is going to look pretty ugly.. use a lookup instead.. > > Right now there is no known plan to add more speeds so I will > stick with the switch. You are right, option of using a lookup > deserves a look-in if there were a lot more speed steps. ok. Regards, Nishanth Menon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot