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

[Andy Shevchenko]

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?

> >       depends OF || COMPILE_TEST
> >
> > ?

So?

...

> > > +   depends on ARCH_S32 && OF

Ditto.

> > > +/**
> > > + * 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.

> > > +};
> > > +
> > > +/**
> > > + * 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.

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