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

[Anson Huang <anson.huang@xxxxxxx>]

> Subject: RE: [PATCH V5 1/9] pinctrl: imx: Support building SCU pinctrl driver as
> module
> 
> > From: Anson Huang <Anson.Huang@xxxxxxx>
> > 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 <Anson.Huang@xxxxxxx>
> > ---
> > 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 ++++++++++++-----------------
> >  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,
> >  	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.

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

Anson.