* Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
2014-10-16 3:51 ` [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000 Chang Rebecca Swee Fun
@ 2014-10-16 4:01 ` Varka Bhadram
2014-10-16 10:19 ` Mika Westerberg
2014-10-17 8:54 ` Alexandre Courbot
2 siblings, 0 replies; 15+ messages in thread
From: Varka Bhadram @ 2014-10-16 4:01 UTC (permalink / raw)
To: Chang Rebecca Swee Fun, Linus Walleij
Cc: Mika Westerberg, GPIO Subsystem Mailing List,
Linux Kernel Mailing List, Denis Turischev
On 10/16/2014 09:21 AM, 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.
(...)
> 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) {
> + 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))
gpiochip_remove() return 'void' [0].
> + dev_err(&pdev->dev,
> + "%s gpiochip_remove() failed\n", __func__);
> +
> +err_sch_gpio:
> + release_region(res->start, resource_size(res));
> +
> + return err;
> }
[0]:https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/tree/drivers/gpio/gpiolib.c#n311
--
Regards,
Varka Bhadram.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
2014-10-16 3:51 ` [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000 Chang Rebecca Swee Fun
2014-10-16 4:01 ` Varka Bhadram
@ 2014-10-16 10:19 ` Mika Westerberg
2014-11-05 9:33 ` Chang, Rebecca Swee Fun
2014-10-17 8:54 ` Alexandre Courbot
2 siblings, 1 reply; 15+ messages in thread
From: Mika Westerberg @ 2014-10-16 10:19 UTC (permalink / raw)
To: Chang Rebecca Swee Fun
Cc: Linus Walleij, GPIO Subsystem Mailing List,
Linux Kernel Mailing List, Denis Turischev
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 <rebecca.swee.fun.chang@intel.com>
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 <linux/pci_ids.h>
>
> #include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>
> #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
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
2014-10-16 10:19 ` Mika Westerberg
@ 2014-11-05 9:33 ` Chang, Rebecca Swee Fun
2014-11-05 10:15 ` Westerberg, Mika
0 siblings, 1 reply; 15+ messages in thread
From: Chang, Rebecca Swee Fun @ 2014-11-05 9:33 UTC (permalink / raw)
To: Westerberg, Mika
Cc: Linus Walleij, GPIO Subsystem Mailing List,
Linux Kernel Mailing List, Denis Turischev
Sorry for the late reply, was working on something else.
> -----Original Message-----
> From: Westerberg, Mika
> Sent: 16 October, 2014 6:19 PM
> 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
>
> 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
> > <rebecca.swee.fun.chang@intel.com>
>
> 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(-)
> >
(...)
> > +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?
According to Alexandre's review feedback, sch_gpio_register_set() and sch_gpio_register_clear() actually having similar block of codes with sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress of reworking the patches in that direction.
>
> > +}
> > +
> > +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?
This is just to ensure the irq_data struct contains data. If you think this is no needed, I will remove it in next submission.
(...)
> > +
> > +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.
>
I think I will take what you've suggested. :)
(...)
> > 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.
Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. This implementation is to avoid enabling irq support if LPC_SCH passed in a negative value.
As for irq 0, I think it is valid. Based on my observation in "cat /proc/interrupt" there is a 0 irq line available.
>
> > + 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.
Noted with thanks.
Rebecca
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
2014-11-05 9:33 ` Chang, Rebecca Swee Fun
@ 2014-11-05 10:15 ` Westerberg, Mika
0 siblings, 0 replies; 15+ messages in thread
From: Westerberg, Mika @ 2014-11-05 10:15 UTC (permalink / raw)
To: Chang, Rebecca Swee Fun
Cc: Linus Walleij, GPIO Subsystem Mailing List,
Linux Kernel Mailing List, Denis Turischev
On Wed, Nov 05, 2014 at 11:33:01AM +0200, Chang, Rebecca Swee Fun wrote:
> Sorry for the late reply, was working on something else.
>
> > -----Original Message-----
> > From: Westerberg, Mika
> > Sent: 16 October, 2014 6:19 PM
> > 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
> >
> > 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
> > > <rebecca.swee.fun.chang@intel.com>
> >
> > 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(-)
> > >
> (...)
>
> > > +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?
>
> According to Alexandre's review feedback, sch_gpio_register_set() and
> sch_gpio_register_clear() actually having similar block of codes with
> sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio,
> reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc,
> gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress
> of reworking the patches in that direction.
Sounds good.
> >
> > > +}
> > > +
> > > +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?
>
> This is just to ensure the irq_data struct contains data. If you think
> this is no needed, I will remove it in next submission.
I think you can drop that check. Also if 'd' would be NULL you will
crash already in
struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> (...)
> > > +
> > > +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.
> >
> I think I will take what you've suggested. :)
>
> (...)
> > > 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.
>
> Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH.
> This implementation is to avoid enabling irq support if LPC_SCH passed
> in a negative value. As for irq 0, I think it is valid. Based on my
> observation in "cat /proc/interrupt" there is a 0 irq line available.
I see.
In that case, how about following?
irq = platform_get_irq(pdev, 0)
if (irq >= 0) {
/* Add irq support here */
}
>
> >
> > > + 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.
>
> Noted with thanks.
>
> Rebecca
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
2014-10-16 3:51 ` [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000 Chang Rebecca Swee Fun
2014-10-16 4:01 ` Varka Bhadram
2014-10-16 10:19 ` Mika Westerberg
@ 2014-10-17 8:54 ` Alexandre Courbot
2014-10-23 7:29 ` Chang, Rebecca Swee Fun
2 siblings, 1 reply; 15+ messages in thread
From: Alexandre Courbot @ 2014-10-17 8:54 UTC (permalink / raw)
To: Chang Rebecca Swee Fun
Cc: Linus Walleij, Mika Westerberg, GPIO Subsystem Mailing List,
Linux Kernel Mailing List, Denis Turischev
On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> 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 <rebecca.swee.fun.chang@intel.com>
> ---
> 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 <linux/pci_ids.h>
>
> #include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>
> #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);
> +}
> +
> +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;
> +
> + 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);
You will have the same problem as I pointed out in patch 1/3 that
sch_gpio_register_set/clear() will try to acquire the already-locked
sch->lock. No way this can ever work, or I am under a serious
misapprehension.
> + 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");
> + }
> +}
> +
> +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);
Same here.
> +
> + 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) {
> + 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));
> +
> + 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-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
2014-10-17 8:54 ` Alexandre Courbot
@ 2014-10-23 7:29 ` Chang, Rebecca Swee Fun
0 siblings, 0 replies; 15+ messages in thread
From: Chang, Rebecca Swee Fun @ 2014-10-23 7:29 UTC (permalink / raw)
To: 'Alexandre Courbot'
Cc: Linus Walleij, Westerberg, Mika, GPIO Subsystem Mailing List,
Linux Kernel Mailing List, Denis Turischev
> > +
> > +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;
> > +
> > + 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);
>
> You will have the same problem as I pointed out in patch 1/3 that
> sch_gpio_register_set/clear() will try to acquire the already-locked
> sch->lock. No way this can ever work, or I am under a serious
> misapprehension.
>
> > + 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");
> > + }
> > +}
> > +
> > +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);
>
> Same here.
Alright. I should have noticed this double locking issue earlier. Thanks. I think the next version will be much cleaner. Thanks for spending time and effort to review.
Rebecca
^ permalink raw reply [flat|nested] 15+ messages in thread