[PATCH v4 2/3] pinctrl: spacemit: add support for SpacemiT K1 SoC

[Linus Walleij]

Hi Yixun,

thanks for your patch!

Some comments and suggestions below:

On Tue, Sep 3, 2024 at 2:27 PM Yixun Lan <dlan at gentoo.org> 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 at gentoo.org>


> +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 at gentoo.org> */
> +
> +#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