linux-mips
[Top] [All Lists]

Re: [PATCH V2 1/2] bcma: gpio: add own IRQ domain

To: Hauke Mehrtens <hauke@hauke-m.de>
Subject: Re: [PATCH V2 1/2] bcma: gpio: add own IRQ domain
From: Rafał Miłecki <zajec5@gmail.com>
Date: Fri, 29 Nov 2013 17:55:39 +0100
Cc: "linux-mips@linux-mips.org" <linux-mips@linux-mips.org>, Ralf Baechle <ralf@linux-mips.org>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=kQGCBSbvVAkdM5Apcz5/BmrBd+G3I+vg4KIdheU9sYQ=; b=gxNvO4IaQQFpAYMKfG1k3WbXhBLSAkcMCYhDRQkiG2oCfYsKrfBkXiiHtx1AJEBzLE Yg1vfH3SezdnrSdPTXgmO6Ea0DsFhayXe/gjDb5pYRpTMRRgmv6nvzPwij0VDUCJAO1o BbUmmEplJHzYlRM1R2EmmzUXa+fSI84AIrAgovVYNn49C/IgDOqSN1/3suZoLNJI7DRb vVMOZl1GT2+EKBflpxsBW/dKWf0Ie7A5RJTFxUCCRmmgnc0mw+1dLk7Ej8PHz9bGvzyP yvUmGcgdg1+Rlm+Zqr/Ckahl4wQypKgG8IE3F5Ad/sCO4O1S8YR2hHe0bPIAGUdmQqoV SVYw==
In-reply-to: <5298C15F.2040101@hauke-m.de>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <1385741397-32740-1-git-send-email-zajec5@gmail.com> <5298C15F.2040101@hauke-m.de>
Sender: linux-mips-bounce@linux-mips.org
2013/11/29 Hauke Mehrtens <hauke@hauke-m.de>:
> On 11/29/2013 05:09 PM, Rafał Miłecki wrote:
>> Input GPIO changes can generate interrupts, but we need kind of ACK for
>> them by changing IRQ polarity. This is required to stop hardware from
>> keep generating interrupts and generate another one on the next GPIO
>> state change.
>> This code allows using GPIOs with standard interrupts and add for
>> example GPIO buttons support.
>
> As far as I know IRQs for GPIOs are only support on SoCs and not on PCIe
> wifi cards like the BCM43224. I think the dependency to IRQ_DOMAIN
> should only be added when BCMA_HOST_SOC is set.

Sounds sane.


>> +     for_each_set_bit(gpio, (unsigned long *)&irqs, cc->gpio.ngpio) {
>> +             generic_handle_irq(bcma_gpio_to_irq(&cc->gpio, gpio));
>> +             bcma_chipco_gpio_polarity(cc, BIT(gpio),
>> +                                       (val & BIT(gpio)) ? BIT(gpio) : 0);
>
> What about setting this once for all irqs at the end of this function?
> bcma_chipco_gpio_polarity() takes an lock.

OK


>>  int bcma_gpio_init(struct bcma_drv_cc *cc)
>>  {
>>       struct gpio_chip *chip = &cc->gpio;
>> +     unsigned int hwirq, gpio;
>> +     int err;
>>
>> +     /*
>> +      * GPIO chip
>> +      */
>>       chip->label             = "bcma_gpio";
>>       chip->owner             = THIS_MODULE;
>>       chip->request           = bcma_gpio_request;
>> @@ -104,8 +152,48 @@ int bcma_gpio_init(struct bcma_drv_cc *cc)
>>               chip->base              = 0;
>>       else
>>               chip->base              = -1;
>> +     err = gpiochip_add(chip);
>> +     if (err)
>> +             goto err_gpiochip_add;
>
> Shouldn't the gpio chip be registered after we set up the irq handling
> so no one uses it before?

I don't know.

gpio-tegra.c calls functions in this order:
1) gpiochip_add
2) irq_create_mapping3
3) irq_set_chip_and_handler
4) irq_set_chained_handler

gpio-msm-v2:
1) gpiochip_add
2) irq_domain_add_linear
3) irq_set_chained_handler

gpio-mpc8xxx.c: like above

gpio-adnp.c on the other hand:
1) irq_domain_add_linear
2) request_threaded_irq
3) gpiochip_add

gpio-grgpio.c:
1) irq_domain_add_linear
2) gpiochip_add

So that differs pretty much between the drivers.

But I think calling "gpiochip_add" at the end makes some sense. I
guess I was looking at gpio-tegra.c too much.


>> +     for (gpio = 0; gpio < chip->ngpio; gpio++) {
>> +             int irq = irq_create_mapping(cc->irq_domain, gpio);
>> +
>> +             irq_set_chip_data(irq, cc);
>> +             irq_set_chip_and_handler(irq, &bcma_gpio_irq_chip,
>> +                                      handle_simple_irq);
>> +     }
>
> Is there some method needed to free or unregister these functions? This
> could be build as an module when it is not used on a SoC.

Yes: irq_dispose_mapping (it calls irq_set_chip_and_handler with NULL
and NULL for us). I'll use it.

-- 
Rafał

<Prev in Thread] Current Thread [Next in Thread>