Re: [PATCH v5 2/3] pinctrl: add NXP S32 SoC family support

[Andy Shevchenko]

On Wed, Mar 8, 2023 at 7:05 PM Chester Lin <clin@xxxxxxxx> wrote:
> On Thu, Mar 09, 2023 at 12:43:35AM +0800, Chester Lin wrote:
> > On Wed, Mar 08, 2023 at 03:21:00PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 8, 2023 at 7:03 AM Chester Lin <clin@xxxxxxxx> wrote:
> > > > On Tue, Mar 07, 2023 at 01:28:09AM +0200, andy.shevchenko@xxxxxxxxx wrote:
> > > > > Mon, Feb 20, 2023 at 10:33:19AM +0800, Chester Lin kirjoitti:

...

> > > > > > +   depends on ARCH_S32 && OF
> > > > >
> > > > > Is OF necessary? Can it be
> > > >
> > > > I think it's required since the driver file refers to of_* APIs.
> > >
> > > And? Is it functional or compilation dependency? If the latter is the
> > > case, what API exactly isn't providing a stub?
> >
> > I was wrong. Looks like the ARM64 arch Kconfig always select OF so it's not
> > really necessary to have OF here.
> >
> > > > >       depends OF || COMPILE_TEST
> > > > >
> > > > > ?
> > >
> > > So?
> >
> > Since the OF dependency is not really necessary here, to fulfill the compile test
> > purpose, the possible dependency might be (ARCH_S32 || COMPILE_TEST), but it
> > could meet a compiling failure on the reference of pinconf_generic_parse_dt_config()
> > for those architectures which do not select OF by default since there's no stub
> > for this function. [pinconf_generic_parse_dt_config() is called in pinctrl-s32cc.c]

So. your driver needs to select the proper module and since there is
no stub it probably needs to be added.

Linus, why do we have no stubs in the first place?

...

> > > > > > +   depends on ARCH_S32 && OF
> > >
> > > Ditto.
> >
> > Based on the previous assumption [OF is not needed and PINCTRL_S32CC doesn't
> > depend on COMPILE_TEST], selecting PINCTRL_S32G2 wouldn't work if it simply
> > depends on (ARCH_S32 || COMPILE_TEST), for example:
> >
> >   WARNING: unmet direct dependencies detected for PINCTRL_S32CC
> >     Depends on [n]: PINCTRL [=y] && ARCH_S32
> >       Selected by [y]:
> >         - PINCTRL_S32G2 [=y] && PINCTRL [=y] && (ARCH_S32 || COMPILE_TEST [=y])
> >
> > So the better solutions is to still have OF in PINCTRL_S32CC, such as:
> >
> > config PINCTRL_S32CC
> >       bool
> >       depends on ARCH_S32 || (OF && COMPILE_TEST)
> >       .....
> >
> > config PINCTRL_S32G2
> >       depends on ARCH_S32 || COMPILE_TEST
>
> Fix the dependency here, it should be:

Nope, this is definitely wrong.

> config PINCTRL_S32G2
>         depends on ARCH_S32 || (OF && COMPILE_TEST)
>         .....
>
> Just in case if OF is not set but COMPILE_TEST is set.

-- 
With Best Regards,
Andy Shevchenko