lkml.org 
[lkml]   [2014]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCHv2 3/3] gpio: sch: Enable IRQ support for Quark X1000
From
On Fri, Sep 26, 2014 at 12:07 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>
(...)

This patch needs to be rebased on the gpio git "devel" branch or
Torvalds' HEAD before I can apply it.

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

So the initial SCH driver for the Intel Poulsbo was submitted by Denis
Turischev in 2010.

Does these registers exist and work on the Poulsbo as well?

Is it really enough to distinguish between these variants by
checking if we're getting an IRQ resource on the device or not?
Is there some version register or so?

> 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;
> + int irq_support;

Isn't that a bool?

> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + sch->irq_support = !!irq;

Yeah, it's a bool....

> + 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 = 0;

= false;

> + 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 add GPIO IRQ descs\n");

Failed to *allocate* actually...

> + sch->irq_base = -1;

This is overzealous. Drop it.

> + goto err_sch_intr_chip;

You're bailing out anyway, see.

> static int sch_gpio_remove(struct platform_device *pdev)
> {
> struct sch_gpio *sch = platform_get_drvdata(pdev);
> + int err;
>
> - gpiochip_remove(&sch->chip);
> - return 0;
> + 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);
> + }
> +
> + err = gpiochip_remove(&sch->chip);
> + if (err)
> + dev_err(&pdev->dev,
> + "%s gpiochip_remove() failed\n", __func__);

So gpiochip_remove() does *NOT* return an error in the current
kernel. We just removed that return value from the SCH driver in the
previous cycle for the reason that we were killing off the return type.
commit 9f5132ae82fdbb047cc187bf689a81c8cc0de7fa
"gpio: remove all usage of gpio_remove retval in driver/gpio"

So don't reintroduce stuff we're actively trying to get rid of.

Apart from this is looks OK, Mika can you ACK the end result?

Yours,
Linus Walleij


\
 
 \ /
  Last update: 2014-10-15 09:42    [W:2.021 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site