Re: [PATCH] ARM: imx: only enable pinctrl as needed

From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
To: Arnd Bergmann <arnd@arndb.de>, Esben Haabendal <esben@geanix.com>
Cc: Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Russell King <linux@armlinux.org.uk>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH] ARM: imx: only enable pinctrl as needed
Date: Tue, 1 Jun 2021 13:24:57 +0200	[thread overview]
Message-ID: <ca33f2de-2f49-cb5e-2d1b-96627c4db2c2@prevas.dk> (raw)
In-Reply-To: <CAK8P3a1HGJpYnmhGb_eZzrv0ZopOJ-JuC6hyNP8V87C1Po9ruw@mail.gmail.com>

On 28/05/2021 09.53, Arnd Bergmann wrote:
> On Tue, May 25, 2021 at 1:22 PM Esben Haabendal <esben@geanix.com> wrote:
>>
>> As not all mach-imx platforms has support for run-time changes of pin
>> configurations (such as LS1021A), a more selective approach to enabling
>> pinctrl infrastructure makes sense, so that an e.g. an LS1021A only kernel
>> could be built without pinctrl support.
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
> 
> I think it would be even better to leave all these drivers to be
> user-configurable. The symbols are currently defined as e.g.
> 
> config PINCTRL_IMX51
>         bool "IMX51 pinctrl driver"
>         depends on SOC_IMX51
>         select PINCTRL_IMX
>         help
>           Say Y here to enable the imx51 pinctrl driver
> 
> which could be changed to
> 
> config PINCTRL_IMX51
>         bool "IMX51 pinctrl driver" if COMPILE_TEST && !SOC_IMX51
>         depends on OF
>         default SOC_IMX51
>         select PINCTRL_IMX
>         help
>           Say Y here to enable the imx51 pinctrl driver
> 
> Today, having it configurable is pointless because you can't turn it off
> when SOC_IMX51 is set, and you can't turn it on when SOC_IMX51
> is disabled.

But if you want to allow turning it off when SOC_IMX51 is set, don't you
want this to be

config PINCTRL_IMX51
        bool "IMX51 pinctrl driver"
        depends on OF
        depends on COMPILE_TEST || SOC_IMX51
        default SOC_IMX51
        select PINCTRL_IMX
        help
          Say Y here to enable the imx51 pinctrl driver

(otherwise, the !SOC_IMX51 condition on the prompt means it's not a
visible and thus changeable item).

But I think all the COMPILE_TEST would be better done later; it's not
immediately clear what "depends on" one would have to add in lieu of
SOC_${soc}.

> The second version allows turning off PINCTRL completely though, 

I'm not really sure what "second version" you're talking about here. If
you refer to Esben's original patch, that is indeed the whole goal -
getting rid of the pinctrl core code and anything else which is under
#ifdef CONFIG_PINCTRL which is useless on ls1021a (and its absence does
not make the board unbootable).

Rasmus