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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
