Hi Linus, Thanks a lot for your comments.
Sorry for my late reply, I was on vacation. The last days I have been working to move NPCM pinctrl GPIO to GENERIC GPIO, most of the work have been done but I had the some issue. I initialize bgpio as follow: ret = bgpio_init(&pctrl->gpio_bank[id].gc, pctrl->dev, 4, pctrl->gpio_bank[id].base + NPCM7XX_GP_N_DIN, pctrl->gpio_bank[id].base + NPCM7XX_GP_N_DOUT, NULL, NULL, pctrl->gpio_bank[id].base + NPCM7XX_GP_N_IEM, BGPIOF_READ_OUTPUT_REG_SET); After doing it, the directions functions I used are: bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv and the I/O get function is bgpio_get_set By using inv directions: direction out = 0 (gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio)) direction in = 1 (gc->bgpio_dir |= bgpio_line2mask(gc, gpio)) The problem occur when reading the GPIO value from bgpio_get_set function, because the directions value are inverse it reading the wrong I/O registers For direction out it reading dat register (instead of set register) For direction in it calling set register (instead of dat register) if (gc->bgpio_dir & pinmask) return !!(gc->read_reg(gc->reg_set) & pinmask); else return !!(gc->read_reg(gc->reg_dat) & pinmask); the same issue occur at bgpio_get_set_multiple function. Maybe in bgpio_dir parameter direction out should be in both cases 1 and direction in = 0. for now i did a local fix in the npcm pinctrl driver by setting bgpio_dir parameters as direction out = 1 and direction in = 0. Thanks a lot, Tomer On 13 July 2018 at 11:51, Linus Walleij <linus.wall...@linaro.org> wrote: > On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon <tmaimo...@gmail.com> wrote: > > > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and > > GPIO controller driver. > > > > Signed-off-by: Tomer Maimon <tmaimo...@gmail.com> > > (...) > > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c > > @@ -0,0 +1,2089 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (c) 2016-2018 Nuvoton Technology corporation. > > +// Copyright (c) 2016, Dell Inc > > + > > +#include <linux/device.h> > > +#include <linux/gpio.h> > > As this is a driver you should only include <linux/gpio/driver.h> > > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/mfd/syscon.h> > > If you need syscon then the driver should select or depend > on MFD_SYSCON in Kconfig. > > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > Do you really need this include? > > > +/* Structure for register banks */ > > +struct NPCM7XX_GPIO { > > Can we have this lowercase? Please? > > > + void __iomem *base; > > + struct gpio_chip gc; > > + int irqbase; > > + int irq; > > + spinlock_t lock; > > + void *priv; > > + struct irq_chip irq_chip; > > + u32 pinctrl_id; > > +}; > > So each GPIO bank has its own gpio chip and register > base, that is NICE! Because then it looks like you can > select GPIO_GENERIC and use the MMIO GPIO helper > library to handle the registers. Let's look into that > option! > > > +struct NPCM7xx_pinctrl { > > + struct pinctrl_dev *pctldev; > > + struct device *dev; > > + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM]; > > + struct irq_domain *domain; > > I wonder why the pin controller needs and IRQ domain but > I keep reading the code and I might find out... > > > +enum operand { > > + op_set, > > + op_getbit, > > + op_setbit, > > + op_clrbit, > > +}; > > This looks complicated. I suspect you can use GPIO_GENERIC > to set/get and clear bits in the register(s). > > > +/* Perform locked bit operations on GPIO registers */ > > +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int > offset, > > + int reg) > > +{ > > + unsigned long flags; > > + u32 mask, val; > > + > > + mask = (1L << offset); > > + spin_lock_irqsave(&bank->lock, flags); > > + switch (op) { > > + case op_set: > > + iowrite32(mask, bank->base + reg); > > + break; > > + case op_getbit: > > + mask &= ioread32(bank->base + reg); > > + break; > > + case op_setbit: > > + val = ioread32(bank->base + reg); > > + iowrite32(val | mask, bank->base + reg); > > + break; > > + case op_clrbit: > > + val = ioread32(bank->base + reg); > > + iowrite32(val & (~mask), bank->base + reg); > > + break; > > + } > > + spin_unlock_irqrestore(&bank->lock, flags); > > + return !!mask; > > +} > > This is essentially a reimplementation of drivers/gpio/gpio-mmio.c > (GPIO_GENERIC, also using a spinlock to protect the registers) > so let's use that instead :) > > There are drivers already that reuse the spinlock inside the > generic GPIO chip to protect their other registers like for > IRQ registers. > > > +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int > offset) > > +{ > > + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip); > > + u32 oe, ie; > > + > > + /* Get Input & Output state */ > > + ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM); > > + oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE); > > + if (ie && !oe) > > + return GPIOF_DIR_IN; > > + else if (oe && !ie) > > + return GPIOF_DIR_OUT; > > These are consumer flags and should not be used in drivers. > Use plain 0/1 instead. > > Anyways the problem goes away with GPIO_GENERIC. > > > +static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned > int offset) > > +{ > > + return pinctrl_gpio_direction_input(offset + chip->base); > > +} > > It's a bit tricksy to get this to work with GPIO_GENERIC. > > After calling bgpio_init() you need to overwrite the assigned > .direction_input handler with this and then direct back to the > one assigned by GPIO_GENERIC. > > Something like this: > > 1. Add two indirection pointers to the npcm7xx_gpio state container: > > struct npcm7xx_gpio { > (...) > int (*direction_input)(struct gpio_chip *chip, unsigned offset); > int (*direction_output)(struct gpio_chip *chip, unsigned offset, > int value); > (...) > }; > > 2. Save the pointers > > struct npcm7xx_gpio *npcm; > > bgpio_init( ... register setup ...) > npcm->direction_input = npcm->gc.direction_input; > npcm->direction_output = npcm->gc.direction_output; > npcm->gc.direction_input = npcmgpio_direction_input; > npcm->gc.direction_output = npcmgpio_direction_output; > > 3. Modify the functions like that: > > static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int > offset) > { > struct npcm7xx_gpio *npcm = gpiochip_get_data(chip); > int ret; > > ret = pinctrl_gpio_direction_input(offset + chip->base); > if (ret) > return ret; > return npcm->direction_input(chip); > } > > I'm sure you get the idea... if you think we can modify gpio-mmio > to be more helpful with this, suggestions are welcome! > > > +/* Set GPIO to Output with initial value */ > > +static int npcmgpio_direction_output(struct gpio_chip *chip, > > + unsigned int offset, int value) > > +{ > > + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip); > > + > > + dev_dbg(chip->parent, "gpio_direction_output: offset%d = %x\n", > offset, > > + value); > > + > > + /* Check if we're enabled as an interrupt.. */ > > + if (gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_EVEN) && > > + gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM)) { > > + dev_dbg(chip->parent, > > + "gpio_direction_output: IRQ enabled on > offset%d\n", > > + offset); > > + return -EINVAL; > > + } > > This should not be necessary as you are using GPIOLIB_IRQCHIP, > which locks the GPIO for interrupt and disallows this to happen. > > > +static int npcmgpio_gpio_request(struct gpio_chip *chip, unsigned int > offset) > > +{ > > + dev_dbg(chip->parent, "gpio_request: offset%d\n", offset); > > + return pinctrl_gpio_request(offset + chip->base); > > +} > > + > > +static void npcmgpio_gpio_free(struct gpio_chip *chip, unsigned int > offset) > > +{ > > + dev_dbg(chip->parent, "gpio_free: offset%d\n", offset); > > + pinctrl_gpio_free(offset + chip->base); > > +} > > This needs the same pattern as the direction functions above, then > you can use GPIO_GENERIC (mmio). > > > +static unsigned int npcmgpio_irq_startup(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + unsigned int gpio = d->hwirq; > > + > > + /* active-high, input, clear interrupt, enable interrupt */ > > + dev_dbg(d->chip->parent_device, "startup: %u.%u\n", gpio, > d->irq); > > + npcmgpio_direction_output(gc, gpio, 1); > > + npcmgpio_direction_input(gc, gpio); > > Interesting dance. So it is required to set the line to > 1 and then switch to input? > > > +static struct irq_chip npcmgpio_irqchip = { > > + .name = "NPCM7XX-GPIO-IRQ", > > + .irq_ack = npcmgpio_irq_ack, > > + .irq_unmask = npcmgpio_irq_unmask, > > + .irq_mask = npcmgpio_irq_mask, > > + .irq_set_type = npcmgpio_set_irq_type, > > + .irq_startup = npcmgpio_irq_startup, > > +}; > > This code is looking good BTW. > > The patch in my inbox just ends in the middle of everything, I wonder > why :( suspect the new gmail interface I'm using. > > Anyways: the pointers above should keep you busy for the next > iteration of the patch, the pin control part seems pretty straight-forward. > > Yours, > Linus Walleij >