Re: [PATCH v8 05/10] pinctrl: eyeq5: add platform driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi folks,

lots of discussion here, lazy Reviewed-by from me, but Andy often (thank God!)
catches things I just miss.

On Thu, Feb 29, 2024 at 4:32 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:

> > > The rule of thumb is to make modules if, otherwise, it's not so critical for
> > > the boot process (and even for some cases we still may have it done as a module
> > > with help of deferred probe mechanism).
> >
> > I'd call SoC pin control a critical resource for the boot process.
> >
> > I also like the simplicity of builtin better for such a resource.
> >  - If we tristate pinctrl-eyeq5 and there is a bug, there is a bug (in a
> >    context that we have no reason to support).
> >  - If we do not allow it and there is a bug, there is no bug.
> >    Plus, it makes one less choice for people configuring the kernel.
>
> The problem is that you reduce the flexibility. Nobody prevents you from having
> it built-in while tristate. But completely different situation when it's bool.
>
> So my argument still stays. I think new code shouldn't be boolean by default.
> The only exceptional cases can do that (like PMIC driver or critical clock one).

I think bool is helpful for users if:

- The system cannot boot without the pin control driver

- The system cannot mount root from a storage medium without the pin control
  driver. Initramfs doesn't count for embedded systems, many of these use things
  like OpenWrt that does not use initramfs the way Debian or Fedora etc does.

This SoC is obviously for the deeply embedded usecase. If this SoC has root
on flash or eMMC and cannot access either without pin control, it is helpful
for users to have this as bool so they don't shoot themselves in the foot with
Kconfig.

> > > > > > +     if (WARN_ON(offset > 31))
> > > > > > +             return false;

I think I asked for this code in my review, because I felt unsafe about offset.

Maybe it's not such a big problem, but, this twoliner is also not a big deal.

Yours,
Linus Walleij