Re: [PATCH v3 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux