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

[Chester Lin]

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 <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:
> > 
> > ...
> > 
> > > 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