lkml.org 
[lkml]   [2020]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
Date
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.

Thanks,
Anson
\
 
 \ /
  Last update: 2020-07-17 11:53    [W:0.092 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site