[U-Boot] [PATCH v2 01/10] ram: Add driver for MPC83xx

[Simon Glass]

+Tom for question below

Hi Mario,

On 4 May 2018 at 02:04, Mario Six <mario.six at gdsys.cc> wrote:
> Hi Simon,
>
> On Thu, May 3, 2018 at 9:01 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Mario,
>>
>> On 27 April 2018 at 06:52, Mario Six <mario.six at gdsys.cc> wrote:
>>> Add a RAM driver for the MPC83xx architecture.
>>>
>>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>> No changes
>>>
>>> ---
>>>  arch/powerpc/cpu/mpc83xx/spd_sdram.c       |   4 +
>>>  drivers/ram/Kconfig                        |   8 +
>>>  drivers/ram/Makefile                       |   1 +
>>>  drivers/ram/mpc83xx_sdram.c                | 948
+++++++++++++++++++++++++++++
>>>  include/dt-bindings/memory/mpc83xx-sdram.h | 143 +++++
>>>  include/mpc83xx.h                          |   6 +
>>>  6 files changed, 1110 insertions(+)
>>>  create mode 100644 drivers/ram/mpc83xx_sdram.c
>>>  create mode 100644 include/dt-bindings/memory/mpc83xx-sdram.h
>>>
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> Question below
>>
[..]

>>> +
>>> +phys_size_t get_effective_memsize(void)
>>> +{
>>> +#ifndef CONFIG_VERY_BIG_RAM
>>
>> Can this (and the #ifdefs below in this file) be converted to
>>
>> if (IS_ENABLED(CONFIG_...))
>>
>> instead, to increase build coverage?
>>
>
> Yes, no problem, will convert for v3.
>
> By the way, is this a practice that's generally encouraged, or is it just
> useful in special cases such as this one?

I think it is better in most cases as I don't really like #ifdefs in the
code when they are easy to remove:
- visually confusing particularly where there is more than one term in the
#if
- creates multiple builds of the code, reducing build coverage for sandbox
- can sometimes be replaced with empty static inline functions, or even
build up logic (e.g. of_live_active() and its callers)

Tom, do you have any thoughts on this one?

Regards,
Simon