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 <[email protected]> wrote: > > Hi Simon, > > > > On Thu, May 3, 2018 at 9:01 PM, Simon Glass <[email protected]> wrote: > >> Hi Mario, > >> > >> On 27 April 2018 at 06:52, Mario Six <[email protected]> wrote: > >>> Add a RAM driver for the MPC83xx architecture. > >>> > >>> Signed-off-by: Mario Six <[email protected]> > >>> > >>> --- > >>> > >>> 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 <[email protected]> > >> > >> 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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
