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
