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

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

[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 <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

[PATCH 2/2] pinctrl: imx: Support building i.MX pinctrl driver as module

[Anson Huang]

Change PINCTRL_IMX to tristate to support loadable module build.

And i.MX common pinctrl driver should depend on CONFIG_OF to make sure
no build error when i.MX common pinctrl driver is enabled for different
architectures without CONFIG_OF.

Also add module author, description and license.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/pinctrl/freescale/Kconfig       | 3 ++-
 drivers/pinctrl/freescale/pinctrl-imx.c | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig
index 570355c..922ae4b 100644
--- a/drivers/pinctrl/freescale/Kconfig
+++ b/drivers/pinctrl/freescale/Kconfig
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config PINCTRL_IMX
-	bool
+	tristate "IMX pinctrl driver"
+	depends on OF
 	select GENERIC_PINCTRL_GROUPS
 	select GENERIC_PINMUX_FUNCTIONS
 	select GENERIC_PINCONF
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index b80c450..3eaafb6 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
@@ -898,3 +899,7 @@ const struct dev_pm_ops imx_pinctrl_pm_ops = {
 					imx_pinctrl_resume)
 };
 EXPORT_SYMBOL_GPL(imx_pinctrl_pm_ops);
+
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("NXP i.MX common pinctrl driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

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

[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 <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");

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

[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

[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

[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

[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 <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.

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

[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 <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

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

[Arnd Bergmann]

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

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

[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 <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

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

[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 <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

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

[Aisheng Dong]

> 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

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

[Aisheng Dong]

> 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

RE: [PATCH 2/2] pinctrl: imx: Support building i.MX pinctrl driver as module

[Aisheng Dong]

> From: Anson Huang <Anson.Huang@nxp.com>
> Sent: Thursday, July 16, 2020 11:07 PM
> Subject: [PATCH 2/2] pinctrl: imx: Support building i.MX pinctrl driver as module
> 

S/pinctrl driver/pinctrl core driver

This also applies for Patch 1/2.

> Change PINCTRL_IMX to tristate to support loadable module build.
> 
> And i.MX common pinctrl driver should depend on CONFIG_OF to make sure no
> build error when i.MX common pinctrl driver is enabled for different
> architectures without CONFIG_OF.
> 
> Also add module author, description and license.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/pinctrl/freescale/Kconfig       | 3 ++-
>  drivers/pinctrl/freescale/pinctrl-imx.c | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig
> index 570355c..922ae4b 100644
> --- a/drivers/pinctrl/freescale/Kconfig
> +++ b/drivers/pinctrl/freescale/Kconfig
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only  config PINCTRL_IMX
> -	bool
> +	tristate "IMX pinctrl driver"

IMX pinctrl core driver

> +	depends on OF
>  	select GENERIC_PINCTRL_GROUPS
>  	select GENERIC_PINMUX_FUNCTIONS
>  	select GENERIC_PINCONF
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index b80c450..3eaafb6 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -11,6 +11,7 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_address.h>
> @@ -898,3 +899,7 @@ const struct dev_pm_ops imx_pinctrl_pm_ops = {
>  					imx_pinctrl_resume)
>  };
>  EXPORT_SYMBOL_GPL(imx_pinctrl_pm_ops);
> +
> +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");

MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");

Regards
Aisheng

> +MODULE_DESCRIPTION("NXP i.MX common pinctrl driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4

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

[Anson Huang]

> 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

RE: [PATCH 2/2] pinctrl: imx: Support building i.MX pinctrl driver as module

[Anson Huang]

> Subject: RE: [PATCH 2/2] pinctrl: imx: Support building i.MX pinctrl driver as
> module
> 
> > From: Anson Huang <Anson.Huang@nxp.com>
> > Sent: Thursday, July 16, 2020 11:07 PM
> > Subject: [PATCH 2/2] pinctrl: imx: Support building i.MX pinctrl
> > driver as module
> >
> 
> S/pinctrl driver/pinctrl core driver
> 
> This also applies for Patch 1/2.

OK

> 
> > Change PINCTRL_IMX to tristate to support loadable module build.
> >
> > And i.MX common pinctrl driver should depend on CONFIG_OF to make sure
> > no build error when i.MX common pinctrl driver is enabled for
> > different architectures without CONFIG_OF.
> >
> > Also add module author, description and license.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/pinctrl/freescale/Kconfig       | 3 ++-
> >  drivers/pinctrl/freescale/pinctrl-imx.c | 5 +++++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pinctrl/freescale/Kconfig
> > b/drivers/pinctrl/freescale/Kconfig
> > index 570355c..922ae4b 100644
> > --- a/drivers/pinctrl/freescale/Kconfig
> > +++ b/drivers/pinctrl/freescale/Kconfig
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only  config PINCTRL_IMX
> > -	bool
> > +	tristate "IMX pinctrl driver"
> 
> IMX pinctrl core driver

OK

> 
> > +	depends on OF
> >  	select GENERIC_PINCTRL_GROUPS
> >  	select GENERIC_PINMUX_FUNCTIONS
> >  	select GENERIC_PINCONF
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > index b80c450..3eaafb6 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> >  #include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_address.h>
> > @@ -898,3 +899,7 @@ const struct dev_pm_ops imx_pinctrl_pm_ops = {
> >  					imx_pinctrl_resume)
> >  };
> >  EXPORT_SYMBOL_GPL(imx_pinctrl_pm_ops);
> > +
> > +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
> 
> MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
> 

OK.

Anson