On 08/12/2010 01:14 AM, Nori, Sekhar wrote:
>
> Hi Nishanth,
>
> On Thu, Aug 12, 2010 at 11:13:10, Nishanth Menon wrote:
>> 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;
>
> [...]
>
>>>
>>>>> + 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..
>
> I still don't see how this handles the case when maxspeed env variable
> is not set. The above code will just default to rev = 0 in that case.
it does the same here as well (default rev = 0). and the fact that
compared to v1, |= was avoided (this is part of v2 of the patch).
>
> We haven't gotten rid of any constructs either so not sure what the
> simplification here is.
got rid of a var maxspeed - but I doubt it will have much benefit,
please consider my objection withdrawn.. just to point to the note that
the var has not much of a lifetime or a function beyond providing data
to the switch..
Regards,
Nishanth Mnon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
