Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 12 Oct 2021 23:02:46 +0300 | Subject | Re: [PATCH v1 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs |
| |
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
| |