Hi, Daniel > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as > module > > On 7/17/20 2:44 AM, Anson Huang wrote: > > Hi, Daniel > > > > > >> Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl > >> driver as module > >> > >> On 7/16/20 6:21 PM, Anson Huang wrote: > >>> Hi, Daniel > >>> > >>> > >>>> Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl > >>>> driver as module > >>>> > >>>> Hi Anson, > >>>> > >>>> Few comments inline: > >>>> > >>>> On 7/16/20 6:06 PM, Anson Huang wrote: > >>>>> To support building i.MX SCU pinctrl driver as module, below > >>>>> things need to > >>>> be changed: > >>>>> - Export SCU related functions and use "IS_ENABLED" instead of > >>>>> "ifdef" to support SCU pinctrl driver user and itself to be > >>>>> built as module; > >>>>> - 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; > >>>>> - Change PINCTR_IMX_SCU to tristate; > >>>>> - Add module author, description and license. > >>>>> > >>>>> With above changes, i.MX SCU pinctrl driver can be built as module. > >>>> There are a lot of changes here. I think it would be better to try > >>>> to split them > >>>> > >>>> per functionality. One functional change per patch. > >>> Actually, I ever tried to split them, but the function will be broken. > >>> All the changes are just to support the module build. If split them, > >>> the bisect will have pinctrl build or function broken. > >> Hi Anson, > >> > >> > >> I see your point and I know that this is a very hard task to get it > >> right from > >> > >> the first patches. > >> > >> But let me suggest at least that: > >> > >> - changes in drivers/pinctrl/freescale/pinctrl-imx.c (include file > >> and MODULE_ macros should go to a separate patch). > > You meant in patch #2, the changes in Kconfig and the changes in .c > > file should be split to 2 patches? > > > No. I mean look at path #1. > > The section above should be placed in a separate patch. > > static const struct of_device_id imx8qxp_pinctrl_of_match[] = { diff --git > a/drivers/pinctrl/freescale/pinctrl-scu.c b/drivers/pinctrl/freescale/pinctrl-scu.c > index 9df45d3..59b5f8a 100644 > --- a/drivers/pinctrl/freescale/pinctrl-scu.c > +++ b/drivers/pinctrl/freescale/pinctrl-scu.c > @@ -7,6 +7,7 @@ > > #include <linux/err.h> > #include <linux/firmware/imx/sci.h> > +#include <linux/module.h> > #include <linux/of_address.h> > #include <linux/pinctrl/pinctrl.h> > #include <linux/platform_device.h> > @@ -123,3 +124,7 @@ void imx_pinctrl_parse_pin_scu(struct imx_pinctrl > *ipctl, > pin_scu->mux_mode, pin_scu->config); > } > EXPORT_SYMBOL_GPL(imx_pinctrl_parse_pin_scu); > + > +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 Thanks, Anson