| Message ID | 20210602120329.2444672-1-j.neuschaefer@gmx.net |
|---|---|
| Headers | show |
| Series | Nuvoton WPCM450 pinctrl and GPIO driver | expand |
On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > This driver is heavily based on the one for NPCM7xx, because the WPCM450 > is a predecessor of those SoCs. > > The biggest difference is in how the GPIO controller works. In the > WPCM450, the GPIO registers are not organized in multiple banks, but > rather placed continually into the same register block, and the driver > reflects this. > > Some functionality implemented in the hardware was (for now) left unused > in the driver, specifically blinking and pull-up/down. ... > +config PINCTRL_WPCM450 > + bool "Pinctrl and GPIO driver for Nuvoton WPCM450" Why it can't be a module? > + depends on (ARCH_WPCM450 || COMPILE_TEST) && OF Is it really OF dependent (see below)? > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF > + select GPIOLIB > + select GPIO_GENERIC > + select GPIOLIB_IRQCHIP > + help > + Say Y here to enable pin controller and GPIO support > + for the Nuvoton WPCM450 SoC.
On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > The Global Control Registers (GCR) are a block of registers in Nuvoton > SoCs that expose misc functionality such as chip model and version > information or pinmux settings. > > This patch adds a GCR node to nuvoton-wpcm450.dtsi in preparation for > enabling pinctrl on this SoC. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> As noted I would name this architecture-neutral with syscon@... Yours, Linus Walleij
Hi Jonathan! thanks for your patch! On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > This driver is heavily based on the one for NPCM7xx, because the WPCM450 > is a predecessor of those SoCs. > > The biggest difference is in how the GPIO controller works. In the > WPCM450, the GPIO registers are not organized in multiple banks, but > rather placed continually into the same register block, and the driver > reflects this. This is unfortunate because now you can't use GPIO_GENERIC anymore. > Some functionality implemented in the hardware was (for now) left unused > in the driver, specifically blinking and pull-up/down. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> (...) > +config PINCTRL_WPCM450 > + bool "Pinctrl and GPIO driver for Nuvoton WPCM450" > + depends on (ARCH_WPCM450 || COMPILE_TEST) && OF > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF > + select GPIOLIB > + select GPIO_GENERIC You are not using GPIO_GENERIC > +struct wpcm450_port { > + /* Range of GPIOs in this port */ > + u8 base; > + u8 length; > + > + /* Register offsets (0 = register doesn't exist in this port) */ > + u8 cfg0, cfg1, cfg2; > + u8 blink; > + u8 dataout, datain; > +}; If you used to have "GPIO banks" and you now have "GPIO ports" what is the difference? Why can't these ports just be individula gpio_chip:s with their own device tree nodes inside the pin controller node? If you split it up then you can go back to using GPIO_GENERIC with bgpio_init() again which is a big win. All you seem to be doing is setting consecutive bits in a register by offset, which is what GPIO_GENERIC is for, just that it assumes offset is always from zero. If you split it into individual gpio_chips per register then you get this nice separation and code reuse. > +static const struct wpcm450_port *to_port(int offset) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(wpcm450_ports); i++) > + if (offset >= wpcm450_ports[i].base && > + offset - wpcm450_ports[i].base < wpcm450_ports[i].length) > + return &wpcm450_ports[i]; > + return NULL; > +} Then you would also get away from this awkward thing. > +static u32 port_mask(const struct wpcm450_port *port, int offset) > +{ > + return BIT(offset - port->base); > +} And awkwardness like this. Generally splitting up gpio_chips is a very good idea. > +static int event_bitmask(int gpio) > +{ > + if (gpio >= 0 && gpio < 16) > + return BIT(gpio); > + if (gpio == 24 || gpio == 25) > + return BIT(gpio - 8); > + return -EINVAL; > +} > + > +static int event_bitnum_to_gpio(int num) > +{ > + if (num >= 0 && num < 16) > + return num; > + if (num == 16 || num == 17) > + return num + 8; > + return -EINVAL; > +} This is also a sign that you have several gpio_chips in practice and now you need all this awkwardness to get back to which GPIO is which instead of handling it in a per-chip manner. This can be done in different ways, the most radical is to have the pin control driver spawn child devices for each GPIO block/bank/port with its own driver, but it can also just register the individual gpio_chips. Yours, Linus Walleij
Hello, On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote: > On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer > <j.neuschaefer@gmx.net> wrote: > > > > This driver is heavily based on the one for NPCM7xx, because the WPCM450 > > is a predecessor of those SoCs. > > > > The biggest difference is in how the GPIO controller works. In the > > WPCM450, the GPIO registers are not organized in multiple banks, but > > rather placed continually into the same register block, and the driver > > reflects this. > > > > Some functionality implemented in the hardware was (for now) left unused > > in the driver, specifically blinking and pull-up/down. > > ... > > > +config PINCTRL_WPCM450 > > + bool "Pinctrl and GPIO driver for Nuvoton WPCM450" > > Why it can't be a module? My mistake, it does indeed work as a module. I'll change it to tristate. > > + depends on (ARCH_WPCM450 || COMPILE_TEST) && OF > > Is it really OF dependent (see below)? After switching to the fwnode API, no. I'll drop the dependency. > > + select PINMUX > > + select PINCONF > > + select GENERIC_PINCONF > > + select GPIOLIB > > + select GPIO_GENERIC > > + select GPIOLIB_IRQCHIP > > + help > > + Say Y here to enable pin controller and GPIO support > > + for the Nuvoton WPCM450 SoC. > > >From this help it's not clear why user should or shouldn't enable it. > Please, elaborate (and hence fix checkpatch warning). I'll try something like this, but I'm open for better ideas: help Say Y or M here to enable pin controller and GPIO support for the Nuvoton WPCM450 SoC. This is strongly recommended when building a kernel that will run on this chip. If this driver is compiled as a module, it will be named pinctrl-wpcm450. I could mention some examples of functionality enabled by this driver: LEDs, host power control, Ethernet. (LEDs and host power control use GPIOs, at least on the Supermicro X9 board I've been using. Ethernet MDIO must be enabled through the pinctrl driver, unless the bootloader has done so already; on my board the bootloader doesn't do this.) > > +#include <linux/module.h> > > mod_devicetable.h I'll add it. > > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > > +#include <linux/pinctrl/machine.h> > > +#include <linux/pinctrl/pinconf.h> > > +#include <linux/pinctrl/pinconf-generic.h> > > +#include <linux/pinctrl/pinctrl.h> > > +#include <linux/pinctrl/pinmux.h> > > Can you move this group... > > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > ...here? > > It will show the relation with pin control subsystem and separate from > global / generic headers. Will do. > > + * This spinlock protects registers and struct wpcm450_pinctrl fields > > spin lock Ok > > +/* GPIO handling in the pinctrl driver */ > > + > > Useless. Ok > > ... > > > +static int wpcm450_gpio_get_direction(struct gpio_chip *chip, > > + unsigned int offset) > > +{ > > + struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip); > > + const struct wpcm450_port *port = to_port(offset); > > + unsigned long flags; > > + u32 cfg0; > > + int dir; > > + > > + spin_lock_irqsave(&pctrl->lock, flags); > > + if (port->cfg0) { > > + cfg0 = ioread32(pctrl->gpio_base + port->cfg0); > > > + dir = !(cfg0 & port_mask(port, offset)); > > + } else { > > + /* If cfg0 is unavailable, the GPIO is always an input */ > > + dir = 1; > > + } > > Why above is under spin lock? > Same question for all other similar places in different functions, if any. My intention was to protect the ioread32. But given that it's just a single MMIO read, this might be unnecessary. wpcm450_gpio_direction_input and a few other functions implement a read/modify/write cycle, where the lock makes more sense: spin_lock_irqsave(&pctrl->lock, flags); if (port->cfg0) { cfg0 = ioread32(pctrl->gpio_base + port->cfg0); cfg0 &= ~port_mask(port, offset); iowrite32(cfg0, pctrl->gpio_base + port->cfg0); } spin_unlock_irqrestore(&pctrl->lock, flags); (Here, as above, the locking calls moved into the if block, but I'm not sure how much of an improvement that would be.) > > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > + return dir; > > +} > > ... > > > +static int wpcm450_gpio_direction_output(struct gpio_chip *chip, > > + unsigned int offset, int value) > > +{ > > + struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip); > > + const struct wpcm450_port *port = to_port(offset); > > + unsigned long flags; > > + u32 dataout, cfg0; > > > + int ret = 0; > > Redundant. Can do it without it. > > > + spin_lock_irqsave(&pctrl->lock, flags); > > + if (port->cfg0) { > > > + } else { > > + ret = -EINVAL; > > + } > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > + return ret; > > +} I'll refactor it to return -EINVAL early. > > +/* Interrupt support */ > > + > > Useless besides being in a bad style. Ok, will remove. > > ... > > > +static int event_bitmask(int gpio) > > +{ > > + if (gpio >= 0 && gpio < 16) > > + return BIT(gpio); > > + if (gpio == 24 || gpio == 25) > > + return BIT(gpio - 8); > > + return -EINVAL; > > +} > > Can you consider to use bitmap_bitremap() > > +static int event_bitnum_to_gpio(int num) > > +{ > > + if (num >= 0 && num < 16) > > + return num; > > + if (num == 16 || num == 17) > > + return num + 8; > > + return -EINVAL; > > +} > > Ditto. > > See gpio-xilinx.c for example. I looked at it now. I'm not convinced it'll improve readability here. > > ... > > > +static void wpcm450_gpio_irq_ack(struct irq_data *d) > > +{ > > + struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d)); > > > + int mask = event_bitmask(d->hwirq); > > Move the assignment closer to the check. > Ditto for other same and similar cases in the code. Will do. > > > + unsigned long flags; > > + > > + if (mask < 0) > > + return; > > > +} > > ... > > > + int mask = event_bitmask(d->hwirq); > > Use irqd_to_hwirq() (please check that I spelled it correctly). > Same for all hwirq getters. Will do. > > ... > > > +static void wpcm450_gpio_fix_evpol(struct wpcm450_pinctrl *pctrl, unsigned long all) > > +{ > > + int bitnum; > > Can it be negative? No. I'll change it to unsigned int. > > > + for_each_set_bit(bitnum, &all, 32) { > > > + int gpio = event_bitnum_to_gpio(bitnum); > > + u32 mask = BIT(bitnum), evpol; > > unsigned long evpol; > > > + int level; > > + > > + do { > > + evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL); > > + level = wpcm450_gpio_get(&pctrl->gc, gpio); > > > + /* Switch event polarity to the opposite of the current level */ > > + if (level) > > + evpol &= ~mask; > > + else > > + evpol |= mask; > > __assign_bit(bitnum, &evpol, level); Ah, very useful. I'll use __assign_bit in a few places. > > +static int wpcm450_gpio_set_irq_type(struct irq_data *d, unsigned int flow_type) > > +{ > > Consider to assign handler type here. Will do. > > +/* pinmux handing */ > > + > > Useless. Ok. > > +/* > > + * pin: name, number > > + * group: name, npins, pins > > + * function: name, ngroups, groups > > + */ > > +struct wpcm450_group { > > + const char *name; > > + const unsigned int *pins; > > + int npins; > > +}; > > Use struct group_desc from core.h. Ok. > > +/* pinctrl_ops */ > > Useless. Ok > > > +static int wpcm450_get_groups_count(struct pinctrl_dev *pctldev) > > +{ > > + struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev); > > > + dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups)); > > Ditto. Removing this and similar messages. > > +/* pinmux_ops */ > > Useless. Ok > > ... > > > +static int wpcm450_gpio_request_enable(struct pinctrl_dev *pctldev, > > + struct pinctrl_gpio_range *range, > > + unsigned int offset) > > +{ > > + struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev); Oops, I forgot to change the name to pctrl here. > > > + if (!range) { > > + dev_err(npcm->dev, "invalid range\n"); > > + return -EINVAL; > > + } > > Dead code? Indeed, the range pointer is checked in pin_request(). > > > + if (!range->gc) { > > + dev_err(npcm->dev, "invalid gpiochip\n"); > > + return -EINVAL; > > + } > > Dead code? ... and range->gc isn't used here... I'll remove the check. > > > + wpcm450_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio); > > + > > + return 0; > > +} > > ... > > > +/* Release GPIO back to pinctrl mode */ > > +static void wpcm450_gpio_request_free(struct pinctrl_dev *pctldev, > > + struct pinctrl_gpio_range *range, > > + unsigned int offset) > > +{ > > + struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + int virq; > > + > > + virq = irq_find_mapping(pctrl->domain, offset); > > + if (virq) > > + irq_dispose_mapping(virq); > > Why it needs to be done here? I copied this function from the pinctrl-npcm7xx driver; I don't really see a reason to call irq_dispose_mapping here. > What about the GPIO library API that does some additional stuff? I don't know which gpiolib function would be appropriate here, sorry. > > +/* pinconf_ops */ > > Useless. Ok > > +static int debounce_bitmask(int gpio) > > +{ > > + if (gpio >= 0 && gpio < 16) > > + return BIT(gpio); > > + return -EINVAL; > > +} > > I don't see users of it except one below, care to inline? It should have been used in wpcm450_config_set_one, but I missed that. > > +static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin, > > + unsigned long *config) > > +{ > > > + switch (param) { > > + case PIN_CONFIG_INPUT_DEBOUNCE: > > + mask = debounce_bitmask(pin); > > + if (mask < 0) > > + return mask; > > > + break; > > + default: > > + return -ENOTSUPP; > > + } > > + > > + return 0; > > +} > > ... > > > +/* Set multiple configuration settings for a pin */ > > Useless. Ok. > > ... > > > +static int wpcm450_config_set(struct pinctrl_dev *pctldev, unsigned int pin, > > + unsigned long *configs, unsigned int num_configs) > > +{ > > + struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > > + int rc; > > Why out of a sudden different (inconsistent) name? No particular reason. I'll change it to ret. > > > + return 0; > > +} > > ... > > > + if (!of_find_property(np, "gpio-controller", NULL)) > > + return -ENODEV; > > Dead code? The point here was to check if the node is marked as a GPIO controller, with the boolean property "gpio-controller" (so device_property_read_bool would probably be more appropriate). However, since the gpio-controller property is already defined as required in the DT binding, I'm not sure it's worth checking here. > > ... > > > + pctrl->gpio_base = of_iomap(np, 0); > > devm_platform_ioremap_resource(); Will change. > > > + if (!pctrl->gpio_base) { > > + dev_err(pctrl->dev, "Resource fail for GPIO controller\n"); > > + return -ENOMEM; > > + } > > Here leak of resources. See above. > > ... > > > + pctrl->gc.get_direction = wpcm450_gpio_get_direction; > > + pctrl->gc.direction_input = wpcm450_gpio_direction_input; > > + pctrl->gc.direction_output = wpcm450_gpio_direction_output; > > + pctrl->gc.get = wpcm450_gpio_get; > > + pctrl->gc.set = wpcm450_gpio_set; > > No ->set_config()? Good point, I'll hook it up to wpcm450_config_set_one(). > > ... > > > + girq->default_type = IRQ_TYPE_NONE; > > > + girq->handler = handle_level_irq; > > Use ->irq_set_type() for this. Here should be handle_bad_irq(). Ok. > > > + for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) { > > > + int irq = irq_of_parse_and_map(np, i); > > fwnode_get_irq() Indeed, will switch to fwnode_irq_get. > > > + if (irq < 0) { > > + dev_err(pctrl->dev, "No IRQ for GPIO controller\n"); > > + return irq; > > + } > > + girq->parents[i] = irq; > > + } > > ... > > > + pctrl->pctldev = devm_pinctrl_register(&pdev->dev, > > + &wpcm450_pinctrl_desc, pctrl); > > + if (IS_ERR(pctrl->pctldev)) { > > > + dev_err(&pdev->dev, "Failed to register pinctrl device\n"); > > + return PTR_ERR(pctrl->pctldev); > > Shouldn't it be return dev_err_probe(...); here? It's so far a rare pattern after devm_pinctrl_register, but it makes sense. Will change (in both cases above). (Perhaps I should also use dev_err_probe in wpcm450_gpio_register, since it's called from the probe function.) > > > + } > > ... > > > + pr_info("WPCM450 pinctrl and GPIO driver probed\n"); > > Besides you have to use dev_*() this is completely useless and noisy message. I'll remove this message and review the other messages. > > ... > > > +static const struct of_device_id wpcm450_pinctrl_match[] = { > > + { .compatible = "nuvoton,wpcm450-pinctrl" }, > > > + { }, > > Comma is not needed for terminator line. True, will remove. > > > +}; > > ... > > > + .suppress_bind_attrs = true, > > Why? Copied from pinctrl-npcm7xx, but I see no reason to keep it. > > -- > With Best Regards, > Andy Shevchenko Thank you for your detailed review, Jonthan Neuschäfer
On Fri, Jun 04, 2021 at 10:00:18AM +0200, Linus Walleij wrote: > On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer > <j.neuschaefer@gmx.net> wrote: > > > A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will > > be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and > > WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and > > version information. > > > > This patch adds a binding to describe this node. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > (...) > > > + gcr: gcr@800000 { > > + compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd"; > > + reg = <0x800000 0x1000>; > > + }; > > gcr looks a bit idiomatic, isn't > > syscon: syscon@... better? I think I'll go with syscon@..., because it's the right generic name, but gcr for the label, matching current usage. > Nitpicky though and looks good to me either way: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thanks! Jonathan Neuschäfer
On Fri, Jun 04, 2021 at 10:01:07AM +0200, Linus Walleij wrote: > On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer > <j.neuschaefer@gmx.net> wrote: > > > The Global Control Registers (GCR) are a block of registers in Nuvoton > > SoCs that expose misc functionality such as chip model and version > > information or pinmux settings. > > > > This patch adds a GCR node to nuvoton-wpcm450.dtsi in preparation for > > enabling pinctrl on this SoC. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > As noted I would name this architecture-neutral with > syscon@... Will do. Jonathan Neuschäfer
On Sun, Jun 13, 2021 at 2:20 AM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote: > > On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer > > <j.neuschaefer@gmx.net> wrote: ... > > > + help > > > + Say Y here to enable pin controller and GPIO support > > > + for the Nuvoton WPCM450 SoC. > > > > >From this help it's not clear why user should or shouldn't enable it. > > Please, elaborate (and hence fix checkpatch warning). > > I'll try something like this, but I'm open for better ideas: > > help > Say Y or M here to enable pin controller and GPIO support for > the Nuvoton WPCM450 SoC. This is strongly recommended when > building a kernel that will run on this chip. > > If this driver is compiled as a module, it will be named > pinctrl-wpcm450. This looks good enough. > I could mention some examples of functionality enabled by this driver: > LEDs, host power control, Ethernet. > > (LEDs and host power control use GPIOs, at least on the Supermicro X9 > board I've been using. Ethernet MDIO must be enabled through the > pinctrl driver, unless the bootloader has done so already; on my board > the bootloader doesn't do this.) ... > > > +static int wpcm450_gpio_get_direction(struct gpio_chip *chip, > > > + unsigned int offset) > > > +{ > > > + struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip); > > > + const struct wpcm450_port *port = to_port(offset); > > > + unsigned long flags; > > > + u32 cfg0; > > > + int dir; > > > + > > > + spin_lock_irqsave(&pctrl->lock, flags); > > > + if (port->cfg0) { > > > + cfg0 = ioread32(pctrl->gpio_base + port->cfg0); > > > > > + dir = !(cfg0 & port_mask(port, offset)); > > > + } else { > > > + /* If cfg0 is unavailable, the GPIO is always an input */ > > > + dir = 1; > > > + } > > > > Why above is under spin lock? > > Same question for all other similar places in different functions, if any. > > My intention was to protect the ioread32. But given that it's just a > single MMIO read, this might be unnecessary. Sometimes it's necessary and I'm not talking about it. (I put blank lines around the code I was commenting on) So, What I meant above is to get something like this if (port->cfg0) { spin lock ... spin unlock } else { ... } or equivalent ideas. > > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > > + return dir; > > > +} ... > > > +static int wpcm450_gpio_direction_output(struct gpio_chip *chip, > > > + unsigned int offset, int value) > > > +{ > > > + struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip); > > > + const struct wpcm450_port *port = to_port(offset); > > > + unsigned long flags; > > > + u32 dataout, cfg0; > > > > > + int ret = 0; > > > > Redundant. Can do it without it. > > > > > + spin_lock_irqsave(&pctrl->lock, flags); > > > + if (port->cfg0) { > > > > > + } else { > > > + ret = -EINVAL; > > > + } > > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > > + return ret; > > > +} > > I'll refactor it to return -EINVAL early. Here a similar approach can be used. ... > > What about the GPIO library API that does some additional stuff? > > I don't know which gpiolib function would be appropriate here, sorry. When you leave those request and release callbacks untouched the GPIO library will assign default ones. You may see what they do. ... > > > + if (!of_find_property(np, "gpio-controller", NULL)) > > > + return -ENODEV; > > > > Dead code? > > The point here was to check if the node is marked as a GPIO controller, > with the boolean property "gpio-controller" (so device_property_read_bool > would probably be more appropriate). > > However, since the gpio-controller property is already defined as > required in the DT binding, I'm not sure it's worth checking here. Exactly my point. -- With Best Regards, Andy Shevchenko
On Fri, Jun 04, 2021 at 11:31:07AM +0200, Linus Walleij wrote: > Hi Jonathan! > > thanks for your patch! > > On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer > <j.neuschaefer@gmx.net> wrote: > > > > This driver is heavily based on the one for NPCM7xx, because the WPCM450 > > is a predecessor of those SoCs. > > > > The biggest difference is in how the GPIO controller works. In the > > WPCM450, the GPIO registers are not organized in multiple banks, but > > rather placed continually into the same register block, and the driver > > reflects this. > > This is unfortunate because now you can't use GPIO_GENERIC anymore. > > > Some functionality implemented in the hardware was (for now) left unused > > in the driver, specifically blinking and pull-up/down. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > (...) > > > +config PINCTRL_WPCM450 > > + bool "Pinctrl and GPIO driver for Nuvoton WPCM450" > > + depends on (ARCH_WPCM450 || COMPILE_TEST) && OF > > + select PINMUX > > + select PINCONF > > + select GENERIC_PINCONF > > + select GPIOLIB > > + select GPIO_GENERIC > > You are not using GPIO_GENERIC I'll remove the this line (depending on the outcome of the rest of the discussion). > > > +struct wpcm450_port { > > + /* Range of GPIOs in this port */ > > + u8 base; > > + u8 length; > > + > > + /* Register offsets (0 = register doesn't exist in this port) */ > > + u8 cfg0, cfg1, cfg2; > > + u8 blink; > > + u8 dataout, datain; > > +}; > > If you used to have "GPIO banks" and you now have > "GPIO ports" what is the difference? Why can't these ports > just be individula gpio_chip:s with their own device tree > nodes inside the pin controller node? The naming difference is a fairly arbitrary choice by me. The real difference is in how the GPIO registers are laid out. On NPCM7xx, there are blocks of registers at +0, +0x1000, +0x2000, etc., and within a block, the registers have the same offsets. On WPCM450, the registers are all mushed together as tightly as possible[1], so that (a) the ports/banks don't start at nice addresses, and (b) the register layout can't be predicted from the offset of the first register in a port (because not all ports have all registers). > If you split it up then you can go back to using > GPIO_GENERIC with bgpio_init() again which is a > big win. > > All you seem to be doing is setting consecutive > bits in a register by offset, which is what GPIO_GENERIC > is for, just that it assumes offset is always from zero. > If you split it into individual gpio_chips per register > then you get this nice separation and code reuse. Indeed, if I keep the wpcm450_ports table but use it to call bgpio_init() with the right register addresses, I think bgpio_init() can work. Thanks, Jonathan Neuschäfer [1]: https://github.com/neuschaefer/wpcm450/wiki/GPIOs-and-pinmux#gpio
On Sun, Jun 13, 2021 at 01:06:15PM +0300, Andy Shevchenko wrote: > On Sun, Jun 13, 2021 at 2:20 AM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote: > > > On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: [...] > > > > +static int wpcm450_gpio_get_direction(struct gpio_chip *chip, > > > > + unsigned int offset) > > > > +{ > > > > + struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip); > > > > + const struct wpcm450_port *port = to_port(offset); > > > > + unsigned long flags; > > > > + u32 cfg0; > > > > + int dir; > > > > + > > > > + spin_lock_irqsave(&pctrl->lock, flags); > > > > + if (port->cfg0) { > > > > + cfg0 = ioread32(pctrl->gpio_base + port->cfg0); > > > > > > > + dir = !(cfg0 & port_mask(port, offset)); > > > > + } else { > > > > + /* If cfg0 is unavailable, the GPIO is always an input */ > > > > + dir = 1; > > > > + } > > > > > > Why above is under spin lock? > > > Same question for all other similar places in different functions, if any. > > > > My intention was to protect the ioread32. But given that it's just a > > single MMIO read, this might be unnecessary. > > Sometimes it's necessary and I'm not talking about it. (I put blank > lines around the code I was commenting on) > > So, What I meant above is to get something like this > > if (port->cfg0) { > spin lock > ... > spin unlock > } else { > ... > } > > or equivalent ideas. Ah, in other words: Narrowing the scope of the lock as far as possible. I'll keep it in mind for v2. > > > What about the GPIO library API that does some additional stuff? > > > > I don't know which gpiolib function would be appropriate here, sorry. > > When you leave those request and release callbacks untouched the GPIO > library will assign default ones. You may see what they do. Ah, I see. I'll look into it. > ... > > > > > + if (!of_find_property(np, "gpio-controller", NULL)) > > > > + return -ENODEV; > > > > > > Dead code? > > > > The point here was to check if the node is marked as a GPIO controller, > > with the boolean property "gpio-controller" (so device_property_read_bool > > would probably be more appropriate). > > > > However, since the gpio-controller property is already defined as > > required in the DT binding, I'm not sure it's worth checking here. > > Exactly my point. Alright. Thanks, Jonthan Neuschäfer
On Wed, Jun 02, 2021 at 02:03:22PM +0200, Jonathan Neuschäfer wrote: > A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will > be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and > WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and > version information. > > This patch adds a binding to describe this node. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > .../bindings/arm/npcm/nuvoton,gcr.yaml | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml > > diff --git a/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml b/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml > new file mode 100644 > index 0000000000000..3174279f7713a > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml > @@ -0,0 +1,38 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/arm/npcm/nuvoton,gcr.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Global Control Registers block in Nuvoton SoCs > + > +maintainers: > + - Jonathan Neuschäfer <j.neuschaefer@gmx.net> > + > +description: | > + The Global Control Registers (GCR) are a block of registers in Nuvoton SoCs > + that expose misc functionality such as chip model and version information or > + pinmux settings. > + > +properties: > + compatible: > + items: > + - enum: > + - nuvoton,wpcm450-gcr > + - nuvoton,npcm750-gcr > + - const: syscon > + - const: simple-mfd How is this a simple-mfd? There are no child nodes. > + reg: true > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + gcr: gcr@800000 { > + compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd"; > + reg = <0x800000 0x1000>; > + }; > -- > 2.30.2
On Tue, Jun 15, 2021 at 05:43:21PM -0600, Rob Herring wrote: > On Wed, Jun 02, 2021 at 02:03:22PM +0200, Jonathan Neuschäfer wrote: > > A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will > > be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and > > WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and > > version information. > > > > This patch adds a binding to describe this node. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > --- [...] > > +properties: > > + compatible: > > + items: > > + - enum: > > + - nuvoton,wpcm450-gcr > > + - nuvoton,npcm750-gcr > > + - const: syscon > > + - const: simple-mfd > > How is this a simple-mfd? There are no child nodes. Some device trees, such as arch/arm/boot/dts/nuvoton-npcm730-gbs.dts, have subnodes of the GCR node (specifically, they (ab)use mmio-mux to set specific registers to specific values). > > + reg: true > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + gcr: gcr@800000 { > > + compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd"; > > + reg = <0x800000 0x1000>; > > + }; ... I guess I should add a child node to the example, here. Best regards, Jonathan Neuschäfer
This series adds support for pinctrl and GPIO in the Nuvoton WPCM450 SoC. Both my DT bindings and my driver are based on the work done by others for the newer Nuvoton NPCM7xx SoC, and I've tried to keep both similar. Instead of extending the pinctrl-npcm7xx driver to add WPCM450 support, I copied/forked it. The pinmux mechanism is very similar (using MFSEL1 and MFSEL2 registers), but the GPIO register interface has been redesigned for NPCM7xx; adding support for the older GPIO controller would make the driver harder to understand, while only enabling a small amount of code sharing. The DT binding in YAML format might make a good template for also converting the nuvoton,npcm7xx-pinctrl binding to YAML. Both in the DT binding and in the driver I kept the name "pinctrl". For the driver, I find it accurate enough because it handles pinctrl and GPIO. For the DT node, it's a bit less accurate because the register block at 0xb8003000 is about GPIOs, and pin control happens in the global control registers (GCR) block, except for input debouncing. So, "GPIO" might be the more appropriate name component there. Jonathan Neuschäfer (8): dt-bindings: arm/npcm: Add binding for global control registers (GCR) MAINTAINERS: Match all of bindings/arm/npcm/ as part of NPCM architecture ARM: dts: wpcm450: Add global control registers (GCR) node dt-bindings: pinctrl: Add Nuvoton WPCM450 pinctrl: nuvoton: Add driver for WPCM450 ARM: dts: wpcm450: Add pinctrl node ARM: dts: wpcm450: Add pin functions ARM: dts: wpcm450-supermicro-x9sci-ln4f: Add GPIO LEDs and buttons .../bindings/arm/npcm/nuvoton,gcr.yaml | 38 + .../pinctrl/nuvoton,wpcm450-pinctrl.yaml | 142 ++ MAINTAINERS | 2 + .../nuvoton-wpcm450-supermicro-x9sci-ln4f.dts | 27 + arch/arm/boot/dts/nuvoton-wpcm450.dtsi | 321 +++++ drivers/pinctrl/Makefile | 2 +- drivers/pinctrl/nuvoton/Kconfig | 14 + drivers/pinctrl/nuvoton/Makefile | 1 + drivers/pinctrl/nuvoton/pinctrl-wpcm450.c | 1230 +++++++++++++++++ 9 files changed, 1776 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml create mode 100644 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c -- 2.30.2