Hello and happy (belated) new year, On Fri, Dec 24, 2021 at 11:15:04PM +0200, Andy Shevchenko wrote: > On Fri, Dec 24, 2021 at 10:10 PM Jonathan Neuschäfer > wrote: > > > > This driver is based on the one for NPCM7xx, because the WPCM450 is a > > predecessor of those SoCs. Notable differences: > > > > - WPCM450, the GPIO registers are not organized in multiple banks, but > > rather placed continually into the same register block. This affects > > how register offsets are computed. > > - Pinmux nodes can explicitly select GPIO mode, whereas, in the npcm7xx > > driver, this happens automatically when a GPIO is requested. > > > > Some functionality implemented in the hardware was (for now) left unused > > in the driver, specifically blinking and pull-up/down. > > Overall looks good. Some cosmetic stuff is required, but there are no > show stoppers. Good to hear! > > Signed-off-by: Jonathan Neuschäfer > > --- > > > > This patch now depends on gpio/for-next, specifically these patches: > > - gpiolib: improve coding style for local variables > > - gpiolib: allow to specify the firmware node in struct gpio_chip > > - gpiolib: of: make fwnode take precedence in struct gpio_chip [...] > > +/* GCR registers */ > > +#define WPCM450_GCR_MFSEL1 0x0C > > Be consistent with capitalization Oops, will fix. > > +struct wpcm450_bank { > > + /* 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; > > + > > + /* Interrupt bit mapping */ > > + u8 first_irq_bit; > > + u8 num_irqs; > > + u8 first_irq_gpio; > > These three are a bit undocumented. I'll add descriptions. > > +static const struct wpcm450_bank wpcm450_banks[WPCM450_NUM_BANKS] = { > > + /* range cfg0 cfg1 cfg2 blink out in IRQ map */ > > + { 0, 16, 0x14, 0x18, 0, 0, 0x1c, 0x20, 0, 16, 0 }, > > + { 16, 16, 0x24, 0x28, 0x2c, 0x30, 0x34, 0x38, 16, 2, 8 }, > > So, the first_irq_gpio is used only here and as far as I understood it > has only two IRQ capable GPIOs starting from offset 8 in this bank. > What I didn't get is the relation on all these three. And could you > confirm that hardware indeed doesn't support full range of IRQs (to me > these settings look weird a bit)? The GPIO controller indeed only has 18 interrupt-capable GPIOs, 16 in the first bank, and 2 in the second. The full mapping is as follows: IRQ* | int. bits** | GPIO bank | offsets ----------|--------------|------------|----------- 2 | 0-3 | 0 | 0-3 3 | 4-11 | 0 | 4-11 4 | 12-15 | 0 | 12-15 5 | 16-17 | 1 | 8-9 *) At the central interrupt controller **) In the GPIO controller's interrupt registers such as GPEVST The hardware is indeed a bit weird. > > + { 32, 16, 0x3c, 0x40, 0x44, 0, 0x48, 0x4c, 0, 0, 0 }, > > + { 48, 16, 0x50, 0x54, 0x58, 0, 0x5c, 0x60, 0, 0, 0 }, > > + { 64, 16, 0x64, 0x68, 0x6c, 0, 0x70, 0x74, 0, 0, 0 }, > > + { 80, 16, 0x78, 0x7c, 0x80, 0, 0x84, 0x88, 0, 0, 0 }, > > + { 96, 18, 0, 0, 0, 0, 0, 0x8c, 0, 0, 0 }, > > + { 114, 14, 0x90, 0x94, 0x98, 0, 0x9c, 0xa0, 0, 0, 0 }, > > +}; > > +static void wpcm450_gpio_irq_ack(struct irq_data *d) > > +{ > > + struct wpcm450_gpio *gpio = gpiochip_get_data(irq_data_get_irq_chip_data(d)); > > + struct wpcm450_pinctrl *pctrl = gpio->pctrl; > > > > + unsigned long flags; > > Is it in IRQ context or not? I think ->irq_ack should run in IRQ context, I'm less sure about the other irq_chip methods. Unfortunately, linux/irq.h doesn't document these details. To avoid confusing myself and introducing bugs, I think I'll stay with spin_lock_irqsave/spin_unlock_irqrestore. > > > + int bit; > > + > > + bit = wpcm450_gpio_irq_bitnum(gpio, d); > > + if (bit < 0) > > + return; > > + > > + spin_lock_irqsave(&pctrl->lock, flags); > > + iowrite32(BIT(bit), pctrl->gpio_base + WPCM450_GPEVST); > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > +} > > +/* > > + * Since the GPIO controller does not support dual-edge triggered interrupts > > + * (IRQ_TYPE_EDGE_BOTH), they are emulated using rising/falling edge triggered > > + * interrupts. wpcm450_gpio_fix_evpol sets the interrupt polarity for the > > + * specified emulated dual-edge triggered interrupts, so that the next edge can > > + * be detected. > > + */ > > +static void wpcm450_gpio_fix_evpol(struct wpcm450_gpio *gpio, unsigned long all) > > +{ > > + struct wpcm450_pinctrl *pctrl = gpio->pctrl; > > + unsigned long flags; > > + unsigned int bit; > > + > > + for_each_set_bit(bit, &all, 32) { > > + int offset = wpcm450_irq_bitnum_to_gpio(gpio, bit); > > + unsigned long evpol; > > + int level; > > + > > + spin_lock_irqsave(&gpio->gc.bgpio_lock, flags); > > + do { > > + evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL); > > > + level = gpio->gc.get(&gpio->gc, offset); > > I'm not sure why here and below you are using a method via GPIO chip. > Why can't you simply call a method directly? The ->get method is defined through bgpio_init, it's probably bgpio_get. The bgpio_* methods are private to gpio-mmio.c, so I can't call them directly. I could theoretically reimplement the functionality here, but I found it easier to call the existing ->get function. > > > + /* Switch event polarity to the opposite of the current level */ > > + __assign_bit(bit, &evpol, !level); > > + > > + iowrite32(evpol, pctrl->gpio_base + WPCM450_GPEVPOL); > > + } while (gpio->gc.get(&gpio->gc, offset) != level); > > + spin_unlock_irqrestore(&gpio->gc.bgpio_lock, flags); > > + } > > +} > > +static void wpcm450_gpio_irqhandler(struct irq_desc *desc) > > +{ > > + struct wpcm450_gpio *gpio = gpiochip_get_data(irq_desc_get_handler_data(desc)); > > + struct wpcm450_pinctrl *pctrl = gpio->pctrl; > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned long pending; > > + unsigned long flags; > > + unsigned long ours; > > + unsigned int bit; > > > + ours = GENMASK(gpio->bank->first_irq_bit + gpio->bank->num_irqs - 1, > > + gpio->bank->first_irq_bit); > > ours = GENMASK(gpio->bank->num_irqs - 1, 0) << gpio->bank->first_irq_bit; > > is better to read and understand. I think I commented on this. Fair; I'll change it. > > +static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin, > > + unsigned long *config) > > +{ > > + struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + enum pin_config_param param = pinconf_to_config_param(*config); > > + unsigned long flags; > > + int bit; > > + u32 reg; > > + > > + switch (param) { > > + case PIN_CONFIG_INPUT_DEBOUNCE: > > + bit = debounce_bitnum(pin); > > + if (bit < 0) > > + return bit; > > + > > + spin_lock_irqsave(&pctrl->lock, flags); > > + reg = ioread32(pctrl->gpio_base + WPCM450_GPEVDBNC); > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > + > > + *config = pinconf_to_config_packed(param, !!(reg & BIT(bit))); > > + break; > > + default: > > + return -ENOTSUPP; > > + } > > > + > > + return 0; > > Why not to return from the case? > Ditto for the rest. I'll change it. > > +static int wpcm450_gpio_register(struct platform_device *pdev, > > + struct wpcm450_pinctrl *pctrl) > > +{ > > + int ret = 0; > > Redundant assignment. Indeed, I'll fix it. > > > + struct fwnode_handle *child; > > + > > + pctrl->gpio_base = devm_platform_ioremap_resource(pdev, 0); > > + if (!pctrl->gpio_base) > > + return dev_err_probe(pctrl->dev, -ENOMEM, "Resource fail for GPIO controller\n"); > > + > > + device_for_each_child_node(pctrl->dev, child) { > > Please, be consistent with the device pointer you are using here and > there, see below as well. Will do as below. > > + gpio->gc.fwnode = child; > > Thanks, but this will make it a material for v5.18 (starting from). I > think since you send the v3 just at the holidays season followed by > release, it's an intention and we have a few weeks to handle this > series. I don't particularly care about one release or another, so 5.18 is soon enough for me. > > +static int wpcm450_pinctrl_probe(struct platform_device *pdev) > > +{ > > + struct wpcm450_pinctrl *pctrl; > > + int ret; > > + > > + pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL); > > + if (!pctrl) > > + return -ENOMEM; > > + > > + pctrl->dev = &pdev->dev; > > + spin_lock_init(&pctrl->lock); > > + dev_set_drvdata(&pdev->dev, pctrl); > > + > > + pctrl->gcr_regmap = > > + syscon_regmap_lookup_by_compatible("nuvoton,wpcm450-gcr"); > > + if (IS_ERR(pctrl->gcr_regmap)) > > + return dev_err_probe(pctrl->dev, PTR_ERR(pctrl->gcr_regmap), > > Please, use the original device pointer in the ->probe(). > Sometimes it's good to have > > struct device *dev = &pdev->dev; Will do. Thanks, Jonathan Neuschäfer