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

["Arnd Bergmann" <arnd@xxxxxxxx>]

On Wed, Nov 27, 2024, at 11:13, Rasmus Villemoes wrote:
> 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:
>>
>> 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.

There are multiple problems here that I conflated, let
me try to explain better:

- The patch here replaces a hard 'select' with a softer 'default'
  for the i.MX drivers. Both variants are used in multiple
  pinctrl drivers, and there are sensible arguments to go
  one way or another. However, mixing 'select' and 'default'
  on a given platform would be wrong, and my point here is
  that mixing 'imply' and 'default' on a single platform is
  just as wrong, specifically because of the COMEFROM issue:
  even if it all works as intended, a reader will have a hard
  time figuring out why exactly it works like this, and this
  likely leads to bugs in the future.

- In the more common cases I've seen, the use of 'imply' is
  itself a bug, usually developers pick it because there is a
  hard dependency between two drivers, but using 'select'
  causes build issues, either from broken Kconfig constraints
  or from link failures. I tend to find out about them when
  a randconfig build still runs into the link failure and
  the 'imply' just made it less likely to happen.

>> 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?

Either one, the two statements have the same effect in this case
since both ARCH_IMX and PINCTRL are 'bool' symbols.

Overall, my best advice here is still to not change the way
i.MX pinctrl works at all, but just fix Layerscape to not depend
on i.MX. The reason for the 'select' here is clearly that the
i.MX machines would fail to boot without pinctrl, and changing
that because of Layerscape seems backwards.

On the other hand, removing all 'select PINCTRL' and instead
using 'default' consistently may be a good idea for the
long run, especially if we want to do the same for clk, irqchip,
timer, regulator etc. At the moment, we have an arbitrary
cutoff where some subsystems are always considered essential,
while others are always considered optional and some are in
the middle even if turning them off still makes the system
unusable.

       Arnd