of_get_named_gpio_flags() and of_get_named_gpio() return -EPROBE_DEFER if the
respective GPIO is not (yet) available. This is useful if driver's probe()
functions try to get a GPIO whose controller isn't probed yet. Thus, the driver
can be probed again later on.
The function still returns -EINVAL on other errors (parse error or node doesn't
exist). This way, the case of an optional/intentionally missing GPIO is handled
appropriately.
Signed-off-by: Roland Stigge <[email protected]>
---
drivers/gpio/gpiolib-of.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--- linux-2.6.orig/drivers/gpio/gpiolib-of.c
+++ linux-2.6/drivers/gpio/gpiolib-of.c
@@ -62,7 +62,10 @@ static int of_gpiochip_find_and_xlate(st
int of_get_named_gpio_flags(struct device_node *np, const char *propname,
int index, enum of_gpio_flags *flags)
{
- struct gg_data gg_data = { .flags = flags, .out_gpio = -ENODEV };
+ /* Return -EPROBE_DEFER to support probe() functions to be called
+ * later when the GPIO actually becomes available
+ */
+ struct gg_data gg_data = { .flags = flags, .out_gpio = -EPROBE_DEFER };
int ret;
/* .of_xlate might decide to not fill in the flags, so clear it. */
If the GPIOs used by the MMCI driver are not registered yet when the driver is
probe()d, they can't be used. This happens if the mmci driver is probed before
the respective GPIO controller (e.g. on the LPC32xx EA3250 board, the PCA9532
GPIO controller would be initialized via DT after mmci). Therefore, we defer
mmci in this case.
Signed-off-by: Roland Stigge <[email protected]>
---
drivers/mmc/host/mmci.c | 8 ++++++++
1 file changed, 8 insertions(+)
--- linux-2.6.orig/drivers/mmc/host/mmci.c
+++ linux-2.6/drivers/mmc/host/mmci.c
@@ -1424,6 +1424,10 @@ static int __devinit mmci_probe(struct a
writel(0, host->base + MMCIMASK1);
writel(0xfff, host->base + MMCICLEAR);
+ if (plat->gpio_cd == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto err_gpio_cd;
+ }
if (gpio_is_valid(plat->gpio_cd)) {
ret = gpio_request(plat->gpio_cd, DRIVER_NAME " (cd)");
if (ret == 0)
@@ -1447,6 +1451,10 @@ static int __devinit mmci_probe(struct a
if (ret >= 0)
host->gpio_cd_irq = gpio_to_irq(plat->gpio_cd);
}
+ if (plat->gpio_wp == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto err_gpio_wp;
+ }
if (gpio_is_valid(plat->gpio_wp)) {
ret = gpio_request(plat->gpio_wp, DRIVER_NAME " (wp)");
if (ret == 0)
Zero is a valid GPIO and shouldn't be handled as an error return code from
of_get_named_gpio(). It was a leftover from old code before getting
pdata->gpio_*() was modified.
Signed-off-by: Roland Stigge <[email protected]>
---
drivers/mmc/host/mmci.c | 5 -----
1 file changed, 5 deletions(-)
--- linux-2.6.orig/drivers/mmc/host/mmci.c
+++ linux-2.6/drivers/mmc/host/mmci.c
@@ -1216,12 +1216,7 @@ static void mmci_dt_populate_generic_pda
int bus_width = 0;
pdata->gpio_wp = of_get_named_gpio(np, "wp-gpios", 0);
- if (!pdata->gpio_wp)
- pdata->gpio_wp = -1;
-
pdata->gpio_cd = of_get_named_gpio(np, "cd-gpios", 0);
- if (!pdata->gpio_cd)
- pdata->gpio_cd = -1;
if (of_get_property(np, "cd-inverted", NULL))
pdata->cd_invert = true;
On Sun, Jun 17, 2012 at 12:11:32PM +0200, Roland Stigge wrote:
> If the GPIOs used by the MMCI driver are not registered yet when the driver is
> probe()d, they can't be used. This happens if the mmci driver is probed before
> the respective GPIO controller (e.g. on the LPC32xx EA3250 board, the PCA9532
> GPIO controller would be initialized via DT after mmci). Therefore, we defer
> mmci in this case.
I'm much happier with this. Provided the change to the GPIO OF stuff is
fine.
On Sun, Jun 17, 2012 at 12:11 PM, Roland Stigge <[email protected]> wrote:
> If the GPIOs used by the MMCI driver are not registered yet when the driver is
> probe()d, they can't be used. This happens if the mmci driver is probed before
> the respective GPIO controller (e.g. on the LPC32xx EA3250 board, the PCA9532
> GPIO controller would be initialized via DT after mmci). Therefore, we defer
> mmci in this case.
>
> Signed-off-by: Roland Stigge <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Pls put this into Russell's patch tracker.
Yours,
Linus Walleij
On Sun, Jun 17, 2012 at 12:11 PM, Roland Stigge <[email protected]> wrote:
> of_get_named_gpio_flags() and of_get_named_gpio() return -EPROBE_DEFER if the
> respective GPIO is not (yet) available. This is useful if driver's probe()
> functions try to get a GPIO whose controller isn't probed yet. Thus, the driver
> can be probed again later on.
>
> The function still returns -EINVAL on other errors (parse error or node doesn't
> exist). This way, the case of an optional/intentionally missing GPIO is handled
> appropriately.
>
> Signed-off-by: Roland Stigge <[email protected]>
Acked-by: Linus Walleij <[email protected]>
This will work, good work!
Yours,
Linus Walleij
On Sun, Jun 17, 2012 at 12:11 PM, Roland Stigge <[email protected]> wrote:
> Zero is a valid GPIO and shouldn't be handled as an error return code from
> of_get_named_gpio(). It was a leftover from old code before getting
> pdata->gpio_*() was modified.
>
> Signed-off-by: Roland Stigge <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Pls put this into Russell's patch tracker with the other patch.
Yours,
Linus Walleij
On 06/17/2012 04:11 AM, Roland Stigge wrote:
> of_get_named_gpio_flags() and of_get_named_gpio() return -EPROBE_DEFER if the
> respective GPIO is not (yet) available. This is useful if driver's probe()
> functions try to get a GPIO whose controller isn't probed yet. Thus, the driver
> can be probed again later on.
>
> The function still returns -EINVAL on other errors (parse error or node doesn't
> exist). This way, the case of an optional/intentionally missing GPIO is handled
> appropriately.
While I agree this is a correct change, it is going to break some
existing code - at least sound/soc/tegra/tegra_{wm8903.c,alc5632.c}. I'm
happy to send patches for those files though (is this going into 3.5 or
3.6?). However, have you audited all existing callers (including
indirect, e.g. through plain of_get_named_gpio()) for issues this will
cause?
On 06/18/2012 04:06 AM, Stephen Warren wrote:
> On 06/17/2012 04:11 AM, Roland Stigge wrote:
>> of_get_named_gpio_flags() and of_get_named_gpio() return -EPROBE_DEFER if the
>> respective GPIO is not (yet) available. This is useful if driver's probe()
>> functions try to get a GPIO whose controller isn't probed yet. Thus, the driver
>> can be probed again later on.
>>
>> The function still returns -EINVAL on other errors (parse error or node doesn't
>> exist). This way, the case of an optional/intentionally missing GPIO is handled
>> appropriately.
>
> While I agree this is a correct change, it is going to break some
> existing code - at least sound/soc/tegra/tegra_{wm8903.c,alc5632.c}.
Can you please tell in which way the patch breaks those drivers?
However, I can see that those drivers solved the same problem in a
different way (deferring of_get_named_gpio(), via the sound init()). So
they could be adjusted to take advantage of new -EPROBE_DEFER.
> I'm
> happy to send patches for those files though (is this going into 3.5 or
> 3.6?).
For 3.6 would be best, IMO.
> However, have you audited all existing callers (including
> indirect, e.g. through plain of_get_named_gpio()) for issues this will
> cause?
Thanks for the hint, I searched the code and found
drivers/spi/spi-pl022.c
to be using -ENODEV as indication to return -EPROBE_DEFER from probe().
Will send a patch that adjusts to our of_get_named_gpio_flags() patch
and if it's good, we should join the two.
Some drivers possibly suffer from the same issue that the patch adresses:
drivers/mfd/twl6040-core.c
drivers/staging/nvec/nvec.c
drivers/usb/host/ohci-at91.c
drivers/usb/host/ehci-tegra.c
drivers/usb/gadget/at91_udc.c
drivers/spi/spi-imx.c
drivers/spi/spi-sirf.c
drivers/mmc/host/sdhci-tegra.c
drivers/mmc/host/omap_hsmmc.c
drivers/mmc/host/mxs-mmc.c
drivers/mmc/host/sdhci-esdhc-imx.c
drivers/regulator/tps62360-regulator.c
drivers/regulator/fixed.c
But the breakage therefore exists even before the patch.
Other drivers don't handle the result of of_get_named_gpio*() correctly
at all:
drivers/power/sbs-battery.c
I can help the respective maintainers to work out this issue when we
have the EPROBE_DEFER ready in gpiolib-of.c.
Thanks,
Roland
On 06/18/2012 11:19 AM, Roland Stigge wrote:
>> While I agree this is a correct change, it is going to break some
>> existing code - at least sound/soc/tegra/tegra_{wm8903.c,alc5632.c}.
>
> Can you please tell in which way the patch breaks those drivers?
> However, I can see that those drivers solved the same problem in a
> different way (deferring of_get_named_gpio(), via the sound init()). So
> they could be adjusted to take advantage of new -EPROBE_DEFER
>
>> However, have you audited all existing callers (including
>> indirect, e.g. through plain of_get_named_gpio()) for issues this will
>> cause?
>
> Thanks for the hint, I searched the code and found
>
> drivers/spi/spi-pl022.c
>
> to be using -ENODEV as indication to return -EPROBE_DEFER from probe().
> Will send a patch that adjusts to our of_get_named_gpio_flags() patch
> and if it's good, we should join the two.
Turned out that this is a fix for a yet-unapplied patch (to support dt
for pl022). Alexandre will merge my fix into his patch.
So I can't find immediate breakage from the gpiolib-of patch.
Thanks,
Roland
On 06/18/2012 03:19 AM, Roland Stigge wrote:
> On 06/18/2012 04:06 AM, Stephen Warren wrote:
>> On 06/17/2012 04:11 AM, Roland Stigge wrote:
>>> of_get_named_gpio_flags() and of_get_named_gpio() return -EPROBE_DEFER if the
>>> respective GPIO is not (yet) available. This is useful if driver's probe()
>>> functions try to get a GPIO whose controller isn't probed yet. Thus, the driver
>>> can be probed again later on.
>>>
>>> The function still returns -EINVAL on other errors (parse error or node doesn't
>>> exist). This way, the case of an optional/intentionally missing GPIO is handled
>>> appropriately.
>>
>> While I agree this is a correct change, it is going to break some
>> existing code - at least sound/soc/tegra/tegra_{wm8903.c,alc5632.c}.
>
> Can you please tell in which way the patch breaks those drivers?
> However, I can see that those drivers solved the same problem in a
> different way (deferring of_get_named_gpio(), via the sound init()). So
> they could be adjusted to take advantage of new -EPROBE_DEFER.
The drivers I mentioned test the return code of of_get_named_gpio() to
see if it's -ENODEV, which means that DT property for that GPIO exists
but the driver for it isn't available yet, so the property can't be
parsed. In this case, the sound drivers defer their own probe. If
of_get_named_gpio() is modified to return -EPROBE_DEFER directly, then
it won't be returning -ENODEV, and hence the sound drivers' check for
-ENODEV won't fire, and hence the sound drivers will just continue their
probe assuming that the particular GPIOs are not present on the board
(since they are all optional, so anything other than an explicit
deferral error from of_get_named_gpio() is not treated as an error).
This will break sound on those platforms.
On 06/18/2012 04:50 PM, Stephen Warren wrote:
>> Can you please tell in which way the patch breaks those drivers?
>> However, I can see that those drivers solved the same problem in a
>> different way (deferring of_get_named_gpio(), via the sound init()). So
>> they could be adjusted to take advantage of new -EPROBE_DEFER.
>
> The drivers I mentioned test the return code of of_get_named_gpio() to
> see if it's -ENODEV, which means that DT property for that GPIO exists
> but the driver for it isn't available yet, so the property can't be
> parsed. In this case, the sound drivers defer their own probe. If
> of_get_named_gpio() is modified to return -EPROBE_DEFER directly, then
> it won't be returning -ENODEV, and hence the sound drivers' check for
> -ENODEV won't fire, and hence the sound drivers will just continue their
> probe assuming that the particular GPIOs are not present on the board
> (since they are all optional, so anything other than an explicit
> deferral error from of_get_named_gpio() is not treated as an error).
> This will break sound on those platforms.
Thanks for the hint! I previously also suspected sth. like this but
didn't find it in v3.5-rc3. In broonie's sound.git for-next, I now
finally found it.
Should be easy to fix (replacing the if (... == -ENODEV) to -EPROBE_DEFER.
Will you provide patches as signalled, of should I? Which branch would
be the correct one to build on top?
Thanks in advance,
Roland
On 06/18/2012 09:08 AM, Roland Stigge wrote:
> On 06/18/2012 04:50 PM, Stephen Warren wrote:
>>> Can you please tell in which way the patch breaks those drivers?
>>> However, I can see that those drivers solved the same problem in a
>>> different way (deferring of_get_named_gpio(), via the sound init()). So
>>> they could be adjusted to take advantage of new -EPROBE_DEFER.
>>
>> The drivers I mentioned test the return code of of_get_named_gpio() to
>> see if it's -ENODEV, which means that DT property for that GPIO exists
>> but the driver for it isn't available yet, so the property can't be
>> parsed. In this case, the sound drivers defer their own probe. If
>> of_get_named_gpio() is modified to return -EPROBE_DEFER directly, then
>> it won't be returning -ENODEV, and hence the sound drivers' check for
>> -ENODEV won't fire, and hence the sound drivers will just continue their
>> probe assuming that the particular GPIOs are not present on the board
>> (since they are all optional, so anything other than an explicit
>> deferral error from of_get_named_gpio() is not treated as an error).
>> This will break sound on those platforms.
>
> Thanks for the hint! I previously also suspected sth. like this but
> didn't find it in v3.5-rc3. In broonie's sound.git for-next, I now
> finally found it.
>
> Should be easy to fix (replacing the if (... == -ENODEV) to -EPROBE_DEFER.
>
> Will you provide patches as signalled, of should I? Which branch would
> be the correct one to build on top?
I'm happy either way. It'd probably be best to roll the change into your
patch/series so you can manage all the dependencies in one series, but
if you can't for some reason, I'm happy to provide a patch for this.
On 06/18/2012 05:12 PM, Stephen Warren wrote:
>> Thanks for the hint! I previously also suspected sth. like this but
>> didn't find it in v3.5-rc3. In broonie's sound.git for-next, I now
>> finally found it.
>>
>> Should be easy to fix (replacing the if (... == -ENODEV) to -EPROBE_DEFER.
>>
>> Will you provide patches as signalled, of should I? Which branch would
>> be the correct one to build on top?
>
> I'm happy either way. It'd probably be best to roll the change into your
> patch/series so you can manage all the dependencies in one series, but
> if you can't for some reason, I'm happy to provide a patch for this.
I should be able ;-) - is broonie's sound.git, branch for-next the
correct one to patch against? Or sth. better suited?
Thanks in advance,
Roland
On 06/18/2012 09:23 AM, Roland Stigge wrote:
> On 06/18/2012 05:12 PM, Stephen Warren wrote:
>>> Thanks for the hint! I previously also suspected sth. like this but
>>> didn't find it in v3.5-rc3. In broonie's sound.git for-next, I now
>>> finally found it.
>>>
>>> Should be easy to fix (replacing the if (... == -ENODEV) to -EPROBE_DEFER.
>>>
>>> Will you provide patches as signalled, of should I? Which branch would
>>> be the correct one to build on top?
>>
>> I'm happy either way. It'd probably be best to roll the change into your
>> patch/series so you can manage all the dependencies in one series, but
>> if you can't for some reason, I'm happy to provide a patch for this.
>
> I should be able ;-) - is broonie's sound.git, branch for-next the
> correct one to patch against?
Yes, that's the one. Thanks.
On 06/18/2012 05:45 PM, Stephen Warren wrote:
>>>> Should be easy to fix (replacing the if (... == -ENODEV) to -EPROBE_DEFER.
>>>>
>>>> Will you provide patches as signalled, of should I? Which branch would
>>>> be the correct one to build on top?
>>>
>>> I'm happy either way. It'd probably be best to roll the change into your
>>> patch/series so you can manage all the dependencies in one series, but
>>> if you can't for some reason, I'm happy to provide a patch for this.
>>
>> I should be able ;-) - is broonie's sound.git, branch for-next the
>> correct one to patch against?
>
> Yes, that's the one. Thanks.
I'm posting this as a series of 2 for the sound changes only. Would be
easiest to merge separately via sound/for-next, and the gpiolib-of
change via gpio. However, this could break bisecting.
Since the respective precondition commits are only in the sound tree,
this would be the only one practical for merging a single combined
patch (combining those 3). Would this be OK for the GPIO maintainers?
It's practically only a one-line change in gpiolib-of.c that would come
in via sound.
Thanks in advance,
Roland
PS: Just for illustration purposes for the sound maintainers:
---
drivers/gpio/gpiolib-of.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--- linux-2.6.orig/drivers/gpio/gpiolib-of.c
+++ linux-2.6/drivers/gpio/gpiolib-of.c
@@ -62,7 +62,10 @@ static int of_gpiochip_find_and_xlate(st
int of_get_named_gpio_flags(struct device_node *np, const char *propname,
int index, enum of_gpio_flags *flags)
{
- struct gg_data gg_data = { .flags = flags, .out_gpio = -ENODEV };
+ /* Return -EPROBE_DEFER to support probe() functions to be called
+ * later when the GPIO actually becomes available
+ */
+ struct gg_data gg_data = { .flags = flags, .out_gpio = -EPROBE_DEFER };
int ret;
/* .of_xlate might decide to not fill in the flags, so clear it. */