From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932658AbcIGIdP (ORCPT ); Wed, 7 Sep 2016 04:33:15 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:42984 "EHLO s-opensource.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751025AbcIGIdK (ORCPT ); Wed, 7 Sep 2016 04:33:10 -0400 Subject: Re: [PATCH] ARM: imx: use IS_ENABLED() instead of checking for built-in or module To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= , Javier Martinez Canillas References: <1472463131-29899-1-git-send-email-javier@dowhile0.org> <20160906064206.7mme3s2pu2s7y465@pengutronix.de> Cc: linux-kernel@vger.kernel.org, Russell King , Sascha Hauer , Fabio Estevam , Shawn Guo , linux-arm-kernel@lists.infradead.org From: Javier Martinez Canillas Message-ID: Date: Wed, 7 Sep 2016 10:33:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160906064206.7mme3s2pu2s7y465@pengutronix.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Uwe, Thanks for your feedback. On 09/06/2016 08:42 AM, Uwe Kleine-König wrote: > Hello Javier, > > On Mon, Aug 29, 2016 at 11:32:11AM +0200, Javier Martinez Canillas wrote: >> From: Javier Martinez Canillas >> >> The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either >> built-in or as a module, use that macro instead of open coding the same. >> >> Using the macro makes the code more readable by helping abstract away some >> of the Kconfig built-in and module enable details. >> >> Signed-off-by: Javier Martinez Canillas >> Signed-off-by: Javier Martinez Canillas > > The following patch should do the same (untested though) and the source > looks still better: > Yes, your patch looks good to me. But is doing two things at once (using the IS_ENABLED macro and getting rid of the stub functions / ifdefery) so I would split it in two different patches. In any case, $SUBJECT was picked by Shawn already so you could post the other change on top of it if you want. > diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c > index 31df4361996f..48f794dbf293 100644 > --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c > +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c > @@ -63,7 +63,6 @@ > */ > #define KZM_ARM11_16550 (MX31_CS4_BASE_ADDR + 0x1050) > > -#if defined(CONFIG_SERIAL_8250) || defined(CONFIG_SERIAL_8250_MODULE) > /* > * KZM-ARM11-01 has an external UART on FPGA > */ > @@ -106,37 +105,35 @@ static struct platform_device serial_device = { > > static int __init kzm_init_ext_uart(void) > { > - u8 tmp; > - > - /* > - * GPIO 1-1: external UART interrupt line > - */ > - mxc_iomux_mode(IOMUX_MODE(MX31_PIN_GPIO1_1, IOMUX_CONFIG_GPIO)); > - gpio_request(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1), "ext-uart-int"); > - gpio_direction_input(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > - > - /* > - * Unmask UART interrupt > - */ > - tmp = __raw_readb(KZM_ARM11_IO_ADDRESS(KZM_ARM11_CTL1)); > - tmp |= 0x2; > - __raw_writeb(tmp, KZM_ARM11_IO_ADDRESS(KZM_ARM11_CTL1)); > - > - serial_platform_data[0].irq = > - gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > - serial8250_resources[1].start = > - gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > - serial8250_resources[1].end = > - gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > - > - return platform_device_register(&serial_device); > + if (IS_ENABLED(CONFIG_SERIAL_8250)) { > + u8 tmp; > + > + /* > + * GPIO 1-1: external UART interrupt line > + */ > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_GPIO1_1, IOMUX_CONFIG_GPIO)); > + gpio_request(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1), "ext-uart-int"); > + gpio_direction_input(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > + > + /* > + * Unmask UART interrupt > + */ > + tmp = __raw_readb(KZM_ARM11_IO_ADDRESS(KZM_ARM11_CTL1)); > + tmp |= 0x2; > + __raw_writeb(tmp, KZM_ARM11_IO_ADDRESS(KZM_ARM11_CTL1)); > + > + serial_platform_data[0].irq = > + gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > + serial8250_resources[1].start = > + gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > + serial8250_resources[1].end = > + gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_1)); > + > + return platform_device_register(&serial_device); > + } else { > + return 0; > + } > } > -#else > -static inline int kzm_init_ext_uart(void) > -{ > - return 0; > -} > -#endif > > /* > * SMSC LAN9118 > @@ -178,44 +175,39 @@ static struct regulator_consumer_supply dummy_supplies[] = { > > static int __init kzm_init_smsc9118(void) > { > - /* > - * GPIO 1-2: SMSC9118 interrupt line > - */ > - mxc_iomux_mode(IOMUX_MODE(MX31_PIN_GPIO1_2, IOMUX_CONFIG_GPIO)); > - gpio_request(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2), "smsc9118-int"); > - gpio_direction_input(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2)); > - > - regulator_register_fixed(0, dummy_supplies, ARRAY_SIZE(dummy_supplies)); > - > - kzm_smsc9118_resources[1].start = > - gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2)); > - kzm_smsc9118_resources[1].end = > - gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2)); > - > - return platform_device_register(&kzm_smsc9118_device); > + if (IS_ENABLED(CONFIG_SMSC911X)) { > + /* > + * GPIO 1-2: SMSC9118 interrupt line > + */ > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_GPIO1_2, IOMUX_CONFIG_GPIO)); > + gpio_request(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2), "smsc9118-int"); > + gpio_direction_input(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2)); > + > + regulator_register_fixed(0, dummy_supplies, > + ARRAY_SIZE(dummy_supplies)); > + > + kzm_smsc9118_resources[1].start = > + gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2)); > + kzm_smsc9118_resources[1].end = > + gpio_to_irq(IOMUX_TO_GPIO(MX31_PIN_GPIO1_2)); > + > + return platform_device_register(&kzm_smsc9118_device); > + } else { > + return 0; > + } > } > -#else > -static inline int kzm_init_smsc9118(void) > -{ > - return 0; > -} > -#endif > > -#if defined(CONFIG_SERIAL_IMX) || defined(CONFIG_SERIAL_IMX_MODULE) > static const struct imxuart_platform_data uart_pdata __initconst = { > .flags = IMXUART_HAVE_RTSCTS, > }; > > static void __init kzm_init_imx_uart(void) > { > - imx31_add_imx_uart0(&uart_pdata); > - imx31_add_imx_uart1(&uart_pdata); > -} > -#else > -static inline void kzm_init_imx_uart(void) > -{ > + if (IS_ENABLED(CONFIG_SERIAL_IMX)) { > + imx31_add_imx_uart0(&uart_pdata); > + imx31_add_imx_uart1(&uart_pdata); > + } > } > -#endif > > static int kzm_pins[] __initdata = { > MX31_PIN_CTS1__CTS1, > > Having said that, just dropping the #ifdefs and registering the devices > even without the matching driver is fine for me, too. > > Best regards > Uwe > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America