On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@xxxxxxxx> 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
