Date:   Thu, 9 Mar 2023 00:42:48 +0800
From:   Chester Lin <clin@suse.com>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>,
        s32@nxp.com, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Larisa Grigore <larisa.grigore@nxp.com>,
        Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com>,
        Andrei Stefanescu <andrei.stefanescu@nxp.com>,
        Radu Pirea <radu-nicolae.pirea@nxp.com>,
        Matthias Brugger <mbrugger@suse.com>,
        Matthew Nunez <matthew.nunez@nxp.com>,
        Phu Luu An <phu.luuan@nxp.com>,
        Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
Subject: Re: [PATCH v5 2/3] pinctrl: add NXP S32 SoC family support
Message-ID: <ZAi7CPXX0z80mKfQ@linux-8mug>
References: <20230220023320.3499-1-clin@suse.com>
 <20230220023320.3499-3-clin@suse.com>
 <ZAZ3CeYiZxR5zlRu@surfacebook>
 <ZAgXCi/BzyEQul9B@linux-8mug>
 <CAHp75VfxffTvAPSB4D2Oc3-vbiYM4DVpZf5=jRYGsCdFgAyxJA@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75VfxffTvAPSB4D2Oc3-vbiYM4DVpZf5=jRYGsCdFgAyxJA@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?XP026NB7lDNjdAvIMePAyWR/0LLogptCpgWndifLEKbRHLSZkqu9jNbukXee?=
 =?us-ascii?Q?K0RYCiC9EOOtxCr0kwvmlI6xXj2Nhezd8Sk10uc5zsNBwV1/0PDywBV2PZHM?=
 =?us-ascii?Q?5OkDEoPU2Vvx3M11cGV9FUBdhxjJQMHDkZ10irvD/vdfwmFwk+GqBZhAyo7k?=
 =?us-ascii?Q?Jhzrv6tIV5M65N4fx3rRBn681SSXuxuXPQ9JyMtusX04dije4OZdpxZlepII?=
 =?us-ascii?Q?mOdPAnZsJK8BVOzcJ5+fEQEPu4WD/imivk3bFeU7iurudoXgrKFKgL3zhzYy?=
 =?us-ascii?Q?WKYw80bW10p7dyDbE5fMOIO7AQS4WYKUbcY8uzZm4J6609ZwwA6oa1F1sJ/C?=
 =?us-ascii?Q?sYgwctZx+f/W2IwPNTubSBiw+1wr3nMBUlHx21AzWPwK8ygFTOixw29B5qG7?=
 =?us-ascii?Q?CKBAxLi/LB3DBYtFCLCBr1dp1BNE/7G73zTg2Lyq195PXwmnlzfIHLFIu0jY?=
 =?us-ascii?Q?YwZu7+5LEEirn7sxJQS2k/zC5OMjDbuVWFCxuee21qU4APZGQe222qsUMFZ0?=
 =?us-ascii?Q?ZQW9gM/4ApcKgq2CsRoLTtJPJ/Dv1jviQT6EB4Ug8Q/2iXoWsTxIKzJlgwC5?=
 =?us-ascii?Q?6yu+5kiQzm6jhv+Be5Iid6KD8GOq+Nijc5eFFriYSt713vRQ67B/oHF/FDgu?=
 =?us-ascii?Q?nB/B5RK6xSuL5ThHSbx5578sPyyvbI0WLpId5nbccvdLTZ13OACe8JIALxCi?=
 =?us-ascii?Q?j0AyVOn5g6SYOX5YeVaYADdm53d3LD4AzfHhCBvOFvvf6oknLlRYanTSjGf/?=
 =?us-ascii?Q?6o3dRRoEgbhXhyKjfJ+CkjXtCgqPC8ozOzqX4630826pUkslCBIX52zKW8Zh?=
 =?us-ascii?Q?Hnp5dgX4sE4M7C3pQWs2YKbGu/7aKJ/2sNPZPEuedCqgLabq7OiXB/+87Zk1?=
 =?us-ascii?Q?koqbwuxf76pR70YqKvLdIqxo/Bx2CCoaVK1J0Hemb5Um8y0wGag8o+SbbQ6/?=
 =?us-ascii?Q?uNeLZRXJjINDRmf/YBqmdGomPcr8inP8Wd9BFVWP1K4omF8LFECzZEqiR8Fm?=
 =?us-ascii?Q?FP4duV6YvsdBpNTHxvV6iyrHazAxbVzJ36TTpz+YHfwfkLMpS45vV9/6aQje?=
 =?us-ascii?Q?2M+7Nhy6TyXsgLxBR/yzkLIho4rjWPl1J5T68R3xpROwvcPJsQlYVn0rNw3z?=
 =?us-ascii?Q?kGa98I3S01vWUnvvyYS+swkoXDFruTsvVfecvgrFr1WD4ECvi+DiQr/msFSe?=
 =?us-ascii?Q?R2ZSewXDfbR0WoD0iaK9SsukcX+1M0UBuiJedFHY6w08WqmbPIhQvyJtOzZc?=
 =?us-ascii?Q?mmXDpswhn7iBYKwAfuTVxDiL9LTxUnOw4D2tUNFGxcccCvggZMRFkX3XrbgO?=
 =?us-ascii?Q?kmtVlMOn32jvjmBkaKQaUz1Gq07YJpjiMpvTg1uEneF5w7w5Q/jzlJ0FIfDG?=
 =?us-ascii?Q?Njjps/v7a9kDGiVtqPxLJHJU0Pu4uz2xlDcBqcIt3gQN2mY2jpJT6xWE5B70?=
 =?us-ascii?Q?9/2DUA4WuaTXNQLbxxN7eDpy8eK6AuntDjMgQ0YyzObPGN6Ams6VPth4weSJ?=
 =?us-ascii?Q?6XiXkLxB4eY+zuzVMBL8UMA1N7og1E9A61CwY9qUyAiUrqj5TFLIpxuopxsN?=
 =?us-ascii?Q?CK+UJHhlYnNsbaGUkZU=3D?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f750851a-034a-4b9c-ab4c-08db1ff43ff7
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0402MB3439.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 16:43:31.4202
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RNPtSjKQoOvorkofM1R/tTdmEva9GpNhYABbFMXqyXa6TSQglsS+B1+V+n5hmWPB
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB7095
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@suse.com> wrote:
> > On Tue, Mar 07, 2023 at 01:28:09AM +0200, andy.shevchenko@gmail.com 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
	.....

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