Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module

[Arnd Bergmann <arnd@xxxxxxxx>]

On Fri, Jul 17, 2020 at 11:24 AM Anson Huang <anson.huang@xxxxxxx> wrote:
> > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
> > +MODULE_AUTHOR("Dong Aisheng<aisheng.dong@xxxxxxx>");
> > +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.

       Arnd