Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus, On 30 July 2018 at 00:59, Linus Walleij wrote: > On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote: > > > I initialize bgpio as follow: > > > > ret = bgpio_init(>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); > > (...) > > 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) > > Hm I don't quite get it... sorry. Maybe if you show your fix and what > you expect to happen I can understand better? > Of course, in the last patch sent three days a go (V3 - https://patchwork.ozlabs.org/patch/949942/) I did as follow to workaround the issue: int (*get)(struct gpio_chip *chip, unsigned offset); int (*get_multiple)(struct gpio_chip *chip, unsigned long *mask, unsigned long *bits); .. static int npcmgpio_get_value(struct gpio_chip *chip, unsigned int offset) { struct npcm7xx_gpio *bank = gpiochip_get_data(chip); unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir; int val; /* * sets bgpio_dir parameter value to the opposite value * for calling the right registers in bgpio_get_set * function */ * bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val = bank->get(chip, offset); bank->gc.bgpio_dir = tmp_bgpio_dir;* return val; } static int npcmgpio_get_multiple_value(struct gpio_chip *chip, unsigned long *mask, unsigned long *bits) { struct npcm7xx_gpio *bank = gpiochip_get_data(chip); unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir; int val; /* * sets bgpio_dir parameter value to the opposite value * for calling the right registers in bgpio_get_set_multiple * function */ * bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val = bank->get_multiple(chip, mask, bits); bank->gc.bgpio_dir = tmp_bgpio_dir;* return val; } .. pctrl->gpio_bank[id].get = pctrl->gpio_bank[id].gc.get; pctrl->gpio_bank[id].gc.get = npcmgpio_get_value; pctrl->gpio_bank[id].get_multiple = ptrl->gpio_bank[id].gc.get_mul tiple; pctrl->gpio_bank[id].gc.get_multiple = npcmgpio_get_multiple_value; but it is not that good solution, because the bold commands are not atomic (locked) operations. > > Do you mean that because you write the inverse value to > IEM this happens, and the BGPIO code assumes that > you always write 1 to set a line as input and 0 to set it > as output? > yes, because of it the bgpio_get_set and bgpio_get_set_multiple setting the opposite data registers . > > I would say if this causes the problem we should just add > a new BGPIOF_INVERTED_REG_DIR with comment in > include/linux/gpio/driver.h and make the necessary fix to > respect this flag in the gpio-mmio.c core so it works right. > > If you do this as a separate patch I would be grateful :) > Sure, I will send a separate patch later on to overcome it. > > Yours, > Linus Walleij > Thanks, Tomer
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus, On 30 July 2018 at 00:59, Linus Walleij wrote: > On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote: > > > I initialize bgpio as follow: > > > > ret = bgpio_init(>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); > > (...) > > 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) > > Hm I don't quite get it... sorry. Maybe if you show your fix and what > you expect to happen I can understand better? > Of course, in the last patch sent three days a go (V3 - https://patchwork.ozlabs.org/patch/949942/) I did as follow to workaround the issue: int (*get)(struct gpio_chip *chip, unsigned offset); int (*get_multiple)(struct gpio_chip *chip, unsigned long *mask, unsigned long *bits); .. static int npcmgpio_get_value(struct gpio_chip *chip, unsigned int offset) { struct npcm7xx_gpio *bank = gpiochip_get_data(chip); unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir; int val; /* * sets bgpio_dir parameter value to the opposite value * for calling the right registers in bgpio_get_set * function */ * bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val = bank->get(chip, offset); bank->gc.bgpio_dir = tmp_bgpio_dir;* return val; } static int npcmgpio_get_multiple_value(struct gpio_chip *chip, unsigned long *mask, unsigned long *bits) { struct npcm7xx_gpio *bank = gpiochip_get_data(chip); unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir; int val; /* * sets bgpio_dir parameter value to the opposite value * for calling the right registers in bgpio_get_set_multiple * function */ * bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val = bank->get_multiple(chip, mask, bits); bank->gc.bgpio_dir = tmp_bgpio_dir;* return val; } .. pctrl->gpio_bank[id].get = pctrl->gpio_bank[id].gc.get; pctrl->gpio_bank[id].gc.get = npcmgpio_get_value; pctrl->gpio_bank[id].get_multiple = ptrl->gpio_bank[id].gc.get_mul tiple; pctrl->gpio_bank[id].gc.get_multiple = npcmgpio_get_multiple_value; but it is not that good solution, because the bold commands are not atomic (locked) operations. > > Do you mean that because you write the inverse value to > IEM this happens, and the BGPIO code assumes that > you always write 1 to set a line as input and 0 to set it > as output? > yes, because of it the bgpio_get_set and bgpio_get_set_multiple setting the opposite data registers . > > I would say if this causes the problem we should just add > a new BGPIOF_INVERTED_REG_DIR with comment in > include/linux/gpio/driver.h and make the necessary fix to > respect this flag in the gpio-mmio.c core so it works right. > > If you do this as a separate patch I would be grateful :) > Sure, I will send a separate patch later on to overcome it. > > Yours, > Linus Walleij > Thanks, Tomer
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote: > I initialize bgpio as follow: > > ret = bgpio_init(>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); (...) > 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) Hm I don't quite get it... sorry. Maybe if you show your fix and what you expect to happen I can understand better? Do you mean that because you write the inverse value to IEM this happens, and the BGPIO code assumes that you always write 1 to set a line as input and 0 to set it as output? I would say if this causes the problem we should just add a new BGPIOF_INVERTED_REG_DIR with comment in include/linux/gpio/driver.h and make the necessary fix to respect this flag in the gpio-mmio.c core so it works right. If you do this as a separate patch I would be grateful :) Yours, Linus Walleij
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote: > I initialize bgpio as follow: > > ret = bgpio_init(>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); (...) > 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) Hm I don't quite get it... sorry. Maybe if you show your fix and what you expect to happen I can understand better? Do you mean that because you write the inverse value to IEM this happens, and the BGPIO code assumes that you always write 1 to set a line as input and 0 to set it as output? I would say if this causes the problem we should just add a new BGPIOF_INVERTED_REG_DIR with comment in include/linux/gpio/driver.h and make the necessary fix to respect this flag in the gpio-mmio.c core so it works right. If you do this as a separate patch I would be grateful :) Yours, Linus Walleij
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
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(>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 wrote: > On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote: > > > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and > > GPIO controller driver. > > > > Signed-off-by: Tomer Maimon > > (...) > > +++ 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 > > +#include > > As this is a driver you should only include > > > +#include > > +#include > > +#include > > If you need syscon then the driver should select or depend > on MFD_SYSCON in Kconfig. > > > +#include > > +#include > > +#include > > +#include > > Do you really need this include? > > > +/* Structure for register banks */ > > +struct NPCM7XX_GPIO { > > Can we have this lowercase? Please? > > > + void __iomem*base; > > + struct gpio_chipgc; > > + 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(>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; > > + } > > +
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
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(>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 wrote: > On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote: > > > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and > > GPIO controller driver. > > > > Signed-off-by: Tomer Maimon > > (...) > > +++ 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 > > +#include > > As this is a driver you should only include > > > +#include > > +#include > > +#include > > If you need syscon then the driver should select or depend > on MFD_SYSCON in Kconfig. > > > +#include > > +#include > > +#include > > +#include > > Do you really need this include? > > > +/* Structure for register banks */ > > +struct NPCM7XX_GPIO { > > Can we have this lowercase? Please? > > > + void __iomem*base; > > + struct gpio_chipgc; > > + 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(>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; > > + } > > +
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote: > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and > GPIO controller driver. > > Signed-off-by: Tomer Maimon (...) > +++ 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 > +#include As this is a driver you should only include > +#include > +#include > +#include If you need syscon then the driver should select or depend on MFD_SYSCON in Kconfig. > +#include > +#include > +#include > +#include Do you really need this include? > +/* Structure for register banks */ > +struct NPCM7XX_GPIO { Can we have this lowercase? Please? > + void __iomem*base; > + struct gpio_chipgc; > + 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(>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(>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)
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote: > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and > GPIO controller driver. > > Signed-off-by: Tomer Maimon (...) > +++ 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 > +#include As this is a driver you should only include > +#include > +#include > +#include If you need syscon then the driver should select or depend on MFD_SYSCON in Kconfig. > +#include > +#include > +#include > +#include Do you really need this include? > +/* Structure for register banks */ > +struct NPCM7XX_GPIO { Can we have this lowercase? Please? > + void __iomem*base; > + struct gpio_chipgc; > + 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(>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(>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)
[PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Add Nuvoton BMC NPCM750/730/715/705 Pinmux and GPIO controller driver. Signed-off-by: Tomer Maimon --- drivers/pinctrl/Kconfig |1 + drivers/pinctrl/Makefile |1 + drivers/pinctrl/nuvoton/Kconfig | 12 + drivers/pinctrl/nuvoton/Makefile |4 + drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c | 2089 + 5 files changed, 2107 insertions(+) create mode 100644 drivers/pinctrl/nuvoton/Kconfig create mode 100644 drivers/pinctrl/nuvoton/Makefile create mode 100644 drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index dd50371225bc..c966d94c2320 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -345,6 +345,7 @@ source "drivers/pinctrl/freescale/Kconfig" source "drivers/pinctrl/intel/Kconfig" source "drivers/pinctrl/mvebu/Kconfig" source "drivers/pinctrl/nomadik/Kconfig" +source "drivers/pinctrl/nuvoton/Kconfig" source "drivers/pinctrl/pxa/Kconfig" source "drivers/pinctrl/qcom/Kconfig" source "drivers/pinctrl/samsung/Kconfig" diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index de40863e7297..1797c09c8981 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -51,6 +51,7 @@ obj-y += freescale/ obj-$(CONFIG_X86) += intel/ obj-y += mvebu/ obj-y += nomadik/ +obj-$(CONFIG_ARCH_NPCM7XX) += nuvoton/ obj-$(CONFIG_PINCTRL_PXA) += pxa/ obj-$(CONFIG_ARCH_QCOM)+= qcom/ obj-$(CONFIG_PINCTRL_SAMSUNG) += samsung/ diff --git a/drivers/pinctrl/nuvoton/Kconfig b/drivers/pinctrl/nuvoton/Kconfig new file mode 100644 index ..70542c16b656 --- /dev/null +++ b/drivers/pinctrl/nuvoton/Kconfig @@ -0,0 +1,12 @@ +config PINCTRL_NPCM7XX + bool "Pinctrl and GPIO driver for Nuvoton NPCM7XX" + depends on (ARCH_NPCM7XX || COMPILE_TEST) && OF + select PINMUX + select PINCONF + select GENERIC_PINCONF + select GPIOLIB + select GPIOLIB_IRQCHIP + help + Say Y here to enable pin controller and GPIO support + for Nuvoton NPCM750/730/715/705 SoCs. + diff --git a/drivers/pinctrl/nuvoton/Makefile b/drivers/pinctrl/nuvoton/Makefile new file mode 100644 index ..886d00784cef --- /dev/null +++ b/drivers/pinctrl/nuvoton/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +# Nuvoton pinctrl support + +obj-$(CONFIG_PINCTRL_NPCM7XX) += pinctrl-npcm7xx.o diff --git a/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c new file mode 100644 index ..f29af0dc1643 --- /dev/null +++ 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* GCR registers */ +#define NPCM7XX_GCR_PDID 0x00 +#define NPCM7XX_GCR_MFSEL1 0x0C +#define NPCM7XX_GCR_MFSEL2 0x10 +#define NPCM7XX_GCR_MFSEL3 0x64 +#define NPCM7XX_GCR_MFSEL4 0xb0 +#define NPCM7XX_GCR_CPCTL 0xD0 +#define NPCM7XX_GCR_CP2BST 0xD4 +#define NPCM7XX_GCR_B2CPNT 0xD8 +#define NPCM7XX_GCR_I2CSEGSEL 0xE0 +#define NPCM7XX_GCR_I2CSEGCTL 0xE4 +#define NPCM7XX_GCR_SRCNT 0x68 +#define NPCM7XX_GCR_FLOCKR10x74 +#define NPCM7XX_GCR_DSCNT 0x78 + +#define SRCNT_ESPI BIT(3) + +/* GPIO registers */ +#define NPCM7XX_GP_N_TLOCK10x00 +#define NPCM7XX_GP_N_DIN 0x04 /* Data IN */ +#define NPCM7XX_GP_N_POL 0x08 /* Polarity */ +#define NPCM7XX_GP_N_DOUT 0x0c /* Data OUT */ +#define NPCM7XX_GP_N_OE0x10 /* Output Enable */ +#define NPCM7XX_GP_N_OTYP 0x14 +#define NPCM7XX_GP_N_MP0x18 +#define NPCM7XX_GP_N_PU0x1c /* Pull-up */ +#define NPCM7XX_GP_N_PD0x20 /* Pull-down */ +#define NPCM7XX_GP_N_DBNC 0x24 /* Debounce */ +#define NPCM7XX_GP_N_EVTYP 0x28 /* Event Type */ +#define NPCM7XX_GP_N_EVBE 0x2c /* Event Both Edge */ +#define NPCM7XX_GP_N_OBL0 0x30 +#define NPCM7XX_GP_N_OBL1 0x34 +#define NPCM7XX_GP_N_OBL2 0x38 +#define NPCM7XX_GP_N_OBL3 0x3c +#define NPCM7XX_GP_N_EVEN 0x40 /* Event Enable */ +#define NPCM7XX_GP_N_EVENS 0x44 /* Event Set (enable) */ +#define NPCM7XX_GP_N_EVENC 0x48 /* Event Clear (disable) */ +#define NPCM7XX_GP_N_EVST 0x4c /* Event Status */ +#define NPCM7XX_GP_N_SPLCK 0x50 +#define NPCM7XX_GP_N_MPLCK 0x54 +#define NPCM7XX_GP_N_IEM 0x58 /* Input Enable */ +#define NPCM7XX_GP_N_OSRC 0x5c +#define NPCM7XX_GP_N_ODSC 0x60 +#define NPCM7XX_GP_N_DOS 0x68 /* Data OUT Set */ +#define NPCM7XX_GP_N_DOC 0x6c /* Data OUT Clear
[PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Add Nuvoton BMC NPCM750/730/715/705 Pinmux and GPIO controller driver. Signed-off-by: Tomer Maimon --- drivers/pinctrl/Kconfig |1 + drivers/pinctrl/Makefile |1 + drivers/pinctrl/nuvoton/Kconfig | 12 + drivers/pinctrl/nuvoton/Makefile |4 + drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c | 2089 + 5 files changed, 2107 insertions(+) create mode 100644 drivers/pinctrl/nuvoton/Kconfig create mode 100644 drivers/pinctrl/nuvoton/Makefile create mode 100644 drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index dd50371225bc..c966d94c2320 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -345,6 +345,7 @@ source "drivers/pinctrl/freescale/Kconfig" source "drivers/pinctrl/intel/Kconfig" source "drivers/pinctrl/mvebu/Kconfig" source "drivers/pinctrl/nomadik/Kconfig" +source "drivers/pinctrl/nuvoton/Kconfig" source "drivers/pinctrl/pxa/Kconfig" source "drivers/pinctrl/qcom/Kconfig" source "drivers/pinctrl/samsung/Kconfig" diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index de40863e7297..1797c09c8981 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -51,6 +51,7 @@ obj-y += freescale/ obj-$(CONFIG_X86) += intel/ obj-y += mvebu/ obj-y += nomadik/ +obj-$(CONFIG_ARCH_NPCM7XX) += nuvoton/ obj-$(CONFIG_PINCTRL_PXA) += pxa/ obj-$(CONFIG_ARCH_QCOM)+= qcom/ obj-$(CONFIG_PINCTRL_SAMSUNG) += samsung/ diff --git a/drivers/pinctrl/nuvoton/Kconfig b/drivers/pinctrl/nuvoton/Kconfig new file mode 100644 index ..70542c16b656 --- /dev/null +++ b/drivers/pinctrl/nuvoton/Kconfig @@ -0,0 +1,12 @@ +config PINCTRL_NPCM7XX + bool "Pinctrl and GPIO driver for Nuvoton NPCM7XX" + depends on (ARCH_NPCM7XX || COMPILE_TEST) && OF + select PINMUX + select PINCONF + select GENERIC_PINCONF + select GPIOLIB + select GPIOLIB_IRQCHIP + help + Say Y here to enable pin controller and GPIO support + for Nuvoton NPCM750/730/715/705 SoCs. + diff --git a/drivers/pinctrl/nuvoton/Makefile b/drivers/pinctrl/nuvoton/Makefile new file mode 100644 index ..886d00784cef --- /dev/null +++ b/drivers/pinctrl/nuvoton/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +# Nuvoton pinctrl support + +obj-$(CONFIG_PINCTRL_NPCM7XX) += pinctrl-npcm7xx.o diff --git a/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c new file mode 100644 index ..f29af0dc1643 --- /dev/null +++ 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* GCR registers */ +#define NPCM7XX_GCR_PDID 0x00 +#define NPCM7XX_GCR_MFSEL1 0x0C +#define NPCM7XX_GCR_MFSEL2 0x10 +#define NPCM7XX_GCR_MFSEL3 0x64 +#define NPCM7XX_GCR_MFSEL4 0xb0 +#define NPCM7XX_GCR_CPCTL 0xD0 +#define NPCM7XX_GCR_CP2BST 0xD4 +#define NPCM7XX_GCR_B2CPNT 0xD8 +#define NPCM7XX_GCR_I2CSEGSEL 0xE0 +#define NPCM7XX_GCR_I2CSEGCTL 0xE4 +#define NPCM7XX_GCR_SRCNT 0x68 +#define NPCM7XX_GCR_FLOCKR10x74 +#define NPCM7XX_GCR_DSCNT 0x78 + +#define SRCNT_ESPI BIT(3) + +/* GPIO registers */ +#define NPCM7XX_GP_N_TLOCK10x00 +#define NPCM7XX_GP_N_DIN 0x04 /* Data IN */ +#define NPCM7XX_GP_N_POL 0x08 /* Polarity */ +#define NPCM7XX_GP_N_DOUT 0x0c /* Data OUT */ +#define NPCM7XX_GP_N_OE0x10 /* Output Enable */ +#define NPCM7XX_GP_N_OTYP 0x14 +#define NPCM7XX_GP_N_MP0x18 +#define NPCM7XX_GP_N_PU0x1c /* Pull-up */ +#define NPCM7XX_GP_N_PD0x20 /* Pull-down */ +#define NPCM7XX_GP_N_DBNC 0x24 /* Debounce */ +#define NPCM7XX_GP_N_EVTYP 0x28 /* Event Type */ +#define NPCM7XX_GP_N_EVBE 0x2c /* Event Both Edge */ +#define NPCM7XX_GP_N_OBL0 0x30 +#define NPCM7XX_GP_N_OBL1 0x34 +#define NPCM7XX_GP_N_OBL2 0x38 +#define NPCM7XX_GP_N_OBL3 0x3c +#define NPCM7XX_GP_N_EVEN 0x40 /* Event Enable */ +#define NPCM7XX_GP_N_EVENS 0x44 /* Event Set (enable) */ +#define NPCM7XX_GP_N_EVENC 0x48 /* Event Clear (disable) */ +#define NPCM7XX_GP_N_EVST 0x4c /* Event Status */ +#define NPCM7XX_GP_N_SPLCK 0x50 +#define NPCM7XX_GP_N_MPLCK 0x54 +#define NPCM7XX_GP_N_IEM 0x58 /* Input Enable */ +#define NPCM7XX_GP_N_OSRC 0x5c +#define NPCM7XX_GP_N_ODSC 0x60 +#define NPCM7XX_GP_N_DOS 0x68 /* Data OUT Set */ +#define NPCM7XX_GP_N_DOC 0x6c /* Data OUT Clear