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

[Aisheng Dong <aisheng.dong@xxxxxxx>]

> From: Anson Huang <anson.huang@xxxxxxx>
> 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@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.
> 

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.

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

Regards
Aisheng

> Anson.