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

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 [off-list ref] 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 <redacted>
Signed-off-by: Phu Luu An <redacted>
Signed-off-by: Stefan-Gabriel Mirea <redacted>
Signed-off-by: Larisa Grigore <redacted>
Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com>
Signed-off-by: Andrei Stefanescu <redacted>
Signed-off-by: Radu Pirea <redacted>
Signed-off-by: Chester Lin <redacted>

(...)

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel