Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752607AbaJPKhg (ORCPT ); Thu, 16 Oct 2014 06:37:36 -0400 Received: from mga11.intel.com ([192.55.52.93]:44479 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751823AbaJPKTX (ORCPT ); Thu, 16 Oct 2014 06:19:23 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,731,1406617200"; d="scan'208";a="606352189" Date: Thu, 16 Oct 2014 13:19:18 +0300 From: Mika Westerberg To: Chang Rebecca Swee Fun Cc: Linus Walleij , GPIO Subsystem Mailing List , Linux Kernel Mailing List , Denis Turischev Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000 Message-ID: <20141016101918.GC2255@lahna.fi.intel.com> References: <1413431475-12799-1-git-send-email-rebecca.swee.fun.chang@intel.com> <1413431475-12799-4-git-send-email-rebecca.swee.fun.chang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1413431475-12799-4-git-send-email-rebecca.swee.fun.chang@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote: > Intel Quark X1000 GPIO controller supports interrupt handling for > both core power well and resume power well. This patch is to enable > the IRQ support and provide IRQ handling for Intel Quark X1000 > GPIO-SCH device driver. > > This piece of work is derived from Dan O'Donovan's initial work for > Quark X1000 enabling. > > Signed-off-by: Chang Rebecca Swee Fun In addition to what Varka Bhadram pointed out, I have few comments. See below. Overall, looks good. > --- > drivers/gpio/gpio-sch.c | 257 ++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 245 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c > index 952990f..dd84b1f 100644 > --- a/drivers/gpio/gpio-sch.c > +++ b/drivers/gpio/gpio-sch.c > @@ -28,17 +28,30 @@ > #include > > #include > +#include > +#include > > #define GEN 0x00 > #define GIO 0x04 > #define GLV 0x08 > +#define GTPE 0x0C > +#define GTNE 0x10 > +#define GGPE 0x14 > +#define GSMI 0x18 > +#define GTS 0x1C > +#define CGNMIEN 0x40 > +#define RGNMIEN 0x44 > > struct sch_gpio { > struct gpio_chip chip; > + struct irq_data data; > spinlock_t lock; > unsigned short iobase; > unsigned short core_base; > unsigned short resume_base; > + int irq_base; > + int irq_num; > + bool irq_support; > }; > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio) > static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > + unsigned long flags; > unsigned short offset, bit; > u8 enable; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > > offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > if (!(enable & BIT(bit))) > outb(enable | BIT(bit), sch->iobase + offset); > > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > + unsigned long flags; > unsigned short offset, bit; > u8 disable; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > > offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > if (disable & BIT(bit)) > outb(disable & ~BIT(bit), sch->iobase + offset); > > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg) > @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg, > static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_register_set(sch, gpio_num, GIO); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > return 0; > } > > @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) > static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_reg_set(gc, gpio_num, GLV, val); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_register_clear(sch, gpio_num, GIO); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > > /* > * according to the datasheet, writing to the level register has no > @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > return 0; > } > > +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > +{ > + struct sch_gpio *sch = to_sch_gpio(gc); > + > + return sch->irq_base + offset; > +} > + > static struct gpio_chip sch_gpio_chip = { > .label = "sch_gpio", > .owner = THIS_MODULE, > @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = { > .get = sch_gpio_get, > .direction_output = sch_gpio_direction_out, > .set = sch_gpio_set, > + .to_irq = sch_gpio_to_irq, > }; > > +static void sch_gpio_irq_enable(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_register_set(sch, gpio_num, GGPE); > +} > + > +static void sch_gpio_irq_disable(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_register_clear(sch, gpio_num, GGPE); > +} > + > +static void sch_gpio_irq_ack(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1); Maybe use the new sch_gpio_register_set() here instead? > +} > + > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + unsigned long flags; > + u32 gpio_num; > + > + if (d == NULL) > + return -EINVAL; How can that happen? > + > + gpio_num = d->irq - sch->irq_base; > + > + spin_lock_irqsave(&sch->lock, flags); > + > + switch (type) { > + case IRQ_TYPE_EDGE_RISING: > + sch_gpio_register_set(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + sch_gpio_register_set(sch, gpio_num, GTNE); > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + break; > + > + case IRQ_TYPE_EDGE_BOTH: > + sch_gpio_register_set(sch, gpio_num, GTPE); > + sch_gpio_register_set(sch, gpio_num, GTNE); > + break; > + > + case IRQ_TYPE_NONE: > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + break; > + > + default: > + spin_unlock_irqrestore(&sch->lock, flags); > + return -EINVAL; > + } > + > + spin_unlock_irqrestore(&sch->lock, flags); > + > + return 0; > +} > + > +static struct irq_chip sch_irq_chip = { > + .irq_enable = sch_gpio_irq_enable, > + .irq_disable = sch_gpio_irq_disable, > + .irq_ack = sch_gpio_irq_ack, > + .irq_set_type = sch_gpio_irq_type, > +}; > + > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + irq_set_chip_data(i + sch->irq_base, sch); > + irq_set_chip_and_handler_name(i + sch->irq_base, > + &sch_irq_chip, > + handle_simple_irq, > + "sch_gpio_irq_chip"); Hmm, I wonder if this irq_set_chip_and_handler_name(i + sch->irq_base, &sch_irq_chip, handle_simple_irq, "sch_gpio_irq_chip"); looks better. Up to you. > + } > +} > + > +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + irq_set_chip_data(i + sch->irq_base, 0); > + irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0); > + } > +} > + > +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned long flags; > + unsigned int gpio_num; > + > + spin_lock_irqsave(&sch->lock, flags); > + > + for (gpio_num = 0; gpio_num < num; gpio_num++) { > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + sch_gpio_register_clear(sch, gpio_num, GGPE); > + sch_gpio_register_clear(sch, gpio_num, GSMI); > + > + if (gpio_num >= 2) > + sch_gpio_register_clear(sch, gpio_num, RGNMIEN); > + else > + sch_gpio_register_clear(sch, gpio_num, CGNMIEN); > + > + /* clear any pending interrupts */ > + sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1); > + } > + > + spin_unlock_irqrestore(&sch->lock, flags); > +} > + > +static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id) > +{ > + struct sch_gpio *sch = dev_id; > + int res; > + unsigned int i; > + int ret = IRQ_NONE; > + > + for (i = 0; i < sch->chip.ngpio; i++) { > + res = sch_gpio_reg_get(&sch->chip, i, GTS); > + if (res) { > + /* clear by setting GTS to 1 */ > + sch_gpio_reg_set(&sch->chip, i, GTS, 1); > + generic_handle_irq(sch->irq_base + i); > + ret = IRQ_HANDLED; > + } > + } > + > + return ret; > +} > + > static int sch_gpio_probe(struct platform_device *pdev) > { > struct sch_gpio *sch; > - struct resource *res; > + struct resource *res, *irq; > + int err; > > sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); > if (!sch) > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev) > pdev->name)) > return -EBUSY; > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + sch->irq_support = !!irq; > + if (sch->irq_support) { > + sch->irq_num = irq->start; > + if (sch->irq_num < 0) { Is this really possible? I mean can't you just detect the irq support by looking sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not valid. > + dev_warn(&pdev->dev, > + "failed to obtain irq number for device\n"); > + sch->irq_support = false; > + } > + } > + > spin_lock_init(&sch->lock); > sch->iobase = res->start; > sch->chip = sch_gpio_chip; > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev) > return -ENODEV; > } > > + err = gpiochip_add(&sch->chip); > + if (err < 0) > + goto err_sch_gpio; > + > + if (sch->irq_support) { > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > + NUMA_NO_NODE); > + if (sch->irq_base < 0) { > + dev_err(&pdev->dev, > + "failed to allocate GPIO IRQ descs\n"); > + goto err_sch_intr_chip; > + } > + > + /* disable interrupts */ > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > + > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > + IRQF_SHARED, KBUILD_MODNAME, sch); > + if (err) { > + dev_err(&pdev->dev, > + "%s failed to request IRQ\n", __func__); > + goto err_sch_request_irq; > + } > + > + sch_gpio_irqs_init(sch, sch->chip.ngpio); > + } > + > platform_set_drvdata(pdev, sch); > > - return gpiochip_add(&sch->chip); > + return 0; > + > +err_sch_request_irq: > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + > +err_sch_intr_chip: > + if (gpiochip_remove(&sch->chip)) > + dev_err(&pdev->dev, > + "%s gpiochip_remove() failed\n", __func__); > + > +err_sch_gpio: > + release_region(res->start, resource_size(res)); This is not needed as devm_request_region() will clean it up for you automatically. Even if probe() fails. > + > + return err; > } > > static int sch_gpio_remove(struct platform_device *pdev) > { > struct sch_gpio *sch = platform_get_drvdata(pdev); > > + if (sch->irq_support) { > + sch_gpio_irqs_deinit(sch, sch->chip.ngpio); > + > + if (sch->irq_num >= 0) > + free_irq(sch->irq_num, sch); > + > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + } > + > gpiochip_remove(&sch->chip); > return 0; > } > -- > 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/