From mboxrd@z Thu Jan 1 00:00:00 1970 From: jamie@jamieiles.com (Jamie Iles) Date: Mon, 19 Dec 2011 00:44:31 +0000 Subject: [PATCH 1/2] gpio: add a driver for the Synopsys DesignWare APB GPIO block In-Reply-To: References: <1324203229-15571-1-git-send-email-jamie@jamieiles.com> Message-ID: <20111219004431.GE2376@gallagher> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Linus, On Sun, Dec 18, 2011 at 11:01:38PM +0100, Linus Walleij wrote: > On Sun, Dec 18, 2011 at 11:13 AM, Jamie Iles wrote: > > > The Synopsys DesignWare block is used in some ARM devices (picoxcell) > > and can be configured to provide multiple banks of GPIO pins. ?The first > > bank (A) can also provide IRQ capabilities. > > Overall this is looking good. > > Here is a problem (I think): > > > +static int dwapb_irq_set_type(struct irq_data *d, u32 type) > > +{ > > + ? ? ? struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > + ? ? ? struct dwapb_gpio *gpio = gc->private; > > + ? ? ? int bit = d->hwirq; > > + ? ? ? unsigned long level, polarity; > > + > > + ? ? ? if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING | > > + ? ? ? ? ? ? ? ? ? ?IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) > > + ? ? ? ? ? ? ? return -EINVAL; > > + > > + ? ? ? level = readl(gpio->regs + INT_TYPE_REG_OFFS); > > + ? ? ? polarity = readl(gpio->regs + INT_POLARITY_REG_OFFS); > > + > > + ? ? ? if (type & IRQ_TYPE_EDGE_RISING) { > > + ? ? ? ? ? ? ? level |= (1 << bit); > > + ? ? ? ? ? ? ? polarity |= (1 << bit); > > + ? ? ? } else if (type & IRQ_TYPE_EDGE_FALLING) { > > + ? ? ? ? ? ? ? level |= (1 << bit); > > + ? ? ? ? ? ? ? polarity &= ~(1 << bit); > > So what if you get request for *both* falling and rising edges? > > This is not uncommon at all, for example a GPIO which detecs > MMC card insertions and removals like drivers/host/mmc/mmci.c > will want interrupts on both edges since we want to change state > whenever the card is inserted *or* removed. > > If you check drivers/gpio/gpio-u300.c you can see how I handled > this on another hardware where triggering on falling and rising > edges was a binary choice and thus mutually exclusive: I toggle > for each interrupt. > > See u300_toggle_trigger(), u300_gpio_irq_type(). > > So either you do something like that, or you detect both set > and return an error, else the poor caller will just get falling > edges in this driver... Hmm, I hadn't thought of that. I've had a quick once-over your u300 gpio driver and that looks pretty neat. I'll give that a go over the next day or two and repost. Thanks for the pointer! Jamie From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jamie Iles Subject: Re: [PATCH 1/2] gpio: add a driver for the Synopsys DesignWare APB GPIO block Date: Mon, 19 Dec 2011 00:44:31 +0000 Message-ID: <20111219004431.GE2376@gallagher> References: <1324203229-15571-1-git-send-email-jamie@jamieiles.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org To: Linus Walleij Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Rob Herring , Linus Walleij , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Linus, On Sun, Dec 18, 2011 at 11:01:38PM +0100, Linus Walleij wrote: > On Sun, Dec 18, 2011 at 11:13 AM, Jamie Iles wrote: > = > > The Synopsys DesignWare block is used in some ARM devices (picoxcell) > > and can be configured to provide multiple banks of GPIO pins. =A0The fi= rst > > bank (A) can also provide IRQ capabilities. > = > Overall this is looking good. > = > Here is a problem (I think): > = > > +static int dwapb_irq_set_type(struct irq_data *d, u32 type) > > +{ > > + =A0 =A0 =A0 struct irq_chip_generic *gc =3D irq_data_get_irq_chip_dat= a(d); > > + =A0 =A0 =A0 struct dwapb_gpio *gpio =3D gc->private; > > + =A0 =A0 =A0 int bit =3D d->hwirq; > > + =A0 =A0 =A0 unsigned long level, polarity; > > + > > + =A0 =A0 =A0 if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING= | > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE= _LEVEL_LOW)) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > > + > > + =A0 =A0 =A0 level =3D readl(gpio->regs + INT_TYPE_REG_OFFS); > > + =A0 =A0 =A0 polarity =3D readl(gpio->regs + INT_POLARITY_REG_OFFS); > > + > > + =A0 =A0 =A0 if (type & IRQ_TYPE_EDGE_RISING) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 level |=3D (1 << bit); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 polarity |=3D (1 << bit); > > + =A0 =A0 =A0 } else if (type & IRQ_TYPE_EDGE_FALLING) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 level |=3D (1 << bit); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 polarity &=3D ~(1 << bit); > = > So what if you get request for *both* falling and rising edges? > = > This is not uncommon at all, for example a GPIO which detecs > MMC card insertions and removals like drivers/host/mmc/mmci.c > will want interrupts on both edges since we want to change state > whenever the card is inserted *or* removed. > = > If you check drivers/gpio/gpio-u300.c you can see how I handled > this on another hardware where triggering on falling and rising > edges was a binary choice and thus mutually exclusive: I toggle > for each interrupt. > = > See u300_toggle_trigger(), u300_gpio_irq_type(). > = > So either you do something like that, or you detect both set > and return an error, else the poor caller will just get falling > edges in this driver... Hmm, I hadn't thought of that. I've had a quick once-over your u300 = gpio driver and that looks pretty neat. I'll give that a go over the = next day or two and repost. Thanks for the pointer! Jamie