On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing [off-list ref] wrote:

Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
is said to feature only minor changes to these pinctrl/GPIO parts.

For each "GPIO" there are two registers for configuring the output and
output enable signals which may come from other peripherals. Among these
are two special signals that are constant 0 and constant 1 respectively.
Controlling the GPIOs from software is done by choosing one of these
signals. In other words the same registers are used for both pin muxing
and controlling the GPIOs, which makes it easier to combine the pinctrl
and GPIO driver in one.

I wrote the pinconf and pinmux parts, but the GPIO part of the code is
based on the GPIO driver in the vendor tree written by Huan Feng with
cleanups and fixes by Drew and me.

...

+       depends on OF

So this descreases test coverage.
Linus, can we provide a necessary stub so we may drop this dependency?

...

+static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
+{
+       return sfp->gc.parent;
+}
+

This seems useless helper. You may do what it's doing just in place.
It will save 5 LOCs.

...

+static void starfive_pin_dbg_show(struct pinctrl_dev *pctldev,
+                                 struct seq_file *s,
+                                 unsigned int pin)
+{
+       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+       unsigned int gpio = starfive_pin_to_gpio(sfp, pin);
+       void __iomem *reg;
+       u32 dout, doen;

+       if (gpio >= NR_GPIOS)
+               return;

Dead code?

+       reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+       dout = readl_relaxed(reg + 0x000);
+       doen = readl_relaxed(reg + 0x004);
+
+       seq_printf(s, "dout=%lu%s doen=%lu%s",
+                  dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
+                  doen & GENMASK(7, 0), (doen & BIT(31)) ? "r" : "");
+}

...

+       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+       struct device *dev = starfive_dev(sfp);
+       const char **pgnames;
+       struct pinctrl_map *map;
+       struct device_node *child;
+       const char *grpname;
+       int *pins;
+       u32 *pinmux;

Reversed xmas tree order?

+       int nmaps;
+       int ngroups;
+       int ret;

...

+static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
+                                     unsigned int gsel,
+                                     unsigned long *configs,
+                                     unsigned int num_configs)
+{
+       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+       const struct group_desc *group;
+       u16 mask, value;
+       int i;
+
+       group = pinctrl_generic_get_group(pctldev, gsel);
+       if (!group)
+               return -EINVAL;
+
+       mask = 0;
+       value = 0;
+       for (i = 0; i < num_configs; i++) {
+               int param = pinconf_to_config_param(configs[i]);
+               u32 arg = pinconf_to_config_argument(configs[i]);
+
+               switch (param) {
+               case PIN_CONFIG_BIAS_DISABLE:
+                       mask |= PAD_BIAS_MASK;
+                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
+                       break;
+               case PIN_CONFIG_BIAS_PULL_DOWN:
+                       if (arg == 0)
+                               return -ENOTSUPP;
+                       mask |= PAD_BIAS_MASK;
+                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
+                       break;
+               case PIN_CONFIG_BIAS_PULL_UP:
+                       if (arg == 0)
+                               return -ENOTSUPP;
+                       mask |= PAD_BIAS_MASK;
+                       value = value & ~PAD_BIAS_MASK;
+                       break;
+               case PIN_CONFIG_DRIVE_STRENGTH:
+                       mask |= PAD_DRIVE_STRENGTH_MASK;
+                       value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
+                               starfive_drive_strength_from_max_mA(arg);
+                       break;
+               case PIN_CONFIG_INPUT_ENABLE:
+                       mask |= PAD_INPUT_ENABLE;
+                       if (arg)
+                               value |= PAD_INPUT_ENABLE;
+                       else
+                               value &= ~PAD_INPUT_ENABLE;
+                       break;
+               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+                       mask |= PAD_INPUT_SCHMITT_ENABLE;
+                       if (arg)
+                               value |= PAD_INPUT_SCHMITT_ENABLE;
+                       else
+                               value &= ~PAD_INPUT_SCHMITT_ENABLE;
+                       break;
+               case PIN_CONFIG_SLEW_RATE:
+                       mask |= PAD_SLEW_RATE_MASK;
+                       value = (value & ~PAD_SLEW_RATE_MASK) |
+                               ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
+                       break;
+               case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
+                       if (arg) {
+                               mask |= PAD_BIAS_MASK;
+                               value = (value & ~PAD_BIAS_MASK) |
+                                       PAD_BIAS_STRONG_PULL_UP;
+                       } else {
+                               mask |= PAD_BIAS_STRONG_PULL_UP;
+                               value = value & ~PAD_BIAS_STRONG_PULL_UP;
+                       }
+                       break;
+               default:
+                       return -ENOTSUPP;
+               }
+       }
+
+       for (i = 0; i < group->num_pins; i++)
+               starfive_padctl_rmw(sfp, group->pins[i], mask, value);
+
+       return 0;
+}

