by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

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
> > > <[email protected]>
> >
> > 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