Hi Yixun, thanks for your patch! Some comments and suggestions below: On Tue, Sep 3, 2024 at 2:27 PM Yixun Lan <dlan@xxxxxxxxxx> wrote: > SpacemiT's K1 SoC has a pinctrl controller which use single register > to describe all functions, which include bias pull up/down(strong pull), > drive strength, schmitter trigger, slew rate, mux mode. > > Signed-off-by: Yixun Lan <dlan@xxxxxxxxxx> > +config PINCTRL_SPACEMIT_K1 > + tristate "SpacemiT K1 SoC Pinctrl driver" > + depends on ARCH_SPACEMIT || COMPILE_TEST > + depends on OF > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + select GENERIC_PINCONF (...) > @@ -0,0 +1,1078 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2024 Yixun Lan <dlan@xxxxxxxxxx> */ > + > +#include <linux/bitfield.h> I think you really just use <linux/bits.h> > +#include <linux/export.h> Why? > +#include <linux/pinctrl/consumer.h> Why? > +#include <linux/pinctrl/machine.h> Why? > +#include "../core.h" > +#include "../pinctrl-utils.h" > +#include "../pinconf.h" > +#include "../pinmux.h" All of them, really? > +static inline void __iomem *spacemit_pin_to_reg(struct spacemit_pinctrl *pctrl, > + unsigned int pin) > +{ > + return pctrl->regs + spacemit_pin_to_offset(pin); > +} If this is the only user of spacemit_pin_to_offset() then fold it into one function, I'd say. > +static unsigned int spacemit_dt_get_pin(u32 value) > +{ > + return (value >> 16) & GENMASK(15, 0); > +} Make it a static u16 and drop the genmask, shifting 32 bits 16 bits right shifts in zeroes in the top bits. > + > +static unsigned int spacemit_dt_get_pin_mux(u32 value) > +{ > + return value & GENMASK(15, 0); > +} Return static u16 > +static void spacemit_pctrl_dbg_show(struct pinctrl_dev *pctldev, > + struct seq_file *seq, unsigned int pin) > +{ > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin); > + enum spacemit_pin_io_type type = spacemit_to_pin_io_type(spin); > + void __iomem *reg; > + u32 value; > + > + seq_printf(seq, "offset: 0x%04x ", spacemit_pin_to_offset(pin)); > + seq_printf(seq, "type: %s ", io_type_desc[type]); > + > + reg = spacemit_pin_to_reg(pctrl, pin); > + value = readl(reg); > + seq_printf(seq, "mux: %ld reg: 0x%04x", (value & PAD_MUX), value); > +} This is nice and helpful for users and debugging! > +static int spacemit_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np, > + struct pinctrl_map **maps, > + unsigned int *num_maps) > +{ > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + struct device *dev = pctrl->dev; > + struct device_node *child; > + struct pinctrl_map *map; > + const char **grpnames; > + const char *grpname; > + int ngroups = 0; > + int nmaps = 0; > + int ret; > + > + for_each_available_child_of_node(np, child) > + ngroups += 1; > + > + grpnames = devm_kcalloc(dev, ngroups, sizeof(*grpnames), GFP_KERNEL); > + if (!grpnames) > + return -ENOMEM; > + > + map = devm_kcalloc(dev, ngroups * 2, sizeof(*map), GFP_KERNEL); > + if (!map) > + return -ENOMEM; > + > + ngroups = 0; > + mutex_lock(&pctrl->mutex); Use a scoped guard in this and other instances: #include <linux/cleanup.h> guard(mutex)(&pctrl->mutex); And just drop the mutex unlock, it will be unlocked when you exit the function. (narrower scopes are possible consult git grep guard drivers/gpio). > + for_each_available_child_of_node(np, child) { Instead of the kludgy construction requireing of_node_put at the end of the function, use for_each_available_child_of_node_scoped(). > +static int spacemit_pmx_set_mux(struct pinctrl_dev *pctldev, > + unsigned int fsel, unsigned int gsel) > +{ > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct group_desc *group; > + const struct spacemit_pin_mux_config *configs; > + unsigned int i, mux; > + void __iomem *reg; > + > + group = pinctrl_generic_get_group(pctldev, gsel); > + if (!group) > + return -EINVAL; > + > + configs = group->data; > + > + for (i = 0; i < group->grp.npins; i++) { > + const struct spacemit_pin *spin = configs[i].pin; > + u32 value = configs[i].config; > + unsigned long flags; > + > + reg = spacemit_pin_to_reg(pctrl, spin->pin); > + mux = spacemit_dt_get_pin_mux(value); > + > + raw_spin_lock_irqsave(&pctrl->lock, flags); > + value = readl_relaxed(reg) & ~PAD_MUX; > + writel_relaxed(mux | value, reg); > + raw_spin_unlock_irqrestore(&pctrl->lock, flags); Use a guard() clause for the lock instead. > +static int spacemit_request_gpio(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned int pin) > +{ > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin); > + void __iomem *reg; > + > + reg = spacemit_pin_to_reg(pctrl, pin); > + writel(spin->gpiofunc, reg); Doesn't this register write require any locking? > +static int spacemit_pin_set_config(struct spacemit_pinctrl *pctrl, > + unsigned int pin, u32 value) > +{ > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin); > + void __iomem *reg; > + unsigned long flags; > + unsigned int mux; > + > + if (!pin) > + return -EINVAL; > + > + reg = spacemit_pin_to_reg(pctrl, spin->pin); > + > + raw_spin_lock_irqsave(&pctrl->lock, flags); > + mux = readl_relaxed(reg) & PAD_MUX; > + writel_relaxed(mux | value, reg); > + raw_spin_unlock_irqrestore(&pctrl->lock, flags); Use a scoped guard. Yours, Linus Walleij