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