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

[Suriyan Ramasami]

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!


> Thanks!
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>