Re: [PATCH v4 2/3] pinctrl: add NXP S32 SoC family support

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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@xxxxxxxx> 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@xxxxxxx>
> Signed-off-by: Phu Luu An <phu.luuan@xxxxxxx>
> Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@xxxxxxx>
> Signed-off-by: Larisa Grigore <larisa.grigore@xxxxxxx>
> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxxxxxx>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxx>
> Signed-off-by: Radu Pirea <radu-nicolae.pirea@xxxxxxx>
> Signed-off-by: Chester Lin <clin@xxxxxxxx>

(...)

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

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

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

> +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?

(...)
> +#ifdef CONFIG_PM_SLEEP

Use __maybe_unused and compile in unconditionally.

> +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?

Yours,
Linus Walleij