Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp1365094imm; Wed, 25 Jul 2018 17:02:56 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfVrG3xFdsvlmjKUVCy6LjuDS6UQ0oCBvBUE8JDX62qKKcKVDS23VtrFf2CIaoZrK4C3nUL X-Received: by 2002:a63:5d09:: with SMTP id r9-v6mr22265021pgb.303.1532563376305; Wed, 25 Jul 2018 17:02:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532563376; cv=none; d=google.com; s=arc-20160816; b=qWOSTHJ2AzQ2ex2Bp38YAtWfcHYuLBiSDF/gw+8YnXb41dp1MU7TG7RQcAyzeFBXyq JuPKTW7DHZV7gyP4hOdLEydwJ2gts75LbGap17Hl5xUJAqnBDinLOedJ9bDlVyC22ntu 9I/KsX9iTZzYI+xny8xQOIIou97el6ZlghV8rtliqsgxewsooVWcO6Gd3j6B9Ffwabcc fAYRpYvtyliSMMnjt764TP9W/zbCBeoo1K6mI3eKyNgc9597eYLwuxnGOWvc2oG1A6x0 FOZxEtviwUaYhbDtUYXqDmgJ+5dQ81H8D/+D/MbSIsNfrnoWxAc+lREweLBYsW/igzpY fEmg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature :arc-authentication-results; bh=U+uqGBPKylBV225zit3wMmkR3cNLVNQrulxG+1UFoMI=; b=pK7bMMRmSynSwNb/E22BEDei6dK5k+40eeX7y/HJD9Iods0O5oGp89/EaZq6mCjI90 w0OzFUkp/NI7ALOa/ByUIEJ7av0pqpY98Bmk8te4/gVylpZdYNdWMrEmPhr84yS1pYON M5NvX9NgRtSUfIOBryOORLS+QrHXyrEpHh5X8KugyBD3GHgr9fL7YGMk9fSnjBXQfDeN OZ6WboKIGUAYXSVERM07lfVsOjhXRzDtow0uVwcFnbL9tsTjv7/egnMvlCBT5FVydkwG /NP1lwDij2i3Zu6jKpW01TzCE1CrBXQ4wXVzv9/wK+drXZChNHCN9dGn4st9Hv/VmrCO B9GQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=eCgjRoNr; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id m24-v6si15568805pfk.56.2018.07.25.17.02.41; Wed, 25 Jul 2018 17:02:56 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=eCgjRoNr; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728641AbeGZBP7 (ORCPT + 99 others); Wed, 25 Jul 2018 21:15:59 -0400 Received: from mail-io0-f196.google.com ([209.85.223.196]:42055 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728485AbeGZBP6 (ORCPT ); Wed, 25 Jul 2018 21:15:58 -0400 Received: by mail-io0-f196.google.com with SMTP id g11-v6so7803757ioq.9; Wed, 25 Jul 2018 17:01:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=U+uqGBPKylBV225zit3wMmkR3cNLVNQrulxG+1UFoMI=; b=eCgjRoNrRC3Nfs/2JiRDOSPAH79RRx2FePAOTgeiAHohJ0FfOhd9jVk3XUafoHgiKZ H0ebVyHqNxMQdsfm6wBtTZxY9kI94878P10DgSW62dNL5nDaintojqnvAfRzZZz138Km AVvsju7A6R+GILtVklXSPkQ+bk86iCjqi9bpVTVUGLyJY+SnecDtdHBBa/cPpaUC7t4h 8rQBHcHbADiNzWgR/6/Ta50wMI1rB8Ve2DsVE3EkToD4qnZ4yDLOWPj3XUZrBRRepqb8 B+T6ZviSP+bAW3cmSIKYqre1tCexJOUVucFLqnE1G9rVj/V44mJqNrDgfsrKNHWP0SjP 33+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=U+uqGBPKylBV225zit3wMmkR3cNLVNQrulxG+1UFoMI=; b=d7Wga7DJySgHnWAdfbJBuxTthrBX2H4YgS75LapnRNjwODvG6aP5VguA3oRVqadIf8 1KD6FkOXvTT+T/uYTGBDzD7Ahf4UR/j5feVZhEHy9JQP5wCa1s50XX070srQVcYwS9fF 6wAmyWqSdEyKal6L0/IOFcLMPAoVXD8V5P/6EthqImsoBLoMsj/E7cXdxuR09QG61KXk xJYBH38nlaA2YMwI9jKpC9Ql6KGb0rkdby0kCKHGZ/fmT6voTQoxHoFDOYoUC/+/JqsW P6ZQfmo+BW9wDB5KMuwHu9iSyV5daIIt1ZObVfLkd3IJ81WkMUgVfjIRH6i5O27haJhs 36QA== X-Gm-Message-State: AOUpUlGYl+sI6X6HoF5ga7Tau3wPkSp+RN1OXwAfTCrLuI0Uszl0BSOa jDYc/s6ARxEBtcbQq2sffLKbAGyfAVbs8bOKLIM= X-Received: by 2002:a6b:278b:: with SMTP id n133-v6mr18434955ion.207.1532563313012; Wed, 25 Jul 2018 17:01:53 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:8506:0:0:0:0:0 with HTTP; Wed, 25 Jul 2018 17:01:52 -0700 (PDT) In-Reply-To: References: <20180712214203.114844-1-tmaimon77@gmail.com> <20180712214203.114844-3-tmaimon77@gmail.com> From: Tomer Maimon Date: Thu, 26 Jul 2018 03:01:52 +0300 Message-ID: Subject: Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver To: Linus Walleij Cc: Rob Herring , Mark Rutland , Avi Fishman , Nancy Yuen , Brendan Higgins , Patrick Venture , Joel Stanley , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "linux-kernel@vger.kernel.org" , "open list:GPIO SUBSYSTEM" , OpenBMC Maillist Content-Type: multipart/alternative; boundary="000000000000de351b0571dbb316" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --000000000000de351b0571dbb316 Content-Type: text/plain; charset="UTF-8" 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 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_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 > --000000000000de351b0571dbb316 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Linus,

Thanks a lot for your comment= s.

Sorry for my late reply, I was on vacation.

The last days I have been working to move NPCM pinctr= l GPIO to GENERIC GPIO, most of the work have been done but I had the some = issue.

I initialize bgpio as follow:
                      =
  ret =3D bgpio_init(&pctrl-&=
gt;gpio_bank[id].gc,
                                         pctrl->dev, 4,
                                         pctrl->gpio_bank[id<=
span style=3D"color:rgb(128,0,0)">].base +
                                         NPCM7XX_GP_N_DIN,
                                         pctrl->gpio_bank[id<=
span style=3D"color:rgb(128,0,0)">].base +
                                         NPCM7XX_GP_N_DOUT,
                                         NULL,
                                         NULL,
                                         pctrl->gpio_bank[id<=
span style=3D"color:rgb(128,0,0)">].base +
                                         NPCM7XX_GP_N_IEM,
                                         BGPIOF_READ_OUTPUT_REG_SET);

After doing it, the dir=
ections functions I used are: bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_ge=
t_dir_inv 
and the I/O get function is bgpio_get=
_set 
By using inv =
directions:
direction out =3D 0 (gc->bgpio_dir &=3D =
~bgpio_line2mask(gc, gpio))
direction in =3D 1 (gc->bgpio_d=
ir |=3D bgpio_line2mask(gc, gpio))
The problem occur when reading the GPIO value from bgpio_get_s=
et 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 f=
unction.
Maybe=
 in bgpio_dir parameter direction o=
ut should be in both cases 1 and direction in =3D 0.
for now i did a local fix in the npc=
m pinctrl driver by setting bgpio_dir parameters as direction out =3D 1 and direction in =3D 0.
Thanks a lot,
Tomer

On 13 July 2018 at 11:51, Linus Walleij <= ;linus.wallei= j@linaro.org> wrote:
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon <tmaimon77@gmail.com> wrote:

> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> GPIO controller driver.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

(...)
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c<= br> > @@ -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&= gt;

> +#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?

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0void __iomem=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 *base;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct gpio_chip=C2=A0 =C2=A0 =C2=A0 =C2= =A0 gc;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0irqbase;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0irq;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0spinlock_t=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 lock;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0void=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *priv;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct irq_chip=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0irq_chip;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0u32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pinctrl_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 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct pinctrl_dev=C2=A0 =C2=A0 =C2=A0 *pc= tldev;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct device=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0*dev;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct NPCM7XX_GPIO=C2=A0 =C2=A0 =C2=A0gpi= o_bank[NPCM7XX_GPIO_BANK_NUM];
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct irq_domain=C2=A0 =C2=A0 =C2=A0 =C2= =A0*domain;

I wonder why the pin controller needs and IRQ domain but
I keep reading the code and I might find out...

> +enum operand {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0op_set,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0op_getbit,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0op_setbit,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0op_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,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0int reg)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long flags;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 mask, val;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0mask =3D (1L << offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock_irqsave(&bank->lock, flag= s);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0switch (op) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0case op_set:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0iowrite32(mask= , bank->base + reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0case op_getbit:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mask &=3D = ioread32(bank->base + reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0case op_setbit:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0val =3D ioread= 32(bank->base + reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0iowrite32(val = | mask, bank->base + reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0case op_clrbit:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0val =3D ioread= 32(bank->base + reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0iowrite32(val = & (~mask), bank->base + reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock_irqrestore(&bank->= lock, flags);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0return !!mask;
> +}

This is essentially a reimplementation of drivers/gpio/gpio-mmi= o.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 in= t offset)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct NPCM7XX_GPIO *bank =3D gpiochip_get= _data(chip);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 oe, ie;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Get Input & Output state */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0ie =3D gpio_bitop(bank, op_getbit, offset,= NPCM7XX_GP_N_IEM);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0oe =3D gpio_bitop(bank, op_getbit, offset,= NPCM7XX_GP_N_OE);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ie && !oe)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return GPIOF_D= IR_IN;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0else if (oe && !ie)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return GPIOF_D= IR_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, unsi= gned int offset)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0return 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 {
=C2=A0 =C2=A0 =C2=A0(...)
=C2=A0 =C2=A0 =C2=A0 int (*direction_input)(struct gpio_chip *chip, unsigne= d offset);
=C2=A0 =C2=A0 =C2=A0 int (*direction_output)(struct gpio_chip *chip, unsign= ed offset,
int value);
=C2=A0 =C2=A0 =C2=A0(...)
};

2. Save the pointers

struct npcm7xx_gpio *npcm;

bgpio_init( ... register setup ...)
npcm->direction_input =3D npcm->gc.direction_input;
npcm->direction_output =3D npcm->gc.direction_output;
npcm->gc.direction_input =3D npcmgpio_direction_input;
npcm->gc.direction_output =3D npcmgpio_direction_output;

3. Modify the functions like that:

static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned i= nt offset)
{
=C2=A0 =C2=A0 struct npcm7xx_gpio *npcm =3D gpiochip_get_data(chip);
=C2=A0 =C2=A0 int ret;

=C2=A0 =C2=A0 ret =3D pinctrl_gpio_direction_input(offset + chip->b= ase);
=C2=A0 =C2=A0 if (ret)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
=C2=A0 =C2=A0 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,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned int offse= t, int value)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct NPCM7XX_GPIO *bank =3D gpiochip_get= _data(chip);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0dev_dbg(chip->parent, "gpio_direct= ion_output: offset%d =3D %x\n", offset,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0value);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Check if we're enabled as an interr= upt.. */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (gpio_bitop(bank, op_getbit, offset, NP= CM7XX_GP_N_EVEN) &&
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpio_bitop(bank, op_getbit, = offset, NPCM7XX_GP_N_IEM)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_dbg(chip-&= gt;parent,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0"gpio_direction_output: IRQ enabled on offset%d\n",=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0}

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)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0dev_dbg(chip->parent, "gpio_reques= t: offset%d\n", offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0return pinctrl_gpio_request(offset + chip-= >base);
> +}
> +
> +static void npcmgpio_gpio_free(struct gpio_chip *chip, unsigned int o= ffset)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0dev_dbg(chip->parent, "gpio_free: = offset%d\n", offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0pinctrl_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)
> +{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct gpio_chip *gc =3D irq_data_get_irq_= chip_data(d);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int gpio =3D d->hwirq;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* active-high, input, clear interrupt, en= able interrupt */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0dev_dbg(d->chip->parent_device,= "startup: %u.%u\n", gpio, d->irq);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0npcmgpio_direction_output(gc, gpio, 1); > +=C2=A0 =C2=A0 =C2=A0 =C2=A0npcmgpio_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 =3D {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0.name =3D "NPCM7XX-GPIO-IRQ", > +=C2=A0 =C2=A0 =C2=A0 =C2=A0.irq_ack =3D npcmgpio_irq_ack,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0.irq_unmask =3D npcmgpio_irq_unmask,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0.irq_mask =3D npcmgpio_irq_mask,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0.irq_set_type =3D npcmgpio_set_irq_type, > +=C2=A0 =C2=A0 =C2=A0 =C2=A0.irq_startup =3D 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

--000000000000de351b0571dbb316--