| Message ID | 1594912013-20859-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] pinctrl: imx: Support building SCU pinctrl driver as module | expand |
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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/pinctrl/freescale/Kconfig | 2 +- > drivers/pinctrl/freescale/pinctrl-imx.c | 8 ++-- > drivers/pinctrl/freescale/pinctrl-imx.h | 57 ++++++++++++----------------- > drivers/pinctrl/freescale/pinctrl-imx8dxl.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-imx8qm.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-imx8qxp.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-scu.c | 5 +++ > 7 files changed, 42 insertions(+), 39 deletions(-) > > diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig > index 08fcf5c..570355c 100644 > --- a/drivers/pinctrl/freescale/Kconfig > +++ b/drivers/pinctrl/freescale/Kconfig > @@ -7,7 +7,7 @@ config PINCTRL_IMX > select REGMAP > > config PINCTRL_IMX_SCU > - bool > + tristate "IMX SCU pinctrl driver" > depends on IMX_SCU > select PINCTRL_IMX > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c > index 507e4af..b80c450 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -373,7 +373,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev, > const struct imx_pinctrl_soc_info *info = ipctl->info; > > if (info->flags & IMX_USE_SCU) > - return imx_pinconf_get_scu(pctldev, pin_id, config); > + return info->imx_pinconf_get(pctldev, pin_id, config); > else > return imx_pinconf_get_mmio(pctldev, pin_id, config); > } > @@ -423,7 +423,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, > const struct imx_pinctrl_soc_info *info = ipctl->info; > > if (info->flags & IMX_USE_SCU) > - return imx_pinconf_set_scu(pctldev, pin_id, > + return info->imx_pinconf_set(pctldev, pin_id, > configs, num_configs); > else > return imx_pinconf_set_mmio(pctldev, pin_id, > @@ -440,7 +440,7 @@ static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev, > int ret; > > if (info->flags & IMX_USE_SCU) { > - ret = imx_pinconf_get_scu(pctldev, pin_id, &config); > + ret = info->imx_pinconf_get(pctldev, pin_id, &config); > if (ret) { > dev_err(ipctl->dev, "failed to get %s pinconf\n", > pin_get_name(pctldev, pin_id)); > @@ -629,7 +629,7 @@ static int imx_pinctrl_parse_groups(struct device_node *np, > for (i = 0; i < grp->num_pins; i++) { > pin = &((struct imx_pin *)(grp->data))[i]; > if (info->flags & IMX_USE_SCU) > - imx_pinctrl_parse_pin_scu(ipctl, &grp->pins[i], > + info->imx_pinctrl_parse_pin(ipctl, &grp->pins[i], > pin, &list); > else > imx_pinctrl_parse_pin_mmio(ipctl, &grp->pins[i], > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h > index 333d32b..bdb86c2 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.h > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h > @@ -75,6 +75,21 @@ struct imx_cfg_params_decode { > bool invert; > }; > > +/** > + * @dev: a pointer back to containing device > + * @base: the offset to the controller in virtual memory > + */ > +struct imx_pinctrl { > + struct device *dev; > + struct pinctrl_dev *pctl; > + void __iomem *base; > + void __iomem *input_sel_base; > + const struct imx_pinctrl_soc_info *info; > + struct imx_pin_reg *pin_regs; > + unsigned int group_index; > + struct mutex mutex; > +}; > + > struct imx_pinctrl_soc_info { > const struct pinctrl_pin_desc *pins; > unsigned int npins; > @@ -98,21 +113,13 @@ struct imx_pinctrl_soc_info { > struct pinctrl_gpio_range *range, > unsigned offset, > bool input); > -}; > - > -/** > - * @dev: a pointer back to containing device > - * @base: the offset to the controller in virtual memory > - */ > -struct imx_pinctrl { > - struct device *dev; > - struct pinctrl_dev *pctl; > - void __iomem *base; > - void __iomem *input_sel_base; > - const struct imx_pinctrl_soc_info *info; > - struct imx_pin_reg *pin_regs; > - unsigned int group_index; > - struct mutex mutex; > + int (*imx_pinconf_get)(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *config); > + int (*imx_pinconf_set)(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *configs, unsigned int num_configs); > + void (*imx_pinctrl_parse_pin)(struct imx_pinctrl *ipctl, > + unsigned int *pin_id, struct imx_pin *pin, > + const __be32 **list_p); > }; > > #define IMX_CFG_PARAMS_DECODE(p, m, o) \ > @@ -137,7 +144,7 @@ struct imx_pinctrl { > int imx_pinctrl_probe(struct platform_device *pdev, > const struct imx_pinctrl_soc_info *info); > > -#ifdef CONFIG_PINCTRL_IMX_SCU > +#if IS_ENABLED(CONFIG_PINCTRL_IMX_SCU) > #define BM_PAD_CTL_GP_ENABLE BIT(30) > #define BM_PAD_CTL_IFMUX_ENABLE BIT(31) > #define BP_PAD_CTL_IFMUX 27 > @@ -150,23 +157,5 @@ int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, unsigned pin_id, > void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, > unsigned int *pin_id, struct imx_pin *pin, > const __be32 **list_p); > -#else > -static inline int imx_pinconf_get_scu(struct pinctrl_dev *pctldev, > - unsigned pin_id, unsigned long *config) > -{ > - return -EINVAL; > -} > -static inline int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, > - unsigned pin_id, unsigned long *configs, > - unsigned num_configs) > -{ > - return -EINVAL; > -} > -static inline void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, > - unsigned int *pin_id, > - struct imx_pin *pin, > - const __be32 **list_p) > -{ > -} > #endif > #endif /* __DRIVERS_PINCTRL_IMX_H */ > diff --git a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > index 12b97da..d3020c0 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > @@ -159,6 +159,9 @@ static struct imx_pinctrl_soc_info imx8dxl_pinctrl_info = { > .pins = imx8dxl_pinctrl_pads, > .npins = ARRAY_SIZE(imx8dxl_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > static const struct of_device_id imx8dxl_pinctrl_of_match[] = { > diff --git a/drivers/pinctrl/freescale/pinctrl-imx8qm.c b/drivers/pinctrl/freescale/pinctrl-imx8qm.c > index 095acf4..8f46b940 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8qm.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8qm.c > @@ -292,6 +292,9 @@ static const struct imx_pinctrl_soc_info imx8qm_pinctrl_info = { > .pins = imx8qm_pinctrl_pads, > .npins = ARRAY_SIZE(imx8qm_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > static const struct of_device_id imx8qm_pinctrl_of_match[] = { > diff --git a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > index 81ebd4c..6776ad6 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > @@ -198,6 +198,9 @@ static struct imx_pinctrl_soc_info imx8qxp_pinctrl_info = { > .pins = imx8qxp_pinctrl_pads, > .npins = ARRAY_SIZE(imx8qxp_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > 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@nxp.com>"); > +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver"); > +MODULE_LICENSE("GPL v2");
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. Thanks, Anson
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). thanks Daniel.
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? Thanks, Anson
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@nxp.com>"); +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver"); +MODULE_LICENSE("GPL v2"); This can be in a separate patch.
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@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 Thanks, Anson
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. Arnd
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
Gentle ping... > Subject: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as > module > > 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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/pinctrl/freescale/Kconfig | 2 +- > drivers/pinctrl/freescale/pinctrl-imx.c | 8 ++-- > drivers/pinctrl/freescale/pinctrl-imx.h | 57 ++++++++++++----------------- > drivers/pinctrl/freescale/pinctrl-imx8dxl.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-imx8qm.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-imx8qxp.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-scu.c | 5 +++ > 7 files changed, 42 insertions(+), 39 deletions(-) > > diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig > index 08fcf5c..570355c 100644 > --- a/drivers/pinctrl/freescale/Kconfig > +++ b/drivers/pinctrl/freescale/Kconfig > @@ -7,7 +7,7 @@ config PINCTRL_IMX > select REGMAP > > config PINCTRL_IMX_SCU > - bool > + tristate "IMX SCU pinctrl driver" > depends on IMX_SCU > select PINCTRL_IMX > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c > b/drivers/pinctrl/freescale/pinctrl-imx.c > index 507e4af..b80c450 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -373,7 +373,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev, > const struct imx_pinctrl_soc_info *info = ipctl->info; > > if (info->flags & IMX_USE_SCU) > - return imx_pinconf_get_scu(pctldev, pin_id, config); > + return info->imx_pinconf_get(pctldev, pin_id, config); > else > return imx_pinconf_get_mmio(pctldev, pin_id, config); } @@ > -423,7 +423,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, > const struct imx_pinctrl_soc_info *info = ipctl->info; > > if (info->flags & IMX_USE_SCU) > - return imx_pinconf_set_scu(pctldev, pin_id, > + return info->imx_pinconf_set(pctldev, pin_id, > configs, num_configs); > else > return imx_pinconf_set_mmio(pctldev, pin_id, @@ -440,7 +440,7 > @@ static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev, > int ret; > > if (info->flags & IMX_USE_SCU) { > - ret = imx_pinconf_get_scu(pctldev, pin_id, &config); > + ret = info->imx_pinconf_get(pctldev, pin_id, &config); > if (ret) { > dev_err(ipctl->dev, "failed to get %s pinconf\n", > pin_get_name(pctldev, pin_id)); > @@ -629,7 +629,7 @@ static int imx_pinctrl_parse_groups(struct > device_node *np, > for (i = 0; i < grp->num_pins; i++) { > pin = &((struct imx_pin *)(grp->data))[i]; > if (info->flags & IMX_USE_SCU) > - imx_pinctrl_parse_pin_scu(ipctl, &grp->pins[i], > + info->imx_pinctrl_parse_pin(ipctl, &grp->pins[i], > pin, &list); > else > imx_pinctrl_parse_pin_mmio(ipctl, &grp->pins[i], diff --git > a/drivers/pinctrl/freescale/pinctrl-imx.h > b/drivers/pinctrl/freescale/pinctrl-imx.h > index 333d32b..bdb86c2 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.h > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h > @@ -75,6 +75,21 @@ struct imx_cfg_params_decode { > bool invert; > }; > > +/** > + * @dev: a pointer back to containing device > + * @base: the offset to the controller in virtual memory */ struct > +imx_pinctrl { > + struct device *dev; > + struct pinctrl_dev *pctl; > + void __iomem *base; > + void __iomem *input_sel_base; > + const struct imx_pinctrl_soc_info *info; > + struct imx_pin_reg *pin_regs; > + unsigned int group_index; > + struct mutex mutex; > +}; > + > struct imx_pinctrl_soc_info { > const struct pinctrl_pin_desc *pins; > unsigned int npins; > @@ -98,21 +113,13 @@ struct imx_pinctrl_soc_info { > struct pinctrl_gpio_range *range, > unsigned offset, > bool input); > -}; > - > -/** > - * @dev: a pointer back to containing device > - * @base: the offset to the controller in virtual memory > - */ > -struct imx_pinctrl { > - struct device *dev; > - struct pinctrl_dev *pctl; > - void __iomem *base; > - void __iomem *input_sel_base; > - const struct imx_pinctrl_soc_info *info; > - struct imx_pin_reg *pin_regs; > - unsigned int group_index; > - struct mutex mutex; > + int (*imx_pinconf_get)(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *config); > + int (*imx_pinconf_set)(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *configs, unsigned int num_configs); > + void (*imx_pinctrl_parse_pin)(struct imx_pinctrl *ipctl, > + unsigned int *pin_id, struct imx_pin *pin, > + const __be32 **list_p); > }; > > #define IMX_CFG_PARAMS_DECODE(p, m, o) \ @@ -137,7 +144,7 @@ struct > imx_pinctrl { int imx_pinctrl_probe(struct platform_device *pdev, > const struct imx_pinctrl_soc_info *info); > > -#ifdef CONFIG_PINCTRL_IMX_SCU > +#if IS_ENABLED(CONFIG_PINCTRL_IMX_SCU) > #define BM_PAD_CTL_GP_ENABLE BIT(30) > #define BM_PAD_CTL_IFMUX_ENABLE BIT(31) > #define BP_PAD_CTL_IFMUX 27 > @@ -150,23 +157,5 @@ int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, > unsigned pin_id, void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, > unsigned int *pin_id, struct imx_pin *pin, > const __be32 **list_p); > -#else > -static inline int imx_pinconf_get_scu(struct pinctrl_dev *pctldev, > - unsigned pin_id, unsigned long *config) > -{ > - return -EINVAL; > -} > -static inline int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, > - unsigned pin_id, unsigned long *configs, > - unsigned num_configs) > -{ > - return -EINVAL; > -} > -static inline void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, > - unsigned int *pin_id, > - struct imx_pin *pin, > - const __be32 **list_p) > -{ > -} > #endif > #endif /* __DRIVERS_PINCTRL_IMX_H */ > diff --git a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > index 12b97da..d3020c0 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > @@ -159,6 +159,9 @@ static struct imx_pinctrl_soc_info imx8dxl_pinctrl_info > = { > .pins = imx8dxl_pinctrl_pads, > .npins = ARRAY_SIZE(imx8dxl_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > static const struct of_device_id imx8dxl_pinctrl_of_match[] = { diff --git > a/drivers/pinctrl/freescale/pinctrl-imx8qm.c > b/drivers/pinctrl/freescale/pinctrl-imx8qm.c > index 095acf4..8f46b940 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8qm.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8qm.c > @@ -292,6 +292,9 @@ static const struct imx_pinctrl_soc_info > imx8qm_pinctrl_info = { > .pins = imx8qm_pinctrl_pads, > .npins = ARRAY_SIZE(imx8qm_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > static const struct of_device_id imx8qm_pinctrl_of_match[] = { diff --git > a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > index 81ebd4c..6776ad6 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > @@ -198,6 +198,9 @@ static struct imx_pinctrl_soc_info > imx8qxp_pinctrl_info = { > .pins = imx8qxp_pinctrl_pads, > .npins = ARRAY_SIZE(imx8qxp_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > 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@nxp.com>"); > +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4
> 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
> From: Anson Huang <Anson.Huang@nxp.com> > Sent: Thursday, July 16, 2020 11:07 PM > Subject: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module > > To support building i.MX SCU pinctrl driver as module, below things need to be > changed: > > - Export SCU related functions This line seems not comply with the patch anymore > 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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/pinctrl/freescale/Kconfig | 2 +- > drivers/pinctrl/freescale/pinctrl-imx.c | 8 ++-- > drivers/pinctrl/freescale/pinctrl-imx.h | 57 ++++++++++++----------------- > drivers/pinctrl/freescale/pinctrl-imx8dxl.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-imx8qm.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-imx8qxp.c | 3 ++ > drivers/pinctrl/freescale/pinctrl-scu.c | 5 +++ > 7 files changed, 42 insertions(+), 39 deletions(-) > > diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig > index 08fcf5c..570355c 100644 > --- a/drivers/pinctrl/freescale/Kconfig > +++ b/drivers/pinctrl/freescale/Kconfig > @@ -7,7 +7,7 @@ config PINCTRL_IMX > select REGMAP > > config PINCTRL_IMX_SCU > - bool > + tristate "IMX SCU pinctrl driver" IMX SCU pinctrl core driver > depends on IMX_SCU > select PINCTRL_IMX > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c > b/drivers/pinctrl/freescale/pinctrl-imx.c > index 507e4af..b80c450 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -373,7 +373,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev, > const struct imx_pinctrl_soc_info *info = ipctl->info; > > if (info->flags & IMX_USE_SCU) > - return imx_pinconf_get_scu(pctldev, pin_id, config); > + return info->imx_pinconf_get(pctldev, pin_id, config); > else > return imx_pinconf_get_mmio(pctldev, pin_id, config); } @@ -423,7 > +423,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, > const struct imx_pinctrl_soc_info *info = ipctl->info; > > if (info->flags & IMX_USE_SCU) > - return imx_pinconf_set_scu(pctldev, pin_id, > + return info->imx_pinconf_set(pctldev, pin_id, > configs, num_configs); > else > return imx_pinconf_set_mmio(pctldev, pin_id, @@ -440,7 +440,7 > @@ static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev, > int ret; > > if (info->flags & IMX_USE_SCU) { > - ret = imx_pinconf_get_scu(pctldev, pin_id, &config); > + ret = info->imx_pinconf_get(pctldev, pin_id, &config); > if (ret) { > dev_err(ipctl->dev, "failed to get %s pinconf\n", > pin_get_name(pctldev, pin_id)); > @@ -629,7 +629,7 @@ static int imx_pinctrl_parse_groups(struct device_node > *np, > for (i = 0; i < grp->num_pins; i++) { > pin = &((struct imx_pin *)(grp->data))[i]; > if (info->flags & IMX_USE_SCU) > - imx_pinctrl_parse_pin_scu(ipctl, &grp->pins[i], > + info->imx_pinctrl_parse_pin(ipctl, &grp->pins[i], > pin, &list); > else > imx_pinctrl_parse_pin_mmio(ipctl, &grp->pins[i], diff --git > a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h > index 333d32b..bdb86c2 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.h > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h > @@ -75,6 +75,21 @@ struct imx_cfg_params_decode { > bool invert; > }; > > +/** > + * @dev: a pointer back to containing device > + * @base: the offset to the controller in virtual memory */ struct > +imx_pinctrl { > + struct device *dev; > + struct pinctrl_dev *pctl; > + void __iomem *base; > + void __iomem *input_sel_base; > + const struct imx_pinctrl_soc_info *info; > + struct imx_pin_reg *pin_regs; > + unsigned int group_index; > + struct mutex mutex; > +}; Any reason to move this part of code? Regards Aisheng > + > struct imx_pinctrl_soc_info { > const struct pinctrl_pin_desc *pins; > unsigned int npins; > @@ -98,21 +113,13 @@ struct imx_pinctrl_soc_info { > struct pinctrl_gpio_range *range, > unsigned offset, > bool input); > -}; > - > -/** > - * @dev: a pointer back to containing device > - * @base: the offset to the controller in virtual memory > - */ > -struct imx_pinctrl { > - struct device *dev; > - struct pinctrl_dev *pctl; > - void __iomem *base; > - void __iomem *input_sel_base; > - const struct imx_pinctrl_soc_info *info; > - struct imx_pin_reg *pin_regs; > - unsigned int group_index; > - struct mutex mutex; > + int (*imx_pinconf_get)(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *config); > + int (*imx_pinconf_set)(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *configs, unsigned int num_configs); > + void (*imx_pinctrl_parse_pin)(struct imx_pinctrl *ipctl, > + unsigned int *pin_id, struct imx_pin *pin, > + const __be32 **list_p); > }; > > #define IMX_CFG_PARAMS_DECODE(p, m, o) \ @@ -137,7 +144,7 @@ struct > imx_pinctrl { int imx_pinctrl_probe(struct platform_device *pdev, > const struct imx_pinctrl_soc_info *info); > > -#ifdef CONFIG_PINCTRL_IMX_SCU > +#if IS_ENABLED(CONFIG_PINCTRL_IMX_SCU) > #define BM_PAD_CTL_GP_ENABLE BIT(30) > #define BM_PAD_CTL_IFMUX_ENABLE BIT(31) > #define BP_PAD_CTL_IFMUX 27 > @@ -150,23 +157,5 @@ int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, > unsigned pin_id, void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, > unsigned int *pin_id, struct imx_pin *pin, > const __be32 **list_p); > -#else > -static inline int imx_pinconf_get_scu(struct pinctrl_dev *pctldev, > - unsigned pin_id, unsigned long *config) > -{ > - return -EINVAL; > -} > -static inline int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, > - unsigned pin_id, unsigned long *configs, > - unsigned num_configs) > -{ > - return -EINVAL; > -} > -static inline void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, > - unsigned int *pin_id, > - struct imx_pin *pin, > - const __be32 **list_p) > -{ > -} > #endif > #endif /* __DRIVERS_PINCTRL_IMX_H */ > diff --git a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > index 12b97da..d3020c0 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c > @@ -159,6 +159,9 @@ static struct imx_pinctrl_soc_info imx8dxl_pinctrl_info > = { > .pins = imx8dxl_pinctrl_pads, > .npins = ARRAY_SIZE(imx8dxl_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > static const struct of_device_id imx8dxl_pinctrl_of_match[] = { diff --git > a/drivers/pinctrl/freescale/pinctrl-imx8qm.c > b/drivers/pinctrl/freescale/pinctrl-imx8qm.c > index 095acf4..8f46b940 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8qm.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8qm.c > @@ -292,6 +292,9 @@ static const struct imx_pinctrl_soc_info > imx8qm_pinctrl_info = { > .pins = imx8qm_pinctrl_pads, > .npins = ARRAY_SIZE(imx8qm_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > static const struct of_device_id imx8qm_pinctrl_of_match[] = { diff --git > a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > index 81ebd4c..6776ad6 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c > @@ -198,6 +198,9 @@ static struct imx_pinctrl_soc_info imx8qxp_pinctrl_info > = { > .pins = imx8qxp_pinctrl_pads, > .npins = ARRAY_SIZE(imx8qxp_pinctrl_pads), > .flags = IMX_USE_SCU, > + .imx_pinconf_get = imx_pinconf_get_scu, > + .imx_pinconf_set = imx_pinconf_set_scu, > + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, > }; > > 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@nxp.com>"); > +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4
> Subject: RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as > module > > > From: Anson Huang <Anson.Huang@nxp.com> > > Sent: Thursday, July 16, 2020 11:07 PM > > Subject: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver > > as module > > > > To support building i.MX SCU pinctrl driver as module, below things > > need to be > > changed: > > > > - Export SCU related functions > > This line seems not comply with the patch anymore > OK. > > 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. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > drivers/pinctrl/freescale/Kconfig | 2 +- > > drivers/pinctrl/freescale/pinctrl-imx.c | 8 ++-- > > drivers/pinctrl/freescale/pinctrl-imx.h | 57 ++++++++++++----------------- > > drivers/pinctrl/freescale/pinctrl-imx8dxl.c | 3 ++ > > drivers/pinctrl/freescale/pinctrl-imx8qm.c | 3 ++ > > drivers/pinctrl/freescale/pinctrl-imx8qxp.c | 3 ++ > > drivers/pinctrl/freescale/pinctrl-scu.c | 5 +++ > > 7 files changed, 42 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/pinctrl/freescale/Kconfig > > b/drivers/pinctrl/freescale/Kconfig > > index 08fcf5c..570355c 100644 > > --- a/drivers/pinctrl/freescale/Kconfig > > +++ b/drivers/pinctrl/freescale/Kconfig > > @@ -7,7 +7,7 @@ config PINCTRL_IMX > > select REGMAP > > > > config PINCTRL_IMX_SCU > > - bool > > + tristate "IMX SCU pinctrl driver" > > IMX SCU pinctrl core driver > Will change it in V2. > > depends on IMX_SCU > > select PINCTRL_IMX > > > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c > > b/drivers/pinctrl/freescale/pinctrl-imx.c > > index 507e4af..b80c450 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > > @@ -373,7 +373,7 @@ static int imx_pinconf_get(struct pinctrl_dev > *pctldev, > > const struct imx_pinctrl_soc_info *info = ipctl->info; > > > > if (info->flags & IMX_USE_SCU) > > - return imx_pinconf_get_scu(pctldev, pin_id, config); > > + return info->imx_pinconf_get(pctldev, pin_id, config); > > else > > return imx_pinconf_get_mmio(pctldev, pin_id, config); } @@ > -423,7 > > +423,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, > > const struct imx_pinctrl_soc_info *info = ipctl->info; > > > > if (info->flags & IMX_USE_SCU) > > - return imx_pinconf_set_scu(pctldev, pin_id, > > + return info->imx_pinconf_set(pctldev, pin_id, > > configs, num_configs); > > else > > return imx_pinconf_set_mmio(pctldev, pin_id, @@ -440,7 +440,7 > @@ > > static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev, > > int ret; > > > > if (info->flags & IMX_USE_SCU) { > > - ret = imx_pinconf_get_scu(pctldev, pin_id, &config); > > + ret = info->imx_pinconf_get(pctldev, pin_id, &config); > > if (ret) { > > dev_err(ipctl->dev, "failed to get %s pinconf\n", > > pin_get_name(pctldev, pin_id)); > > @@ -629,7 +629,7 @@ static int imx_pinctrl_parse_groups(struct > > device_node *np, > > for (i = 0; i < grp->num_pins; i++) { > > pin = &((struct imx_pin *)(grp->data))[i]; > > if (info->flags & IMX_USE_SCU) > > - imx_pinctrl_parse_pin_scu(ipctl, &grp->pins[i], > > + info->imx_pinctrl_parse_pin(ipctl, &grp->pins[i], > > pin, &list); > > else > > imx_pinctrl_parse_pin_mmio(ipctl, &grp->pins[i], diff --git > > a/drivers/pinctrl/freescale/pinctrl-imx.h > > b/drivers/pinctrl/freescale/pinctrl-imx.h > > index 333d32b..bdb86c2 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h > > @@ -75,6 +75,21 @@ struct imx_cfg_params_decode { > > bool invert; > > }; > > > > +/** > > + * @dev: a pointer back to containing device > > + * @base: the offset to the controller in virtual memory */ struct > > +imx_pinctrl { > > + struct device *dev; > > + struct pinctrl_dev *pctl; > > + void __iomem *base; > > + void __iomem *input_sel_base; > > + const struct imx_pinctrl_soc_info *info; > > + struct imx_pin_reg *pin_regs; > > + unsigned int group_index; > > + struct mutex mutex; > > +}; > > Any reason to move this part of code? It is because below function callback added in imx_pinctrl_soc_info structure need to use imx_pinctrl, otherwise, build will fail. + void (*imx_pinctrl_parse_pin)(struct imx_pinctrl *ipctl, Anson
diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig index 08fcf5c..570355c 100644 --- a/drivers/pinctrl/freescale/Kconfig +++ b/drivers/pinctrl/freescale/Kconfig @@ -7,7 +7,7 @@ config PINCTRL_IMX select REGMAP config PINCTRL_IMX_SCU - bool + tristate "IMX SCU pinctrl driver" depends on IMX_SCU select PINCTRL_IMX diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 507e4af..b80c450 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -373,7 +373,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev, const struct imx_pinctrl_soc_info *info = ipctl->info; if (info->flags & IMX_USE_SCU) - return imx_pinconf_get_scu(pctldev, pin_id, config); + return info->imx_pinconf_get(pctldev, pin_id, config); else return imx_pinconf_get_mmio(pctldev, pin_id, config); } @@ -423,7 +423,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, const struct imx_pinctrl_soc_info *info = ipctl->info; if (info->flags & IMX_USE_SCU) - return imx_pinconf_set_scu(pctldev, pin_id, + return info->imx_pinconf_set(pctldev, pin_id, configs, num_configs); else return imx_pinconf_set_mmio(pctldev, pin_id, @@ -440,7 +440,7 @@ static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev, int ret; if (info->flags & IMX_USE_SCU) { - ret = imx_pinconf_get_scu(pctldev, pin_id, &config); + ret = info->imx_pinconf_get(pctldev, pin_id, &config); if (ret) { dev_err(ipctl->dev, "failed to get %s pinconf\n", pin_get_name(pctldev, pin_id)); @@ -629,7 +629,7 @@ static int imx_pinctrl_parse_groups(struct device_node *np, for (i = 0; i < grp->num_pins; i++) { pin = &((struct imx_pin *)(grp->data))[i]; if (info->flags & IMX_USE_SCU) - imx_pinctrl_parse_pin_scu(ipctl, &grp->pins[i], + info->imx_pinctrl_parse_pin(ipctl, &grp->pins[i], pin, &list); else imx_pinctrl_parse_pin_mmio(ipctl, &grp->pins[i], diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h index 333d32b..bdb86c2 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.h +++ b/drivers/pinctrl/freescale/pinctrl-imx.h @@ -75,6 +75,21 @@ struct imx_cfg_params_decode { bool invert; }; +/** + * @dev: a pointer back to containing device + * @base: the offset to the controller in virtual memory + */ +struct imx_pinctrl { + struct device *dev; + struct pinctrl_dev *pctl; + void __iomem *base; + void __iomem *input_sel_base; + const struct imx_pinctrl_soc_info *info; + struct imx_pin_reg *pin_regs; + unsigned int group_index; + struct mutex mutex; +}; + struct imx_pinctrl_soc_info { const struct pinctrl_pin_desc *pins; unsigned int npins; @@ -98,21 +113,13 @@ struct imx_pinctrl_soc_info { struct pinctrl_gpio_range *range, unsigned offset, bool input); -}; - -/** - * @dev: a pointer back to containing device - * @base: the offset to the controller in virtual memory - */ -struct imx_pinctrl { - struct device *dev; - struct pinctrl_dev *pctl; - void __iomem *base; - void __iomem *input_sel_base; - const struct imx_pinctrl_soc_info *info; - struct imx_pin_reg *pin_regs; - unsigned int group_index; - struct mutex mutex; + int (*imx_pinconf_get)(struct pinctrl_dev *pctldev, unsigned int pin_id, + unsigned long *config); + int (*imx_pinconf_set)(struct pinctrl_dev *pctldev, unsigned int pin_id, + unsigned long *configs, unsigned int num_configs); + void (*imx_pinctrl_parse_pin)(struct imx_pinctrl *ipctl, + unsigned int *pin_id, struct imx_pin *pin, + const __be32 **list_p); }; #define IMX_CFG_PARAMS_DECODE(p, m, o) \ @@ -137,7 +144,7 @@ struct imx_pinctrl { int imx_pinctrl_probe(struct platform_device *pdev, const struct imx_pinctrl_soc_info *info); -#ifdef CONFIG_PINCTRL_IMX_SCU +#if IS_ENABLED(CONFIG_PINCTRL_IMX_SCU) #define BM_PAD_CTL_GP_ENABLE BIT(30) #define BM_PAD_CTL_IFMUX_ENABLE BIT(31) #define BP_PAD_CTL_IFMUX 27 @@ -150,23 +157,5 @@ int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, unsigned pin_id, void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, unsigned int *pin_id, struct imx_pin *pin, const __be32 **list_p); -#else -static inline int imx_pinconf_get_scu(struct pinctrl_dev *pctldev, - unsigned pin_id, unsigned long *config) -{ - return -EINVAL; -} -static inline int imx_pinconf_set_scu(struct pinctrl_dev *pctldev, - unsigned pin_id, unsigned long *configs, - unsigned num_configs) -{ - return -EINVAL; -} -static inline void imx_pinctrl_parse_pin_scu(struct imx_pinctrl *ipctl, - unsigned int *pin_id, - struct imx_pin *pin, - const __be32 **list_p) -{ -} #endif #endif /* __DRIVERS_PINCTRL_IMX_H */ diff --git a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c index 12b97da..d3020c0 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx8dxl.c +++ b/drivers/pinctrl/freescale/pinctrl-imx8dxl.c @@ -159,6 +159,9 @@ static struct imx_pinctrl_soc_info imx8dxl_pinctrl_info = { .pins = imx8dxl_pinctrl_pads, .npins = ARRAY_SIZE(imx8dxl_pinctrl_pads), .flags = IMX_USE_SCU, + .imx_pinconf_get = imx_pinconf_get_scu, + .imx_pinconf_set = imx_pinconf_set_scu, + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, }; static const struct of_device_id imx8dxl_pinctrl_of_match[] = { diff --git a/drivers/pinctrl/freescale/pinctrl-imx8qm.c b/drivers/pinctrl/freescale/pinctrl-imx8qm.c index 095acf4..8f46b940 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx8qm.c +++ b/drivers/pinctrl/freescale/pinctrl-imx8qm.c @@ -292,6 +292,9 @@ static const struct imx_pinctrl_soc_info imx8qm_pinctrl_info = { .pins = imx8qm_pinctrl_pads, .npins = ARRAY_SIZE(imx8qm_pinctrl_pads), .flags = IMX_USE_SCU, + .imx_pinconf_get = imx_pinconf_get_scu, + .imx_pinconf_set = imx_pinconf_set_scu, + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, }; static const struct of_device_id imx8qm_pinctrl_of_match[] = { diff --git a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c index 81ebd4c..6776ad6 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx8qxp.c +++ b/drivers/pinctrl/freescale/pinctrl-imx8qxp.c @@ -198,6 +198,9 @@ static struct imx_pinctrl_soc_info imx8qxp_pinctrl_info = { .pins = imx8qxp_pinctrl_pads, .npins = ARRAY_SIZE(imx8qxp_pinctrl_pads), .flags = IMX_USE_SCU, + .imx_pinconf_get = imx_pinconf_get_scu, + .imx_pinconf_set = imx_pinconf_set_scu, + .imx_pinctrl_parse_pin = imx_pinctrl_parse_pin_scu, }; 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@nxp.com>"); +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver"); +MODULE_LICENSE("GPL v2");
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. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- drivers/pinctrl/freescale/Kconfig | 2 +- drivers/pinctrl/freescale/pinctrl-imx.c | 8 ++-- drivers/pinctrl/freescale/pinctrl-imx.h | 57 ++++++++++++----------------- drivers/pinctrl/freescale/pinctrl-imx8dxl.c | 3 ++ drivers/pinctrl/freescale/pinctrl-imx8qm.c | 3 ++ drivers/pinctrl/freescale/pinctrl-imx8qxp.c | 3 ++ drivers/pinctrl/freescale/pinctrl-scu.c | 5 +++ 7 files changed, 42 insertions(+), 39 deletions(-)