Re: [PATCH v2 1/3] ARM: imx: Allow user to disable pinctrl

[Rasmus Villemoes <ravi@xxxxxxxxx>]

On Wed, Nov 27 2024, "Arnd Bergmann" <arnd@xxxxxxxx> wrote:

> On Wed, Nov 27, 2024, at 10:13, Rasmus Villemoes wrote:
>> On Tue, Nov 26 2024, Fabio Estevam <festevam@xxxxxxxxx> wrote:
>>>> Fabio submitted a patch enabling PINCTRL for imx_v4_v5_defconfig and
>>>> imx_v6_v7_defconfig explicitly [1]. I don't know if that fixes the
>>>> problem for good - I see CONFIG_ARCH_MXC in other configurations as
>>>> well.
>>>
>>> Good point. I can send a v2 adding CONFIG_PINCTRL=y to the other defconfigs.
>>>
>>
>> Instead of doing that, isn't this exactly what the 'imply' keyword is
>> for?
>>
>> - weak reverse dependencies: "imply" <symbol> ["if" <expr>]
>>
>>   This is similar to "select" as it enforces a lower limit on another
>>   symbol except that the "implied" symbol's value may still be set to n
>>   from a direct dependency or with a visible prompt.
>>
>>
>> So how about adding 'imply PINCTRL' in lieu of the previous 'select
>> PINCTRL'? And that would also better match the intention of the patch in
>> question (namely that the user needs to take explicit action to disable
>> PINCTRL).
>
> Please never use imply. Even if you think it's the right
> thing in a particular case, it will come back to bite you
> later.

Could you elaborate?

> See also https://en.wikipedia.org/wiki/COMEFROM ;-)

Yes yes, we've probably all seen that at some point and chuckled, but I
fail to see why imply would be worse than select.

> I would prefer we completely kill off that keyword from the Kconfig
> language and replace it with the reverse 'default'. In this
> particular case, having 'default ARCH_IMX' in 'PINCTRL'
> would of course not be a great idea,

Just to be clear, it would be 'default y if ARCH_MXC', not 'default
ARCH_IMX', right? Yes, given that the PINCTRL entry doesn't currently
have a list of "default y if ...", it would seem inappropriate.

> but for the exact same reason, the 'imply' is wrong here.

I don't follow, sorry.

Rasmus