Subject: RE: [PATCH V5 1/9] pinctrl: imx: Support building SCU pinctrl driver as
module

quoted

From: Anson Huang <redacted>
Sent: Tuesday, June 16, 2020 6:44 PM

quoted

Subject: RE: [PATCH V5 1/9] pinctrl: imx: Support building SCU
pinctrl driver as module

quoted

From: Anson Huang <redacted>
Sent: Thursday, June 11, 2020 7:35 PM

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 <redacted>
---
Changes since V4:
	- add module author and description.
---
 drivers/pinctrl/freescale/Kconfig           |  2 +-
 drivers/pinctrl/freescale/pinctrl-imx.c     | 18 ++++-----
 drivers/pinctrl/freescale/pinctrl-imx.h     | 57

++++++++++++-----------------

quoted

quoted

 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     |  9 +++++
 7 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/drivers/pinctrl/freescale/Kconfig

b/drivers/pinctrl/freescale/Kconfig
index 4ca44dd..a3a30f1d 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 cb7e0f0..c1faae1 100644

--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c

@@ -372,8 +372,8 @@ static int imx_pinconf_get(struct pinctrl_dev

*pctldev,

quoted

 	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(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);
+	if ((info->flags & IMX_USE_SCU) && info->imx_pinconf_get)
+		return info->imx_pinconf_get(pctldev, pin_id, config);

Pointer check here seems not be necessary

I think it is NOT harmful and it is just in case the drivers using scu
pinctrl do NOT initialize these functions callback and lead to NULL pointer

dump.

quoted

It is a bit harmful to the code readability as we already use flag IMX_USE_SCU
to distinguish the difference. Not need double check the pointer again because
platforms driver must have defined it.

I am fine, it is just because checking the function callback before calling it is better.
I can remove it if you insist to NOT check it. If there is other comment, will remove
them together in next version.

quoted

quoted

quoted

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,

quoted

quoted

quoted

+			       unsigned long *config);
+	int (*imx_pinconf_set)(struct pinctrl_dev *pctldev, unsigned int

pin_id,

quoted

quoted

quoted

+			       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);

Compared with V4, this new implementation seems a bit complicated.
I guess we don't have to support PINCTRL_IMX=y && PINCTRL_IMX_SCU=m
case.
Will that make the support a bit easier?

I am NOT sure if such scenario meets requirement, the fact is other
non-i.MX SoC also selects the PINCTRL_IMX which will make
PINCTRL_IMX=y, so in that case, even all i.MX PINCTRL drivers are set
to module, it will still have PINCTRL_IMX=y and PINCTRL_IMX_SCU=m,
then build will fail. And I believe the auto build test may also cover
such case and build error will be reported, that is why this change is
needed and with this change, function is NOT impacted,

Is it possible to add some constrainst to make sure PINCTRL_IMX_SCU value is
the same as PINCTRL_IMX? Or combine them into one?
If we can do that, it may ease the implementation a lot and make the code still
clean.

Combine PINCTRL_IMX_SCU and PINCTRL_IMX is NOT making sense, since for non-SCU
platforms, PINCTRL_IMX_SCU is NOT necessary, to make PINCTRL_IMX_SCU same value
as PINCTRL_IMX, unless make "select PINCTRL_IMX_SCU" in PINCTRL_IMX, but that is
also NOT making sense, because, PINCTRL_IMX does NOT depends on PINCTRL_IMX_SCU
at all.

The change is NOT that big IMO, and no better idea in my mind, have tried that in previous versions
of patch series.

Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel