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

2020-09-07 Thread Anson Huang 


> 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, );
> > +   ret = info->imx_pinconf_get(pctldev, pin_id, );
> > 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, >pins[i],
> > +   info->imx_pinctrl_parse_pin(ipctl, >pins[i],
> >   pin, );
> > else
> > imx_pinctrl_parse_pin_mmio(ipctl, >pins[i], diff 
> > --git
> > a/drivers/pinctrl/freescale/pinctrl-imx.h
> > b/drivers/pinctrl/freescale/pinctrl-imx.h
> > index 333d32b..

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

2020-09-07 Thread Aisheng Dong 
> 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, );
> + ret = info->imx_pinconf_get(pctldev, pin_id, );
>   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, >pins[i],
> + info->imx_pinctrl_parse_pin(ipctl, >pins[i],
> pin, );
>   else
>   imx_pinctrl_parse_pin_mmio(ipctl, >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;
>

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

2020-09-07 Thread Aisheng Dong 
> 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

2020-08-31 Thread Anson Huang 
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, );
> + ret = info->imx_pinconf_get(pctldev, pin_id, );
>   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, >pins[i],
> + info->imx_pinctrl_parse_pin(ipctl, >pins[i],
> pin, );
>   else
>   imx_pinctrl_parse_pin_mmio(ipctl, >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_

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

2020-07-17 Thread Anson Huang 
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

2020-07-17 Thread Arnd Bergmann 
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

2020-07-17 Thread Anson Huang 
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

2020-07-17 Thread Daniel Baluta 

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

2020-07-16 Thread Anson Huang 
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

2020-07-16 Thread Daniel Baluta 

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

2020-07-16 Thread Anson Huang 
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

2020-07-16 Thread Daniel Baluta 

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, );
+   ret = info->imx_pinconf_get(pctldev, pin_id, );
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, >pins[i],
+   info->imx_pinctrl_parse_pin(ipctl, >pins[i],
  pin, );
else
imx_pinctrl_parse_pin_mmio(ipctl, >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 {
-

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

2020-07-16 Thread Anson Huang 
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, );
+   ret = info->imx_pinconf_get(pctldev, pin_id, );
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, >pins[i],
+   info->imx_pinctrl_parse_pin(ipctl, >pins[i],
  pin, );
else
imx_pinctrl_parse_pin_mmio(ipctl, >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