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