| Message ID | 1409335127-26712-3-git-send-email-soren.brinkmann@xilinx.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Aug 29, 2014 at 7:58 PM, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > From: Ezra Savard <ezra.savard@xilinx.com> > > Use of unmask/mask in set_wake was an incorrect implementation. The new > implementation correctly sets wakeup for the gpio chip's IRQ so the gpio chip > will not sleep while wakeup-enabled gpio are in use. > > Signed-off-by: Ezra Savard <ezra.savard@xilinx.com> > Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com> Patch applied. However the problems seems quite generic. Do you see this kind of error in other GPIO drivers? IRQchip semantics always make me nervous. Yours, Linus Walleij -- 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
On Thu, 2014-09-04 at 06:27PM +0200, Linus Walleij wrote: > On Fri, Aug 29, 2014 at 7:58 PM, Soren Brinkmann > <soren.brinkmann@xilinx.com> wrote: > > > From: Ezra Savard <ezra.savard@xilinx.com> > > > > Use of unmask/mask in set_wake was an incorrect implementation. The new > > implementation correctly sets wakeup for the gpio chip's IRQ so the gpio chip > > will not sleep while wakeup-enabled gpio are in use. > > > > Signed-off-by: Ezra Savard <ezra.savard@xilinx.com> > > Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > Patch applied. > > However the problems seems quite generic. > > Do you see this kind of error in other GPIO drivers? > > IRQchip semantics always make me nervous. Our implementation was just completely flawed. It did work with our limited tests using the sysfs interface. But once we started with the gpio_keys things fell apart. The set_wake did mask/unmask IRQs, which is clearly the job of the respective mask/unmask function of a gpiochip. After we found that, we looked at a few other drivers and designed this following gpio-mxs. So, the core part was to do the right thing in set_wake. Once that was done, the runtime PM callbacks needed some realignment to determine whether GPIO is a wakeup device or not. So, this was really just a gpio-zynq problem, not really generic. Thanks for applying. I'll post the gpiolib-sysfs patch on its own in a separate submission. Sören -- 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
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c index d80d722529ad..1e6f19a07454 100644 --- a/drivers/gpio/gpio-zynq.c +++ b/drivers/gpio/gpio-zynq.c @@ -88,16 +88,17 @@ * @chip: instance of the gpio_chip * @base_addr: base address of the GPIO device * @clk: clock resource for this controller + * @irq: interrupt for the GPIO device */ struct zynq_gpio { struct gpio_chip chip; void __iomem *base_addr; struct clk *clk; + int irq; }; static struct irq_chip zynq_gpio_level_irqchip; static struct irq_chip zynq_gpio_edge_irqchip; - /** * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank * for a given pin in the GPIO device @@ -434,10 +435,9 @@ static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type) static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on) { - if (on) - zynq_gpio_irq_unmask(data); - else - zynq_gpio_irq_mask(data); + struct zynq_gpio *gpio = irq_data_get_irq_chip_data(data); + + irq_set_irq_wake(gpio->irq, on); return 0; } @@ -518,7 +518,11 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc) static int __maybe_unused zynq_gpio_suspend(struct device *dev) { - if (!device_may_wakeup(dev)) + struct platform_device *pdev = to_platform_device(dev); + int irq = platform_get_irq(pdev, 0); + struct irq_data *data = irq_get_irq_data(irq); + + if (!irqd_is_wakeup_set(data)) return pm_runtime_force_suspend(dev); return 0; @@ -526,7 +530,11 @@ static int __maybe_unused zynq_gpio_suspend(struct device *dev) static int __maybe_unused zynq_gpio_resume(struct device *dev) { - if (!device_may_wakeup(dev)) + struct platform_device *pdev = to_platform_device(dev); + int irq = platform_get_irq(pdev, 0); + struct irq_data *data = irq_get_irq_data(irq); + + if (!irqd_is_wakeup_set(data)) return pm_runtime_force_resume(dev); return 0; @@ -587,7 +595,7 @@ static const struct dev_pm_ops zynq_gpio_dev_pm_ops = { */ static int zynq_gpio_probe(struct platform_device *pdev) { - int ret, bank_num, irq; + int ret, bank_num; struct zynq_gpio *gpio; struct gpio_chip *chip; struct resource *res; @@ -603,10 +611,10 @@ static int zynq_gpio_probe(struct platform_device *pdev) if (IS_ERR(gpio->base_addr)) return PTR_ERR(gpio->base_addr); - irq = platform_get_irq(pdev, 0); - if (irq < 0) { + gpio->irq = platform_get_irq(pdev, 0); + if (gpio->irq < 0) { dev_err(&pdev->dev, "invalid IRQ\n"); - return irq; + return gpio->irq; } /* configure the gpio chip */ @@ -654,14 +662,12 @@ static int zynq_gpio_probe(struct platform_device *pdev) goto err_rm_gpiochip; } - gpiochip_set_chained_irqchip(chip, &zynq_gpio_edge_irqchip, irq, + gpiochip_set_chained_irqchip(chip, &zynq_gpio_edge_irqchip, gpio->irq, zynq_gpio_irqhandler); pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); - device_set_wakeup_capable(&pdev->dev, 1); - return 0; err_rm_gpiochip: