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_setBy 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 Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:
> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> GPIO controller driver.
>
> Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
(...)
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c As this is a driver you should only include <linux/gpio/driver.h>
> @@ -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>
> +#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]; I wonder why the pin controller needs and IRQ domain but
> +   Âstruct irq_domain   Â*domain;
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).
This is essentially a reimplementation of drivers/gpio/gpio-mmio.c
> +/* 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;
> +}
(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, This should not be necessary as you are using GPIOLIB_IRQCHIP,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 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;
> +Â Â Â Â}
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); Interesting dance. So it is required to set the line to
> +Â Â Â Ânpcmgpio_direction_output(gc, gpio, 1);
> +Â Â Â Ânpcmgpio_direction_input(gc, gpio);
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