From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH 19/23] memory: omap-gpmc: Enclose macro statements in do-while Date: Thu, 23 Jul 2020 11:09:40 +0200 Message-ID: References: <20200723073744.13400-1-krzk@kernel.org> <20200723073744.13400-20-krzk@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20200723073744.13400-20-krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Krzysztof Kozlowski Cc: Olof Johansson , arm-soc , SoC Team , Markus Mayer , bcm-kernel-feedback-list , Florian Fainelli , Santosh Shilimkar , Matthias Brugger , Roger Quadros , Tony Lindgren , Vladimir Zapolskiy , Thierry Reding , Jonathan Hunter , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Linux ARM , "moderated list:ARM/Mediatek SoC..." , linux-omap "open list:TEGRA ARCHITECTURE SUPPORT"

List-Id: linux-tegra@vger.kernel.org On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski wrote: > > do-while is a preferred way for complex macros because of safety > reasons. This fixes checkpatch error: > > ERROR: Macros starting with if should be enclosed by a do - while > loop to avoid possible if/else logic defects > > Signed-off-by: Krzysztof Kozlowski This is an improvement, but the macro still has other issues that are just as bad as the one you address: - Using the # operator to avoid the "" in the invocation seems confusing - it implicitly uses the 'cs' and 't' variables of the calling function instead of passing them as arguments. - it calls 'return -1' in a function that otherwise uses errno-style return codes, so this gets interpreted as EPERM "Operation not permitted". I would probably just open-code the entire thing and remove the macro like: ret = 0; ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2, 0, 3, 0, t->cs_on, GPMC_CD_FCLK, "cs_on"); ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2, 8, 12, 0, t->cs_rd_off, GPMC_CD_FCLK, "cs_rd_off"); ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2, 16, 20, 0, t->cs_wr_off, GPMC_CD_FCLK, "cs_wr_off); ... if (ret) return -ENXIO; Of maybe leave the macro, but remove the if/return part and use the "ret |= GPMC_SET_ONE(...)" trick to avoid some of the problems. Arnd