Message-ID: <Y9qFxbDIzJClmEKz@linux-8mug>
Date:   Wed, 1 Feb 2023 23:31:17 +0800
From:   Chester Lin <clin@...e.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Andreas Färber <afaerber@...e.de>, s32@....com,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Larisa Grigore <larisa.grigore@....com>,
        Ghennadi Procopciuc <Ghennadi.Procopciuc@....nxp.com>,
        Andrei Stefanescu <andrei.stefanescu@....com>,
        Radu Pirea <radu-nicolae.pirea@....com>,
        Matthias Brugger <mbrugger@...e.com>,
        Fabio Estevam <festevam@...il.com>,
        Matthew Nunez <matthew.nunez@....com>,
        Phu Luu An <phu.luuan@....com>,
        Stefan-Gabriel Mirea <stefan-gabriel.mirea@....com>
Subject: Re: [PATCH v4 2/3] pinctrl: add NXP S32 SoC family support

Hi Linus,

Sorry for the late reply and thank you for reviewing this patch!

On Thu, Jan 26, 2023 at 02:25:50PM +0100, Linus Walleij wrote:
> Hi Chester!
> 
> thanks for your patch!
> 
> This looks much better and the DT bindings are finished which is
> nice. As the driver is pretty big I need to find time to do review and
> look closer.
> 
> Here follows some concerns:
> 
> On Wed, Jan 18, 2023 at 10:47 AM Chester Lin <clin@...e.com> wrote:
> 
> > Add the pinctrl driver for NXP S32 SoC family. This driver is mainly based
> > on NXP's downstream implementation on nxp-auto-linux repo[1].
> >
> > [1] https://github.com/nxp-auto-linux/linux/tree/bsp35.0-5.15.73-rt/drivers/pinctrl/freescale
> >
> > Signed-off-by: Matthew Nunez <matthew.nunez@....com>
> > Signed-off-by: Phu Luu An <phu.luuan@....com>
> > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@....com>
> > Signed-off-by: Larisa Grigore <larisa.grigore@....com>
> > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@....nxp.com>
> > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@....com>
> > Signed-off-by: Radu Pirea <radu-nicolae.pirea@....com>
> > Signed-off-by: Chester Lin <clin@...e.com>
> 
> (...)
> 
> > +++ b/drivers/pinctrl/nxp/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config PINCTRL_S32CC
> > +       bool
> > +       depends on ARCH_S32 && OF
> > +       select GENERIC_PINCTRL_GROUPS
> > +       select GENERIC_PINMUX_FUNCTIONS
> > +       select GENERIC_PINCONF
> 
> Maybe select REGMAP_MMIO
> Maybe select GPIO_GENERIC or GPIO_REGMAP
> see further below.
> 
> > +#ifdef CONFIG_PM_SLEEP
> > +int s32_pinctrl_resume(struct device *dev);
> > +int s32_pinctrl_suspend(struct device *dev);
> > +#endif
> 
> I think these are usually handled by tagging the functions with __maybe_unused.
> 

Will fix it.

