RE: [PATCHv2 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@xxxxxxxxxx]
> Sent: 15 October, 2014 3:13 PM
> To: Chang, Rebecca Swee Fun; Denis Turischev
> Cc: Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel Mailing List
> Subject: Re: [PATCHv2 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Fri, Sep 26, 2014 at 12:07 PM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@xxxxxxxxx> 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@xxxxxxxxx>
> (...)
> 
> This patch needs to be rebased on the gpio git "devel" branch or Torvalds'
> HEAD before I can apply it.

I will rebase and resend with the fixes below.

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

The register values defined here are offset value, they are not the exact register address. 
They are not version register as it just carries a register offset value.

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

I will change it to 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;

Noted
> 
> > +       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.

Noted. I will change the phrase accordingly and remove the expression on next submission.

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

Noted with thanks. I will do the changes required and resend the series.
Thanks.

Regards
Rebecca
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå