From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Fri, 4 May 2018 17:57:15 -0400 Subject: [U-Boot] [PATCH v2 01/10] ram: Add driver for MPC83xx In-Reply-To: References: <20180427125227.1026-1-mario.six@gdsys.cc> Message-ID: <20180504215715.GT12235@bill-the-cat.ec.rr.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 wrote: > > Hi Simon, > > > > On Thu, May 3, 2018 at 9:01 PM, Simon Glass wrote: > >> Hi Mario, > >> > >> On 27 April 2018 at 06:52, Mario Six wrote: > >>> Add a RAM driver for the MPC83xx architecture. > >>> > >>> Signed-off-by: Mario Six > >>> > >>> --- > >>> > >>> 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 > >> > >> 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: