lkml.org 
[lkml]   [2021]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs
    On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> 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

    \
     
     \ /
      Last update: 2021-11-02 21:03    [W:6.116 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site