[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Simon Glass]

Hi,

On 24 August 2015 at 16:14, Suriyan Ramasami <suriyan.r at gmail.com> wrote:
> Hello Heiko/Simon,
>
> On Sun, Aug 23, 2015 at 10:37 PM, Heiko Schocher <hs at denx.de> wrote:
>>
>> Hello Simon, Suriyan Ramasami,
>>
>> Am 22.08.2015 um 02:36 schrieb Simon Glass:
>>>
>>> Hi,
>>>
>>> On 18 August 2015 at 10:25, Suriyan Ramasami <suriyan.r at gmail.com> wrote:
>>>>
>>>> Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
>>>> I have seen this in the odroid U3 board, where on boot one sees this:
>>>> CPU:   Exynos4412 @  GHz
>>>> instead of:
>>>> CPU:   Exynos4412 @ 1 GHz
>>>>
>>>> I am assuming that this change was done to get rid of compiler
>>>> warnings related to unused variables when building with
>>>> CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.
>>>>
>>>> Signed-off-by: Suriyan Ramasami <suriyan.r at gmail.com>
>>>> ---
>>>>   lib/display_options.c | 6 ------
>>>>   1 file changed, 6 deletions(-)
>>>
>>>
>>> Acked-by: Simon Glass <sjg at chromium.org>
>>>
>>> That's strange. Your patch looks correct to me.
>>
>>
>> Yes, strange, how this slipped me ...
>>
>> This patch leads in the following compiler warning for the smartweb
>> board:
>>
>>   CC      spl/lib/display_options.o
>> /home/hs/zug/u-boot/lib/display_options.c: In function 'print_freq':
>> /home/hs/zug/u-boot/lib/display_options.c:29:16: warning: variable 'n' set
>> but not used [-Wunused-but-set-variable]
>>
>> @Suriyan Ramasami:
>> Could you add to your patch the following:
>>
>> diff --git a/lib/display_options.c b/lib/display_options.c
>> index 80316a4..a4a5032 100644
>> --- a/lib/display_options.c
>> +++ b/lib/display_options.c
>> @@ -23,6 +23,8 @@ int display_options (void)
>>         return 0;
>>  }
>>
>> +#if !defined(CONFIG_SPL_BUILD) || \
>> +    defined(CONFIG_SPL_SERIAL_SUPPORT)
>>  void print_freq(uint64_t freq, const char *s)
>>  {
>>         unsigned long m = 0;
>> @@ -185,3 +187,4 @@ int print_buffer(ulong addr, const void *data, uint
>> width, uint count,
>>
>>         return 0;
>>  }
>> +#endif
>>
> I am trying to understand this a bit. The compiler warning seems to stem
> from the fact that printf() is eradicated with CONFIG_SPL_BUILD being set,
> and hence results in the 'n being set but not used' situation.
> As its just one variable 'n' causing this, would it be more readable if we
> do not have the #ifdef you suggested, but rather just have this instead:
> 1. We get rid of:
> unsigned long n;
> 2. We substitute this:
>         n = freq;
>         printf("%lu", n);
> with:
>         printf("%lu", (unsigned long) freq);
>
> Comments and thoughts welcome! Thanks!
>

Yes you are right - I don't think we need that variable at all.

Regards,
Simon