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

[Anson Huang <anson.huang@nxp.com>]

> Subject: RE: [PATCH V5 1/9] pinctrl: imx: Support building SCU pinctrl driver as
> module
> 
> > From: Anson Huang <anson.huang@nxp.com>
> > Sent: Tuesday, June 16, 2020 6:44 PM
> >
> > > Subject: RE: [PATCH V5 1/9] pinctrl: imx: Support building SCU
> > > pinctrl driver as module
> > >
> > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > > 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@nxp.com>
> > > > ---
> > > > 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.
> >
> 
> 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.

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