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

[Tom Rini]

On Fri, May 04, 2018 at 09:37:54PM +0000, Simon Glass wrote:
> +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?

Can this even be built for sandbox?  If yes, we should do what we can to
increase build coverage (and coverity coverage).  Otherwise I don't
think that if (CONFIG_IS_ENABLED(FOO)) is always better.  If we're
talking about:
#ifdef FOO
    if (bar) {
        ...
    }
#endif

Then yes, testing for CONFIG_IS_ENABLED(FOO) && bar is great.  Or:
#ifdef FOO
    ... a few lines ...
#endif

It's also good.  But if we're talking about a lot of lines, I think the
added indent doesn't help and can start making it a bit harder with
additional wrap.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180504/245d0ac3/attachment.sig>