[PATCH v1 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs
Andy Shevchenko
andy.shevchenko at gmail.com
Tue Oct 12 13:02:46 PDT 2021
On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel at esmil.dk> wrote:
>
> Add a combined pinctrl and gpio driver for the StarFive JH7100 SoC.
>
> 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 pinmuxing
> 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.
s/gpio/GPIO/g
...
> +config PINCTRL_STARFIVE
> + bool "Pinctrl and GPIO driver for the StarFive JH7100 SoC"
Why not module?
> + depends on SOC_STARFIVE || COMPILE_TEST
> + depends on OF
Do you really need this taking into account...
> + default SOC_STARFIVE
> + select GENERIC_PINCTRL_GROUPS
> + select GENERIC_PINMUX_FUNCTIONS
> + select GENERIC_PINCONF
> + select GPIOLIB
> + select GPIOLIB_IRQCHIP
> + select OF_GPIO
...this one?
...
bits.h ?
> +#include <linux/clk.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
mod_devicetable.h ?
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
Can you move these as a group after generic linux/* ones?
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/spinlock.h>
...
> +/*
> + * refer to Section 12. GPIO Registers in JH7100 datasheet:
Be consistent in your style, here for example missed capitalization.
> + * https://github.com/starfive-tech/StarLight_Docs
Is it possible to have the datasheet to be provided as Datasheet: tag
in the commit message?
> + */
...
> +/*
> + * Global enable for GPIO interrupts, offset: 0x0, field: GPIOEN
> + * set to 1 if GPIO interrupts are enabled, set to 0 to disable
> + */
> +#define IRQ_GLOBAL_EN 0x0
s/0x0/0x00/g
...
> +/*
> + * Interrupt Type for GPIO[31:0], offset: 0x10, field: GPIOS_0
> + * set to 1 if edge-triggered, set to 0 for level-triggered
> + */
> +#define IRQ_TYPE_LOW 0x10
> +
> +/*
> + * Interrupt Type for GPIO[63:32], offset: 0x14, field: GPIOS_1
> + */
> +#define IRQ_TYPE_HIGH 0x14
As I reviewed below, the IRQ is represented by a few registers in a
row, no need to define low and high separately. Ditto for the rest
register pairs.
...
> +/*
> + * Interrupt Status after Masking GPIO[31:0], offset: 0x40, field: GPIOMIS_0
> + * status of edge-triggered or level-triggered after masking
> + * value of 1 means edge or level was detected, value of 0 menas not detected
menas?!
> + */
...
> +/*
> + * Data Value of GPIO for GPIO[31:0], offest: 0x48, field: GPIODIN_0
offest?!
> + * dynamically reflects value on the GPIO pin
> + */
Please, run a spellchecker.
...
> +#define IO_PADSHARE_SEL 0x1a0
Okay, make all registers to be fixed width, i.e. 0x000 for IRQ global
enabling and so on.
...
> +#define PAD_SLEW_RATE_MASK 0xe00U
GENMASK()
> +#define PAD_BIAS_STRONG_PULL_UP 0x100U
> +#define PAD_INPUT_ENABLE 0x080U
> +#define PAD_INPUT_SCHMITT_ENABLE 0x040U
> +#define PAD_BIAS_DISABLE 0x020U
> +#define PAD_BIAS_PULL_DOWN 0x010U
All above seems like BIT(_something_).
> +#define PAD_BIAS_MASK 0x130U
> +#define PAD_DRIVE_STRENGTH_MASK 0x007U
GENMASK()
...
> +#ifdef CONFIG_DEBUG_FS
__maybe_unused ?
> +#else
> +#define starfive_pin_dbg_show NULL
> +#endif
...
> + dout = readl_relaxed(reg);
readl_relaxed(reg + 0x00)
> + reg += 4;
> + doen = readl_relaxed(reg);
readl_relaxed(reg + 0x04);
...
> + seq_printf(s, "dout=%u%s doen=%u%s",
> + dout & 0xffU, (dout & 0x80000000U) ? "r" : "",
> + doen & 0xffU, (doen & 0x80000000U) ? "r" : "");
GENMASK()
BIT()
...
> + for_each_child_of_node(np, child) {
> + const __be32 *pinmux_list;
> + const __be32 *pins_list;
> + int pinmux_size;
> + int pins_size;
> +
> + pinmux_list = of_get_property(child, "pinmux", &pinmux_size);
> + pins_list = of_get_property(child, "pins", &pins_size);
> + if (pinmux_list && pins_list) {
> + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n",
> + np, child, "both pinmux and pins set");
> + of_node_put(child);
> + return -EINVAL;
> + }
> +
> + if (pinmux_list && pinmux_size > 0) {
> + nmaps += 2;
> + } else if (pins_list && pins_size > 0) {
> + nmaps += 1;
> + } else {
> + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n",
> + np, child, "neither pinmux nor pins set");
> + of_node_put(child);
> + return -EINVAL;
> + }
> + ngroups += 1;
> + }
This entire loop seems like
1) it should be based on something from pin control core;
2) it's using some low level APIs instead of better ones like
of_property_read_uXX();
3) smells like unoptimized NIH.
...
> + if ((list = of_get_property(child, "pinmux", &npins))) {
Why not of_property_read_...() ?
...
> + u32 v = be32_to_cpu(*list++);
My gosh!
...
> + for (i = 0; i < npins; i++)
> + pins[i] = be32_to_cpu(*list++);
Ditto.
Even for this we have something in byteorder headers.
Summary, make sure you use much better _existing_ APIs instead of the
above crap.
...
> +free_pinmux:
> + devm_kfree(dev, pinmux);
> +free_pins:
> + devm_kfree(dev, pins);
> +free_grpname:
> + devm_kfree(dev, grpname);
What the heck?!
> +free_pgnames:
> + devm_kfree(dev, pgnames);
Ditto.
...
> +out:
Useless label.
> + return ret;
...
> + for (i = 0; i < group->num_pins; i++) {
> + unsigned int gpio = starfive_pin_to_gpio(sfp, group->pins[i]);
> + void __iomem *reg_dout;
> + void __iomem *reg_doen;
> + void __iomem *reg_din;
> + u32 v, dout, doen, din;
> + unsigned long flags;
> + if (dev_WARN_ONCE(dev, gpio >= MAX_GPIO,
What?!
> + "%s: invalid gpiomux pin", group->name))
> + continue;
> +
> + v = pinmux[i];
> + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU);
> + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU);
> + din = (v >> 8) & 0xffU;
What is this voodoo for?
> + if (din != 0xff)
> + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din;
> + else
> + reg_din = NULL;
This looks like you maybe use gpio-regmap instead?
...
> + void __iomem *reg = sfp->padctl + 4 * (pin / 2);
> + u32 value = readl_relaxed(reg);
> +
> + if (pin & 1U)
> + value >>= 16;
> + return value;
u8 shift = 16 * (pin % 2);
return readl_relaxed() >> shift;
?
Something similar for below code.
...
> +#ifdef CONFIG_DEBUG_FS
> +static const struct pin_config_item
> +starfive_pinconf_custom_conf_items[ARRAY_SIZE(starfive_pinconf_custom_params)] = {
Instead of using ARAY_SIZE() here, use static_assert().
__maybe_unused?
> + PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false),
> +};
> +#else
> +#define starfive_pinconf_custom_conf_items NULL
> +#endif
...
> +static const unsigned char starfive_drive_strength[] = {
> + 14, 21, 28, 35, 42, 49, 56, 63,
Why table? Can you simply use the formula?!
> +};
...
> + if (unlikely(!group))
Why unlikely() Must be justified here and everywhere where you are using it.
> + return -EINVAL;
> +
> + return starfive_pinconf_get(pctldev, group->pins[0], config);
> +}
...
> + case PIN_CONFIG_BIAS_DISABLE:
> + mask |= PAD_BIAS_MASK;
Use it...
> + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
...here. Ditto for the similar cases in this function and elsewhere.
After done this, you will see how you can simplify and deduplicate the
switch-cases.
...
> +#ifdef CONFIG_DEBUG_FS
__maybe_unused ?
> +#else
> +#define starfive_pinconf_dbg_show NULL
> +#endif
...
> + if (gpio < 32) {
> + value = readl_relaxed(sfp->base + GPIO_DIN_LOW);
> + value = (value >> gpio) & 1U;
Drop
> + } else {
> + value = readl_relaxed(sfp->base + GPIO_DIN_HIGH);
> + value = (value >> (gpio - 32)) & 1U;
Drop
> + }
> + return value;
return !!(value & BIT(gpio % 32));
...
> + if (arg == 0)
> + return -ENOTSUPP;
Shouldn't we return something else and pin control core will change it
to something else if needed?
> + if (arg == 0)
> + return -ENOTSUPP;
Ditto.
> + default:
> + return -ENOTSUPP;
...
> + if (gpio < 0 || gpio >= MAX_GPIO)
> + return;
> +
> + if (gpio < 32) {
> + ie = sfp->base + IRQ_ENABLE_LOW;
> + mask = BIT(gpio);
> + } else {
> + ie = sfp->base + IRQ_ENABLE_HIGH;
> + mask = BIT(gpio - 32);
> + }
See below. And update all occurrences of these lines accordingly and
everywhere. Also for IRQ may use helper functions if needed (but I
don't believe the high and low register have stride more than 4).
...
> + if (gpio < 0 || gpio >= MAX_GPIO)
> + return -EINVAL;
How is it possible to be ever triggered?
...
> + if (gpio < 32) {
> + base = sfp->base;
> + mask = BIT(gpio);
> + } else {
> + base = sfp->base + 4;
> + mask = BIT(gpio - 32);
> + }
base = sfp_base + 4 * (gpio / 32);
mask = BIT(gpio % 32);
...
> + irq_set_handler_locked(d, handle_edge_irq);
> + irq_set_handler_locked(d, handle_edge_irq);
Dup.
...
> + irq_set_handler_locked(d, handle_edge_irq);
> + irq_set_handler_locked(d, handle_level_irq);
> + irq_set_handler_locked(d, handle_level_irq);
Ditto.
...
> + irq_set_handler_locked(d, handle_bad_irq);
Why is this here? Move it to ->probe().
...
> + clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + dev_err(dev, "could not get clock: %d\n", ret);
Thank you for spamming logs with this noise.
> + return ret;
Hint: return dev_err_probe(). Ditto for the rest in this function.
> + }
...
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + reset_control_deassert(rst);
Use devm_add_action_or_reset().
> + dev_err(dev, "could not enable clock: %d\n", ret);
> + return ret;
> + }
...
> + if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) {
Can be refactored without conditional. Also, why not to use
device_property_read_u32()?
> + if (value <= 6)
> + writel(value, sfp->padctl + IO_PADSHARE_SEL);
> + else
> + dev_err(dev, "invalid signal group %u\n", value);
Why _err if you not bail out here?
> + }
...
> + value = readl(sfp->padctl + IO_PADSHARE_SEL);
> + switch (value) {
> + case 0:
> + sfp->gpios.pin_base = 0x10000;
Magic number!
> + 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:
> + dev_err(dev, "invalid signal group %u\n", value);
> + return -EINVAL;
> + }
...
> + sfp->gc.of_node = dev->of_node;
Isn't GPIO library do this for you?
...
> + starfive_irq_chip.parent_device = dev;
Ditto?
...
> + sfp->gc.irq.parents =
> + devm_kcalloc(dev, 1, sizeof(*sfp->gc.irq.parents), GFP_KERNEL);
1 -> sfp->gc.irq.num_parents
And hence move below line up.
> + if (!sfp->gc.irq.parents)
> + return -ENOMEM;
> + sfp->gc.irq.num_parents = 1;
...
> + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio);
Redundant noise.
...
> +static const struct of_device_id starfive_of_match[] = {
> + { .compatible = "starfive,jh7100-pinctrl" },
> + { /* sentinel */ },
No comma needed for terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
More information about the linux-riscv
mailing list