Hi Linus:
thanks for your review..
On 10:58 Mon 23 Sep , Linus Walleij wrote:
> 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>
>
ok
> > +#include <linux/export.h>
>
> Why?
>
will drop
> > +#include <linux/pinctrl/consumer.h>
>
> Why?
will drop
>
> > +#include <linux/pinctrl/machine.h>
>
> Why?
will drop
>
> > +#include "../core.h"
for pinctrl_generic_add_group()
for pinctrl_generic_get_group()
..
> > +#include "../pinctrl-utils.h"
for pinctrl_utils_free_map()
> > +#include "../pinconf.h"
for pinconf_generic_parse_dt_config()
> > +#include "../pinmux.h"
for pinmux_generic_add_function()
for pinmux_generic_get_function_count()
for pinmux_generic_get_function_name()
for pinmux_generic_get_function_groups()
..
>
> All of them, really?
>
can't, or any suggestion? if I'm taking wrong approach..
> > +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.
>
also used in spacemit_pctrl_dbg_show(), it would be more clean to show pins' offset
so I will keep this
> > +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.
>
ok
> > +
> > +static unsigned int spacemit_dt_get_pin_mux(u32 value)
> > +{
> > + return value & GENMASK(15, 0);
> > +}
>
> Return static u16
>
ok
> > +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).
>
good idea, I will fix all locks according to this..
> > + 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().
>
good suggestion, will fix it
> > +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.
>
ditto
> > +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?
>
right, will fix it
> > +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.
>
ok
> Yours,
> Linus Walleij
--
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55