...

+static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+       struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+       void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+
+       /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
+       return readl_relaxed(doen) != GPO_ENABLE;

I believe the idea was to return the predefined values for the direction.

+}

...

+static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
+{
+       struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+       irq_hw_number_t gpio = irqd_to_hwirq(d);
+       void __iomem *base = sfp->base + 4 * (gpio / 32);
+       u32 mask = BIT(gpio % 32);
+       u32 irq_type, edge_both, polarity;
+       unsigned long flags;
+
+       if (trigger & IRQ_TYPE_EDGE_BOTH)
+               irq_set_handler_locked(d, handle_edge_irq);
+       else if (trigger & IRQ_TYPE_LEVEL_MASK)
+               irq_set_handler_locked(d, handle_level_irq);

Usually we don't assign this twice, so it should be after the switch.

+       switch (trigger) {
+       case IRQ_TYPE_EDGE_RISING:
+               irq_type  = mask; /* 1: edge triggered */
+               edge_both = 0;    /* 0: single edge */
+               polarity  = mask; /* 1: rising edge */
+               break;
+       case IRQ_TYPE_EDGE_FALLING:
+               irq_type  = mask; /* 1: edge triggered */
+               edge_both = 0;    /* 0: single edge */
+               polarity  = 0;    /* 0: falling edge */
+               break;
+       case IRQ_TYPE_EDGE_BOTH:
+               irq_type  = mask; /* 1: edge triggered */
+               edge_both = mask; /* 1: both edges */
+               polarity  = 0;    /* 0: ignored */
+               break;
+       case IRQ_TYPE_LEVEL_HIGH:
+               irq_type  = 0;    /* 0: level triggered */
+               edge_both = 0;    /* 0: ignored */
+               polarity  = mask; /* 1: high level */
+               break;
+       case IRQ_TYPE_LEVEL_LOW:
+               irq_type  = 0;    /* 0: level triggered */
+               edge_both = 0;    /* 0: ignored */
+               polarity  = 0;    /* 0: low level */
+               break;
+       default:

+               irq_set_handler_locked(d, handle_bad_irq);

Why? You have it already in ->probe(), what's the point?

+               return -EINVAL;
+       }
+
+       raw_spin_lock_irqsave(&sfp->lock, flags);
+       irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
+       writel_relaxed(irq_type, base + GPIOIS);
+       edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
+       writel_relaxed(edge_both, base + GPIOIBE);
+       polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
+       writel_relaxed(polarity, base + GPIOIEV);
+       raw_spin_unlock_irqrestore(&sfp->lock, flags);
+       return 0;
+}

...

+       ret = reset_control_deassert(rst);
+       if (ret)
+               return dev_err_probe(dev, ret, "could not deassert resetd\n");

+       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
+       if (ret)

I don't see who will assert reset here.

+               return dev_err_probe(dev, ret, "could not register pinctrl driver\n");

...

+       switch (value) {
+       case 0:
+               sfp->gpios.pin_base = PAD_INVALID_GPIO;
+               goto done;
+       case 1:
+               sfp->gpios.pin_base = PAD_GPIO(0);
+               break;
+       case 2:
+               sfp->gpios.pin_base = PAD_FUNC_SHARE(72);
+               break;
+       case 3:
+               sfp->gpios.pin_base = PAD_FUNC_SHARE(70);
+               break;
+       case 4: case 5: case 6:
+               sfp->gpios.pin_base = PAD_FUNC_SHARE(0);
+               break;
+       default:

Ditto.

+               return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
+       }

...

+       ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp);
+       if (ret)

Ditto.

+               return dev_err_probe(dev, ret, "could not register gpiochip\n");
+
+done:
+       return pinctrl_enable(sfp->pctl);

Ditto.

And better to use label name like following
out_pinctrl_enable:

-- 
With Best Regards,
Andy Shevchenko