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

Andy Shevchenko
2021-10-12T17:03:37Z

On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@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