From: Aisheng Dong <aisheng.dong@nxp.com>
To: Anson Huang <anson.huang@nxp.com>, Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Baluta <daniel.baluta@nxp.com>,
"festevam@gmail.com" <festevam@gmail.com>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"stefan@agner.ch" <stefan@agner.ch>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
Date: Mon, 7 Sep 2020 08:17:55 +0000 [thread overview]
Message-ID: <AM6PR04MB4966AE313DE13AD65B2D47B280280@AM6PR04MB4966.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <DB3PR0402MB391681666012C43C471FA0E2F57C0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
> From: Anson Huang <anson.huang@nxp.com>
> Sent: Friday, July 17, 2020 5:53 PM
>
> Hi, Arnd
>
> > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl
> > driver as module
> >
> > On Fri, Jul 17, 2020 at 11:24 AM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU
> > > > pinctrl driver as module
> > > > +MODULE_AUTHOR("Dong Aisheng<aisheng.dong@nxp.com>");
> > > > +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > >
> > > >
> > > > This can be in a separate patch.
> > >
> > > I don't understand, without adding module license, changing the SCU
> > > pinctrl driver to "tristate", when building with =M, the build will
> > > have warning as below, so I think it does NOT make sense to split it
> > > to 2
> > patches.
> > >
> > > CC [M] drivers/pinctrl/freescale/pinctrl-scu.o
> > > MODPOST Module.symvers
> > > WARNING: modpost: missing MODULE_LICENSE() in
> > drivers/pinctrl/freescale/pinctrl-scu.o
> > > LD [M] drivers/pinctrl/freescale/pinctrl-scu.ko
> >
> > I agree it would be clearer to do it as separate patches, but you then
> > have to be careful about the order to avoid the problem you mention.
> >
> > A clear indication that it may be sensible to split up the patch is
> > that your changelog has a list of five items in it, which are mostly doing
> different things.
> > The ideal way to split up a patch series is to have each patch with a
> > changelog that has to explain exactly one thing, and makes it obvious
> > how each changed line corresponds to the description, but never
> > explain the same thing in more than one patch (i.e. you combine
> > patches that do the same thing in multiple files).
> >
> > In this case, a good split may be:
> >
> > patch 1:
> > - Use function callbacks for SCU related functions in pinctrl-imx.c
> > in order to support the scenario of PINCTRL_IMX is built in
> > while PINCTRL_IMX_SCU is built as module;
> > - All drivers using SCU pinctrl driver need to initialize the
> > SCU related function callback;
> >
> > patch 2:
> > - Export SCU related functions and use "IS_ENABLED" instead of
> > "ifdef" to support SCU pinctrl driver user and itself to be
> > built as module;
> > - Change PINCTR_IMX_SCU to tristate;
> > - Add module author, description and license.
> >
> > and then rewrite the description to not have a bulleted list.
> >
> > That said, I don't think it is critical here, and I would not have
> > complained about the version you sent.
> >
> > If you end up changing the patch, I think you can actually drop the
> > "#if IS_ENABLED()" check entirely, as the functions are now always
> > assumed to be available, and we don't #ifdef declarations when there
> > is no #else path otherwise.
> >
>
> Thanks for the good suggestion, if there is other comment need a V2, or
> maintainer thinks it is better to split it following your guide, I will send V2
> following your guide.
>
Pls do as Arnd suggested.
Besides that, I have a few minor comments in separate replies.
Regards
Aisheng
> Thanks,
> Anson
next prev parent reply other threads:[~2020-09-07 8:18 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-16 15:06 [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module Anson Huang
2020-07-16 15:06 ` [PATCH 2/2] pinctrl: imx: Support building i.MX " Anson Huang
2020-09-07 8:33 ` Aisheng Dong
2020-09-07 8:35 ` Anson Huang
2020-07-16 15:14 ` [PATCH 1/2] pinctrl: imx: Support building SCU " Daniel Baluta
2020-07-16 15:21 ` Anson Huang
2020-07-16 15:58 ` Daniel Baluta
2020-07-16 23:44 ` Anson Huang
2020-07-17 8:39 ` Daniel Baluta
2020-07-17 9:22 ` Anson Huang
2020-07-17 9:45 ` Arnd Bergmann
2020-07-17 9:53 ` Anson Huang
2020-09-07 8:17 ` Aisheng Dong [this message]
2020-09-01 2:15 ` Anson Huang
2020-09-07 8:27 ` Aisheng Dong
2020-09-07 8:34 ` Anson Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM6PR04MB4966AE313DE13AD65B2D47B280280@AM6PR04MB4966.eurprd04.prod.outlook.com \
--to=aisheng.dong@nxp.com \
--cc=anson.huang@nxp.com \
--cc=arnd@arndb.de \
--cc=daniel.baluta@nxp.com \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=stefan@agner.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).