by Chester Lin

[permalink] [raw]

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

On Thu, Mar 09, 2023 at 12:43:35AM +0800, Chester Lin wrote:
> Hi Andy,
>
> On Wed, Mar 08, 2023 at 03:21:00PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 8, 2023 at 7:03 AM Chester Lin <[email protected]> wrote:
> > > On Tue, Mar 07, 2023 at 01:28:09AM +0200, [email protected] wrote:
> > > > Mon, Feb 20, 2023 at 10:33:19AM +0800, Chester Lin kirjoitti:
> >
> > ...
> >
> > > but the driver patch
> > > has been merged into the maintainer's for-next so I would not change this part
> > > unless the driver patch needs to be reverted and re-submitted in the end.
> >
> > As I said you have to keep it in mind for all your future
> > contributions to the Linux kernel independently on the destiny of this
> > one.
> >
> > ...
> >
> > > > > + 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]
>
> > ...
> >
> > > > > + 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:

config PINCTRL_S32G2
depends on ARCH_S32 || (OF && COMPILE_TEST)
.....

Just in case if OF is not set but COMPILE_TEST is set.

> .....
>
> Regards,
> Chester
>
> > > > > +/**
> > > > > + * struct s32_pin_group - describes an S32 pin group
> > > > > + * @name: the name of this specific pin group
> > > > > + * @npins: the number of pins in this group array, i.e. the number of
> > > > > + * elements in pin_ids and pin_sss so we can iterate over that array
> > > > > + * @pin_ids: an array of pin IDs in this group
> > > > > + * @pin_sss: an array of source signal select configs paired with pin_ids
> > > > > + */
> > > > > +struct s32_pin_group {
> > > > > + const char *name;
> > > > > + unsigned int npins;
> > > > > + unsigned int *pin_ids;
> > > > > + unsigned int *pin_sss;
> > > >
> > > > Why didn't you embed struct pingroup?
> > >
> > > I did think about that but there's an additional 'pin_sss' which could make code
> > > a bit messy. For example:
> > >
> > > s32_regmap_update(pctldev, grp->grp.pins[i],
> > > S32_MSCR_SSS_MASK, grp->pin_sss[i]);
> >
> > We specifically provide those data types to separate generic things
> > with custom ones. I don't think about the code getting longer, the
> > access to the proper data seems reasonable to me. Look into other
> > drivers that utilise these data types.
> >
>
> Will change it, thanks for your suggestions.
>
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct s32_pmx_func - describes S32 pinmux functions
> > > > > + * @name: the name of this specific function
> > > > > + * @groups: corresponding pin groups
> > > > > + * @num_groups: the number of groups
> > > > > + */
> > > > > +struct s32_pmx_func {
> > > > > + const char *name;
> > > > > + const char **groups;
> > > > > + unsigned int num_groups;
> > > > > +};
> > > >
> > > > struct pinfunction.
> > >
> > > Thanks for your information. I was not aware of this new struct since it just got
> > > merged recently.
> >
> > That's why the rule is to keep an eye on the subsystem development by
> > regular rebasing on top of its tip (pinctrl tree, devel branch in this
> > case).
> >
> > ...
> >
> > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > +int __maybe_unused s32_pinctrl_resume(struct device *dev);
> > > > > +int __maybe_unused s32_pinctrl_suspend(struct device *dev);
> > > > > +#endif
> > > >
> > > > Please, consider using new PM macros, like pm_ptr().
> > >
> > > Maybe pm_sleep_ptr() is more accurate?
> >
> > You are the author, choose what you think fits the best!
> >
> > ...
> >
> >
> > > > > + case PIN_CONFIG_BIAS_PULL_UP:
> > > > > + if (arg)
> > > > > + *config |= S32_MSCR_PUS;
> > > > > + else
> > > > > + *config &= ~S32_MSCR_PUS;
> > > >
> > > > > + fallthrough;
> > > >
> > > > It's quite easy to miss this and tell us about how is it supposed to work with PU + PD?
> > > >
> > > I admit that it's ambiguous and should be improved in order to have better readability.
> > >
> > > In a S32G2 MSCR register, there are two register bits related to pull up/down functions:
> > >
> > > PUE (Pull Enable, MSCR[13]): 0'b: Disabled, 1'b: Enabled
> > > PUS (Pull Select, MSCR[12]): 0'b: Pull Down, 1'b: Pull Up
> > >
> > > The dt properties could be like these:
> > >
> > > 1) 'bias-pull-up' or 'bias-pull-up: true' --> arg = 1
> > > In this case both PUE and PUS are set.
> > >
> > > 2) 'bias-pull-up: false' --> arg = 0
> > > In this case both PUE and PUS are cleared since the pull-up function must be disabled.
> >
> > So, split it to a separate function where you do the enabling only once.
> > I can point to drivers/pinctrl/intel/pinctrl-intel.c for the idea to take from.
> >
>
> Will do.
>
> > > > > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > > + if (arg)
> > > > > + *config |= S32_MSCR_PUE;
> > > > > + else
> > > > > + *config &= ~S32_MSCR_PUE;
> > > > > + *mask |= S32_MSCR_PUE | S32_MSCR_PUS;
> > > > > + break;
> > > > > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> > > > > + *config &= ~(S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE);
> > > > > + *mask |= S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE;
> > > > > + fallthrough;
> > > >
> > > > Ditto.
> > > >
> > >
> > > It's similar to the case 'PIN_CONFIG_BIAS_PULL_UP' although the PUS bit is assumed
> > > as 0 via the config variable so only the PUE bit needs to be configured, for example:
> > >
> > > 1) 'bias-pull-down' or 'bias-pull-down: true' --> arg = 1
> > > PUE is set and PUS is cleared.
> > >
> > > 2) 'bias-pull-down: false' --> arg = 0
> > > In this case both PUE and PUS are cleared since the pull-down function must be disabled.
> > >
> > > > > + case PIN_CONFIG_BIAS_DISABLE:
> > > > > + *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
> > > > > + *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
> > > > > + break;
> >
> > Ditto.
> >
> > ...
> >
> > I assume that non-commented is equal to silent agreement and will be
> > addressed accordingly. If it's not the case, reply again with your
> > objections.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko

2023-03-09 12:51:40

by Andy Shevchenko

[permalink] [raw]

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

On Wed, Mar 8, 2023 at 7:05 PM Chester Lin <[email protected]> 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 <[email protected]> wrote:
> > > > On Tue, Mar 07, 2023 at 01:28:09AM +0200, [email protected] 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