> > +static u32 get_pin_no(u32 pinmux)
> > +{
> > +       return pinmux >> S32CC_PIN_NO_SHIFT;
> 
> Maybe add a mask too so it is clear that you just rely
> on bits being shifted out to the righy.
> 

Will do.

> > +static inline int s32_pinctrl_readl_nolock(struct pinctrl_dev *pctldev,
> > +                                          unsigned int pin,
> > +                                          unsigned long *config)
> > +{
> > +       struct s32_pinctrl_mem_region *region;
> > +       unsigned int offset;
> > +
> > +       region = s32_get_region(pctldev, pin);
> > +       if (!region)
> > +               return -EINVAL;
> > +
> > +       offset = pin - region->pin_range->start;
> > +
> > +       *config = readl(region->base + S32_PAD_CONFIG(offset));
> > +
> > +       return 0;
> > +}
> > +
> > +static inline int s32_pinctrl_readl(struct pinctrl_dev *pctldev,
> > +                                   unsigned int pin,
> > +                                   unsigned long *config)
> > +{
> > +       struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       spin_lock_irqsave(&ipctl->reg_lock, flags);
> > +       ret = s32_pinctrl_readl_nolock(pctldev, pin, config);
> > +       spin_unlock_irqrestore(&ipctl->reg_lock, flags);
> > +
> > +       return ret;
> > +}
> > +
> > +static inline int s32_pinctrl_writel_nolock(struct pinctrl_dev *pctldev,
> > +                                           unsigned int pin,
> > +                                           unsigned long config)
> > +{
> > +       struct s32_pinctrl_mem_region *region;
> > +       unsigned int offset;
> > +
> > +       region = s32_get_region(pctldev, pin);
> > +       if (!region)
> > +               return -EINVAL;
> > +
> > +       offset = pin - region->pin_range->start;
> > +
> > +       writel(config, region->base + S32_PAD_CONFIG(offset));
> > +
> > +       return 0;
> > +
> > +}
> > +
> > +static inline int s32_pinctrl_writel(unsigned long config,
> > +                                    struct pinctrl_dev *pctldev,
> > +                                    unsigned int pin)
> > +{
> > +       struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       spin_lock_irqsave(&ipctl->reg_lock, flags);
> > +       ret = s32_pinctrl_writel_nolock(pctldev, pin, config);
> > +       spin_unlock_irqrestore(&ipctl->reg_lock, flags);
> > +
> > +       return ret;
> > +}
> 
> If you turn this around, *first* get the offset and *then* issye the read/write
> to respective registers, you will find that you have re-implemented
> regmap_mmio, which will take care of serializing your writes so that
> you do not need a lock either. At least consider it.
> 
> > +static int s32_update_pin_mscr(struct pinctrl_dev *pctldev, unsigned int pin,
> > +                              unsigned long mask, unsigned long new_mask)
> > +{
> > +       struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +       unsigned long config, flags;
> > +       int ret;
> > +
> > +       spin_lock_irqsave(&ipctl->reg_lock, flags);
> > +
> > +       ret = s32_pinctrl_readl_nolock(pctldev, pin, &config);
> > +       if (ret)
> > +               goto unlock;
> > +
> > +       config &= ~mask;
> > +       config |= new_mask;
> > +
> > +       ret = s32_pinctrl_writel_nolock(pctldev, pin, config);
> > +       if (ret)
> > +               goto unlock;
> 
> And after having pointed out how regmap MMIO was reimplemented,
> here you re-implement regmap_update_bits() which performs mask
> and set.
> 

Thanks for your suggestion.

IIUC, that means I should use devm_regmap_init_mmio() to acquire a regmap and
then use regmap APIs to access registers, such as regmap_read(), regmap_write()
or regmap_update_bits(). My main concern is that this driver would need a regmap
array to map all register base addresses since they are scattered based on the
memory map of S32G2 SoC.

> > +static int s32_pinconf_get(struct pinctrl_dev *pctldev,
> > +                          unsigned int pin_id,
> > +                          unsigned long *config)
> > +{
> > +       int ret = s32_pinctrl_readl(pctldev, pin_id, config);
> > +
> > +       if (ret)
> > +               return -EINVAL;
> > +
> > +       return 0;
> 
> This looks like unnecessary indirection since every call site has
> to check the return code anyway, can't you just inline the s32_pinctrl_readl()
> calls?
> 

Will fix it, thanks!

> (...)
> > +#ifdef CONFIG_PM_SLEEP
> 
> Use __maybe_unused and compile in unconditionally.
> 

Will do.

> > +static void s32_pinctrl_parse_groups(struct device_node *np,
> > +                                    struct s32_pin_group *grp,
> > +                                    struct s32_pinctrl_soc_info *info)
> > +{
> > +       const __be32 *p;
> > +       struct device *dev;
> > +       struct property *prop;
> > +       int i, npins;
> > +       u32 pinmux;
> > +
> > +       dev = info->dev;
> > +
> > +       dev_dbg(dev, "group: %s\n", np->name);
> > +
> > +       /* Initialise group */
> > +       grp->name = np->name;
> > +
> > +       npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
> 
> There is a lot of code here for handling the funky pinmux stuff. Don't we have
> generic helpers for this? Well maybe not :/
> 
> > +static void s32_pinctrl_parse_functions(struct device_node *np,
> > +                                       struct s32_pinctrl_soc_info *info,
> > +                                       u32 index)
> > +{
> > +       struct device_node *child;
> > +       struct s32_pmx_func *func;
> > +       struct s32_pin_group *grp;
> > +       u32 i = 0;
> > +
> > +       dev_dbg(info->dev, "parse function(%d): %s\n", index, np->name);
> > +
> > +       func = &info->functions[index];
> > +
> > +       /* Initialise function */
> > +       func->name = np->name;
> > +       func->num_groups = of_get_child_count(np);
> > +       if (func->num_groups == 0) {
> > +               dev_err(info->dev, "no groups defined in %s\n", np->full_name);
> > +               return;
> > +       }
> > +       func->groups = devm_kzalloc(info->dev,
> > +                       func->num_groups * sizeof(char *), GFP_KERNEL);
> > +
> > +       for_each_child_of_node(np, child) {
> > +               func->groups[i] = child->name;
> > +               grp = &info->groups[info->grp_index++];
> > +               s32_pinctrl_parse_groups(child, grp, info);
> > +               i++;
> > +       }
> > +}
> 
> This also looks like helpers we should already have, can you look around
>  a bit in other recently merged drivers?

You probably mean these two helpers in pinconf-generic.c:

  pinconf_generic_dt_node_to_map()
  pinconf_generic_dt_subnode_to_map()

Since we use specific pinmux format [as described in dt-bindings] to represent
pin ID and mux settings, to my understanding, calling generic helpers would get
nothing due to lack of generic properties, such as 'pins', 'groups' and 'function'.

Please feel free to correct me if anything wrong.

Thanks,
Chester

> 
> Yours,
> Linus Walleij

Powered by blists - more mailing lists