Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

On 09/11/2013 12:34 AM, Stephen Warren wrote:
> On 09/10/2013 03:37 PM, Mark Brown wrote:
>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>> 
>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>> hence prevent the driver's own gpio_request() from succeeding,
>>> since the GPIO is already requested? If this is not a problem, it
>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>> for the GPIO.
>> 
>> Or at the very least something that's likely to break in the
>> future.
> 
> Looking at the GPIO code, it already prevents double-requests:
> 
>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>                 desc_set_label(desc, label ? : "?");
>>                 status = 0;
>>         } else {
>>                 status = -EBUSY;
>>                 module_put(chip->owner);
>>                 goto done;
>>         }
> 
> And I tested it in practice, and it really does fail.
> 

I'm a bit confused now. Doesn't the fact that gpio_request() prevents
double-requests mean that the use-case that you say that have not been covered
by this patch can't actually happen?

I mean, if when using board files an explicit call to gpio_request() is made by
platform code then a driver can't call gpio_request() for the same gpio. So this
patch shouldn't cause any regression since is just auto-requesting a GPIO when
is mapped as an IRQ in a DT which basically will be the same that was made by
board files before.

To give you an example of an use-case that this patch is trying to solve:

OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
interface with Pseudo-SRAM devices such as ethernet controllers. So with board
files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):

void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *gpmc_cfg)
{
       ....
       if (gpio_request_one(gpmc_cfg->gpio_irq, GPIOF_IN, "smsc911x irq")) {
                pr_err("Failed to request IRQ GPIO%d\n", gpmc_cfg->gpio_irq);
                goto free1;
       }
       ....
       gpmc_smsc911x_resources[1].start = gpio_to_irq(gpmc_cfg->gpio_irq);
       ...
       pdev = platform_device_register_resndata(NULL, "smsc911x", gpmc_cfg->id,
                 gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources),
                 &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config));
       ...
}

and later in the smsc911x ethernet driver probe function:

static int smsc911x_drv_probe(struct platform_device *pdev)
{
       retval = request_irq(dev->irq, smsc911x_irqhandler,
                             irq_flags | IRQF_SHARED, dev->name, dev);
       ...
       irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
       ...
       dev->irq = irq_res->start;
       ...
       retval = request_irq(dev->irq, smsc911x_irqhandler,
                             irq_flags | IRQF_SHARED, dev->name, dev);
       ...
}

The driver just knows that it has to get the IRQ from a struct resource and it
doesn't care if that is a real IRQ line from an interrupt controller or a GPIO
pin mapped as an IRQ. With linus patch I just can define on a DT (GPMC
properties omitted for simplicity):

        ethernet@5,0 {
                pinctrl-names = "default";
                pinctrl-0 = <&smsc911x_pins>;
                compatible = "smsc,lan9221", "smsc,lan9115";
                reg = <5 0 0xff>;
                bank-width = <2>;
                interrupt-parent = <&gpio6>;
                interrupts = <16 8>;
                vmmc-supply = <&vddvario>;
                vmmc_aux-supply = <&vdd33a>;
                reg-io-width = <4>;

                smsc,save-mac-address;
        };

and it will just work. Without Linus patch the call to request_irq() will fail
because a call to gpio_request() has not been made (and thus the GPIO bank was
not enabled).

Thanks a lot and best regards,
Javier