> From: Anson Huang <[email protected]>
> 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 <[email protected]>
> > > 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 <[email protected]>
> > > ---
> > > 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.
