RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
> Subject: RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as > module > > > From: Anson Huang > > 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 > > --- > > 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/p
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
> From: Anson Huang > 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 > --- > 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, >
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
> From: Anson Huang > 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 > > wrote: > > > > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU > > > > pinctrl driver as module > > > > +MODULE_AUTHOR("Dong Aisheng"); > > > > +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
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
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 > --- > 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
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
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 > wrote: > > > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl > > > driver as module > > > +MODULE_AUTHOR("Dong Aisheng"); > > > +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
Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
On Fri, Jul 17, 2020 at 11:24 AM Anson Huang wrote: > > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver > > as module > > +MODULE_AUTHOR("Dong Aisheng"); > > +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
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
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 > #include > +#include > #include > #include > #include > @@ -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"); > +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
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 #include +#include #include #include #include @@ -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"); +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver"); +MODULE_LICENSE("GPL v2"); This can be in a separate patch.
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
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
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). thanks Daniel.
RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module
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
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. Signed-off-by: Anson Huang --- 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 mem