[alsa-devel] regression v4.16 on Nokia N900:/dev/input/event6 aka AV Jack support disappeared
Hi!
In v4.16, AV jack support disappeared.
This one is suspect:
commit 7be4b5dc7ffa9499ac6ef33a5ffa9ff43f9b7057 Author: Andrew F. Davis afd@ti.com Date: Wed Nov 29 11:13:59 2017 -0600
ARM: dts: omap3-n900: Fix the audio CODEC's reset pin
The correct DT property for specifying a GPIO used for reset is "reset-gpios", fix this here.
Fixes: 14e3e295b2b9 ("ARM: dts: omap3-n900: Add TLV320AIC3X support")
Signed-off-by: Andrew F. Davis afd@ti.com Signed-off-by: Tony Lindgren tony@atomide.com
How was it tested? It reverts polarity of reset pin, but sound/soc/codecs/tlv320aic3x.c treats those as aliases:
ret = of_get_named_gpio(np, "reset-gpios", 0); if (ret >= 0) { aic3x->gpio_reset = ret; } else { ret = of_get_named_gpio(np, "gpio-reset", 0); if (ret > 0) { dev_warn(&i2c->dev, "Using deprecated property "gpio-r\eset", please update your DT"); aic3x->gpio_reset = ret;
Pavel
JFYI: This issues is tracked in the regression reports for Linux 4.16 (http://bit.ly/lnxregrep416 ) with this id:
Linux-Regression-ID: lr#4b650f
Please include this line in the comment section of patches that are supposed to fix the issue. Please also mention the string once in other mailinglist threads or different bug tracking entries if you or someone else start to discuss the issue there. By including that string you make it a whole lot easier to track where an issue gets discussed and how far patches to fix it have made it. For more details on this please see here: http://bit.ly/lnxregtrackid
Thx for your help. Ciao, Thorsten
On 24.02.2018 22:46, Pavel Machek wrote:
Hi!
In v4.16, AV jack support disappeared.
This one is suspect:
commit 7be4b5dc7ffa9499ac6ef33a5ffa9ff43f9b7057 Author: Andrew F. Davis afd@ti.com Date: Wed Nov 29 11:13:59 2017 -0600
ARM: dts: omap3-n900: Fix the audio CODEC's reset pin The correct DT property for specifying a GPIO used for reset is "reset-gpios", fix this here. Fixes: 14e3e295b2b9 ("ARM: dts: omap3-n900: Add TLV320AIC3X support")Signed-off-by: Andrew F. Davis afd@ti.com Signed-off-by: Tony Lindgren tony@atomide.com
How was it tested? It reverts polarity of reset pin, but sound/soc/codecs/tlv320aic3x.c treats those as aliases:
ret = of_get_named_gpio(np, "reset-gpios", 0); if (ret >= 0) { aic3x->gpio_reset = ret; } else { ret = of_get_named_gpio(np, "gpio-reset", 0); if (ret > 0) { dev_warn(&i2c->dev, "Using deprecated property "gpio-r\eset", please update your DT"); aic3x->gpio_reset = ret;
Pavel
Hi!
JFYI: This issues is tracked in the regression reports for Linux 4.16 (http://bit.ly/lnxregrep416 ) with this id:
Linux-Regression-ID: lr#4b650f
Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.
7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.
I tried to revert sound/soc changes, and sound is broken, too. Nasty :-(.
Pavel
On Mon, Feb 26, 2018 at 3:13 PM, Pavel Machek pavel@ucw.cz wrote:
Hi!
JFYI: This issues is tracked in the regression reports for Linux 4.16 (http://bit.ly/lnxregrep416 ) with this id:
Linux-Regression-ID: lr#4b650f
Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.
7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.
I tried to revert sound/soc changes, and sound is broken, too. Nasty
dmesg log?
On Mon 2018-02-26 16:02:22, Daniel Baluta wrote:
On Mon, Feb 26, 2018 at 3:13 PM, Pavel Machek pavel@ucw.cz wrote:
Hi!
JFYI: This issues is tracked in the regression reports for Linux 4.16 (http://bit.ly/lnxregrep416 ) with this id:
Linux-Regression-ID: lr#4b650f
Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.
7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.
I tried to revert sound/soc changes, and sound is broken, too. Nasty
dmesg log?
Partial dmesg is at: https://github.com/pavelmachek/missy/blob/master/db/phone/nokia/n900/pavel/2...
I should be able to get full one...
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
gpio: of: Support SPI nonstandard GPIO properties
Before it was clearly established that all GPIO properties in the device tree shall be named "foo-gpios" (with the deprecated variant "foo-gpio" for single lines) we unfortunately merged a few bindings which named the lines "gpio-foo" instead.
This is most prominent in the GPIO SPI driver in Linux which names the lines "gpio-sck", "gpio-mosi" and "gpio-miso".
As we want to switch the GPIO SPI driver to using descriptors, we need devm_gpiod_get() to return something reasonable when looking up these in the device tree.
Put in a special #ifdef:ed kludge to do this special lookup only for the SPI case and gets compiled out if we're not enabling SPI. If we have more oddly defined legacy GPIOs like this, they can be handled in a similar manner.
Reviewed-by: Rob Herring robh@kernel.org Signed-off-by: Linus Walleij linus.walleij@linaro.org
Unfortunately, it does not seem to revert cleanly on my v4.16 branch.
Pavel
Hi!
JFYI: This issues is tracked in the regression reports for Linux 4.16 (http://bit.ly/lnxregrep416 ) with this id:
Linux-Regression-ID: lr#4b650f
Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.
7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.
I tried to revert sound/soc changes, and sound is broken, too. Nasty
dmesg log?
Partial dmesg is at: https://github.com/pavelmachek/missy/blob/master/db/phone/nokia/n900/pavel/2...
I should be able to get full one...
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
I reverted it on top of v4.16-rc2, and sound now works. Ideas?
(Aha, and I see I made small mistake reverting... but...)
Pavel
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 564bb7a..50cc590 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -157,36 +157,6 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name, EXPORT_SYMBOL(of_get_named_gpio_flags);
/* - * The SPI GPIO bindings happened before we managed to establish that GPIO - * properties should be named "foo-gpios" so we have this special kludge for - * them. - */ -static struct gpio_desc *of_find_spi_gpio(struct device *dev, const char *con_id, - enum of_gpio_flags *of_flags) -{ - char prop_name[32]; /* 32 is max size of property name */ - struct device_node *np = dev->of_node; - struct gpio_desc *desc; - - /* - * Hopefully the compiler stubs the rest of the function if this - * is false. - */ - if (!IS_ENABLED(CONFIG_SPI_MASTER)) - return ERR_PTR(-ENOENT); - - /* Allow this specifically for "spi-gpio" devices */ - if (!of_device_is_compatible(np, "spi-gpio") || !con_id) - return ERR_PTR(-ENOENT); - - /* Will be "gpio-sck", "gpio-mosi" or "gpio-miso" */ - snprintf(prop_name, sizeof(prop_name), "%s-%s", "gpio", con_id); - - desc = of_get_named_gpiod_flags(np, prop_name, 0, of_flags); - return desc; -} - -/* * Some regulator bindings happened before we managed to establish that GPIO * properties should be named "foo-gpios" so we have this special kludge for * them. @@ -230,7 +200,6 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, struct gpio_desc *desc; unsigned int i;
- /* Try GPIO property "foo-gpios" and "foo-gpio" */ for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) { if (con_id) snprintf(prop_name, sizeof(prop_name), "%s-%s", con_id, @@ -245,14 +214,6 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, break; }
- /* Special handling for SPI GPIOs if used */ - if (IS_ERR(desc)) - desc = of_find_spi_gpio(dev, con_id, &of_flags); - - /* Special handling for regulator GPIOs if used */ - if (IS_ERR(desc)) - desc = of_find_regulator_gpio(dev, con_id, &of_flags); - if (IS_ERR(desc)) return desc;
On Tue, Feb 27, 2018 at 12:13 AM, Pavel Machek pavel@ucw.cz wrote:
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
gpio: of: Support SPI nonstandard GPIO properties
I have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.
Yours, Linus Walleij
Hi!
Linux-Regression-ID: lr#4b650f
On Tue 2018-02-27 09:43:32, Linus Walleij wrote:
On Tue, Feb 27, 2018 at 12:13 AM, Pavel Machek pavel@ucw.cz wrote:
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
gpio: of: Support SPI nonstandard GPIO propertiesI have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.
Do you have any updates? Is there way to fix dts so that this does not trigger on N900?
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
Pavel
On Fri, Mar 2, 2018 at 10:10 AM, Pavel Machek pavel@ucw.cz wrote:
Linux-Regression-ID: lr#4b650f
On Tue 2018-02-27 09:43:32, Linus Walleij wrote:
On Tue, Feb 27, 2018 at 12:13 AM, Pavel Machek pavel@ucw.cz wrote:
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
gpio: of: Support SPI nonstandard GPIO propertiesI have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.
Do you have any updates? Is there way to fix dts so that this does not trigger on N900?
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
Yours, Linus Walleij
On Fri 2018-03-02 10:33:24, Linus Walleij wrote:
On Fri, Mar 2, 2018 at 10:10 AM, Pavel Machek pavel@ucw.cz wrote:
Linux-Regression-ID: lr#4b650f
On Tue 2018-02-27 09:43:32, Linus Walleij wrote:
On Tue, Feb 27, 2018 at 12:13 AM, Pavel Machek pavel@ucw.cz wrote:
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
gpio: of: Support SPI nonstandard GPIO propertiesI have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.
Do you have any updates? Is there way to fix dts so that this does not trigger on N900?
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
After I spent hours bisecting, I was kind of assuming you'd cc me on merge request or something like that... so that I could test it before going mainline. Which of those should fix it?
I tested today's mainline, and... sound fails, different way:
pavel@n900:~$ cat /proc/asound/cards 0 [RX51 ]: RX-51 - RX-51 RX-51 pavel@n900:~$ cd g/tui/ofone/ pavel@n900:~/g/tui/ofone$ cd pavel@n900:~$ festival --tts ahoj ^D uname -a
(Festival is hung).
Let me try the revert again... Pavel
On Fri, Mar 2, 2018 at 11:31 AM, Pavel Machek pavel@ucw.cz wrote:
On Fri 2018-03-02 10:33:24, Linus Walleij wrote:
On Fri, Mar 2, 2018 at 10:10 AM, Pavel Machek pavel@ucw.cz wrote:
Linux-Regression-ID: lr#4b650f
On Tue 2018-02-27 09:43:32, Linus Walleij wrote:
On Tue, Feb 27, 2018 at 12:13 AM, Pavel Machek pavel@ucw.cz wrote:
I did git bisect, and the winner seems to be:
pavel@duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80 Author: Linus Walleij linus.walleij@linaro.org Date: Wed Dec 27 16:37:44 2017 +0100
gpio: of: Support SPI nonstandard GPIO propertiesI have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.
Do you have any updates? Is there way to fix dts so that this does not trigger on N900?
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
After I spent hours bisecting, I was kind of assuming you'd cc me on merge request or something like that... so that I could test it before going mainline.
Sorry, I'm not very good with planning and coordination. I just try my best to keep this working.
I guess ideally we should use Bugzilla to track regressions like this, but it also comes with a bit of overhead so I don't know if it helps more than trying to keep things in the head.
Which of those should fix it?
The first one I guess, the second is mostly about supporting -EPROBE_DEFER for different use cases.
I tested today's mainline, and... sound fails, different way:
pavel@n900:~$ cat /proc/asound/cards 0 [RX51 ]: RX-51 - RX-51 RX-51 pavel@n900:~$ cd g/tui/ofone/ pavel@n900:~/g/tui/ofone$ cd pavel@n900:~$ festival --tts ahoj ^D uname -a
(Festival is hung).
Sorry but I need some background here, I have no idea what festival is, other than it seems to be some kind of userspace test program?
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before.
Does it mean that mainline is working find for you or do we need to look deeper into the problem in the OF lookup?
Yours, Linus Walleij
Hi!
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before.
Does it mean that mainline is working find for you or do we need to look deeper into the problem in the OF lookup?
It now works for me.
(But take a look at the patch I sent -- I believe you are still overwriting return values in a bad way, and code is quite confusing).
Pavel
On Fri, Mar 2, 2018 at 1:14 PM, Pavel Machek pavel@ucw.cz wrote:
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before.
Does it mean that mainline is working find for you or do we need to look deeper into the problem in the OF lookup?
It now works for me.
(But take a look at the patch I sent -- I believe you are still overwriting return values in a bad way, and code is quite confusing).
I think that code is a result of piled fixes, so noone really designed it to look like that.
I'll try to look at it, I am a bit afraid of the code since I've broken so many things with it.
Yours, Linus Walleij
Hi!
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
What is going on with the ()s around == s? You made me look up C operator precedence.
Hmm, and it is also wrong, right? It turns any error code into ENOENT, as it tries to do the "special handling".
* * This means we don't need to look any further for * alternate name conventions, and we should really * preserve the return code for our user to be able to * retry probing later. */ if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER) return desc;
if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT)) break; }
/* Special handling for SPI GPIOs if used */ if (IS_ERR(desc)) desc = of_find_spi_gpio(dev, con_id, &of_flags);
/* Special handling for regulator GPIOs if used */ if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER) desc = of_find_regulator_gpio(dev, con_id, &of_flags);
Something like this?
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 84e5a9d..f0fab26 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx, &of_flags); - /* - * -EPROBE_DEFER in our case means that we found a - * valid GPIO property, but no controller has been - * registered so far. - * - * This means we don't need to look any further for - * alternate name conventions, and we should really - * preserve the return code for our user to be able to - * retry probing later. - */ - if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER) - return desc;
- if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT)) + if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT) break; }
/* Special handling for SPI GPIOs if used */ - if (IS_ERR(desc)) + if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) desc = of_find_spi_gpio(dev, con_id, &of_flags);
/* Special handling for regulator GPIOs if used */ - if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER) + if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) desc = of_find_regulator_gpio(dev, con_id, &of_flags);
if (IS_ERR(desc))
On Fri 2018-03-02 12:10:40, Pavel Machek wrote:
Hi!
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before. Pavel
On 03/02/2018 05:10 AM, Pavel Machek wrote:
Hi!
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
What is going on with the ()s around == s? You made me look up C operator precedence.
Hmm, and it is also wrong, right? It turns any error code into ENOENT, as it tries to do the "special handling".
* * This means we don't need to look any further for * alternate name conventions, and we should really * preserve the return code for our user to be able to * retry probing later. */ if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER) return desc; if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT)) break; } /* Special handling for SPI GPIOs if used */ if (IS_ERR(desc)) desc = of_find_spi_gpio(dev, con_id, &of_flags); /* Special handling for regulator GPIOs if used */ if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER) desc = of_find_regulator_gpio(dev, con_id, &of_flags);Something like this?
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 84e5a9d..f0fab26 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx, &of_flags);
/** -EPROBE_DEFER in our case means that we found a* valid GPIO property, but no controller has been* registered so far.** This means we don't need to look any further for* alternate name conventions, and we should really* preserve the return code for our user to be able to* retry probing later.*/if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)return desc;if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)
I rather like the () so one doesn't always have to look up C operator precedence to verify..
break;}
/* Special handling for SPI GPIOs if used */
- if (IS_ERR(desc))
if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) desc = of_find_spi_gpio(dev, con_id, &of_flags);
/* Special handling for regulator GPIOs if used */
- if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) desc = of_find_regulator_gpio(dev, con_id, &of_flags);
if (IS_ERR(desc))
On Fri 2018-03-02 08:22:52, Andrew F. Davis wrote:
On 03/02/2018 05:10 AM, Pavel Machek wrote:
Hi!
If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...
No patience between the v4.16 release candidates eh ;)
commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")
and
commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")
that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?
Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.
What is going on with the ()s around == s? You made me look up C operator precedence.
Hmm, and it is also wrong, right? It turns any error code into ENOENT, as it tries to do the "special handling".
* * This means we don't need to look any further for * alternate name conventions, and we should really * preserve the return code for our user to be able to * retry probing later. */ if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER) return desc; if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT)) break; } /* Special handling for SPI GPIOs if used */ if (IS_ERR(desc)) desc = of_find_spi_gpio(dev, con_id, &of_flags); /* Special handling for regulator GPIOs if used */ if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER) desc = of_find_regulator_gpio(dev, con_id, &of_flags);Something like this?
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 84e5a9d..f0fab26 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx, &of_flags);
/** -EPROBE_DEFER in our case means that we found a* valid GPIO property, but no controller has been* registered so far.** This means we don't need to look any further for* alternate name conventions, and we should really* preserve the return code for our user to be able to* retry probing later.*/if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)return desc;if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)I rather like the () so one doesn't always have to look up C operator precedence to verify..
Well, Ok, but then the ()s should be at the other ifs nearby, too. See?
break;}
/* Special handling for SPI GPIOs if used */
- if (IS_ERR(desc))
if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) desc = of_find_spi_gpio(dev, con_id, &of_flags);
/* Special handling for regulator GPIOs if used */
- if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) desc = of_find_regulator_gpio(dev, con_id, &of_flags);
if (IS_ERR(desc))
Pavel
On Fri, Mar 02, 2018 at 08:22:52AM -0600, Andrew F. Davis wrote:
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 84e5a9d..f0fab26 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx, &of_flags);
/** -EPROBE_DEFER in our case means that we found a* valid GPIO property, but no controller has been* registered so far.** This means we don't need to look any further for* alternate name conventions, and we should really* preserve the return code for our user to be able to* retry probing later.*/if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)return desc;if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)I rather like the () so one doesn't always have to look up C operator precedence to verify..
Too many make it impossible to see which close paren ties up with which open paren. I've spent way too long in the past reformatting code, where people think that () are a good thing, to work out what the comparison is actually doing before then rewriting the damn thing with less () and in a much clearer way. I'm now convinced that unnecessary () are a very bad thing as they severely harm readability as test complexity increases.
break;}
/* Special handling for SPI GPIOs if used */
- if (IS_ERR(desc))
- if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
These can be simplified to:
if (desc == ERR_PTR(-ENOENT))
since error pointers are unique - ERR_PTR(-ENOENT) is what was returned as an error-pointer, and if IS_ERR(ERR_PTR(-ENOMENT)) etc were false, we'd have big problems as it'd mean we couldn't detect error pointers!
As an added bonus, they don't involve any operator precedence questions either.
On 03/02/2018 11:08 AM, Russell King - ARM Linux wrote:
On Fri, Mar 02, 2018 at 08:22:52AM -0600, Andrew F. Davis wrote:
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 84e5a9d..f0fab26 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx, &of_flags);
/** -EPROBE_DEFER in our case means that we found a* valid GPIO property, but no controller has been* registered so far.** This means we don't need to look any further for* alternate name conventions, and we should really* preserve the return code for our user to be able to* retry probing later.*/if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)return desc;if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)I rather like the () so one doesn't always have to look up C operator precedence to verify..
Too many make it impossible to see which close paren ties up with which open paren. I've spent way too long in the past reformatting code, where people think that () are a good thing, to work out what the comparison is actually doing before then rewriting the damn thing with less () and in a much clearer way. I'm now convinced that unnecessary () are a very bad thing as they severely harm readability as test complexity increases.
Fair enough, I personally prefer always having a new line per condition when doing chained conditionals:
if (something && this == that && !not_this)
break;}
/* Special handling for SPI GPIOs if used */
- if (IS_ERR(desc))
- if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
These can be simplified to:
if (desc == ERR_PTR(-ENOENT))
since error pointers are unique - ERR_PTR(-ENOENT) is what was returned as an error-pointer, and if IS_ERR(ERR_PTR(-ENOMENT)) etc were false, we'd have big problems as it'd mean we couldn't detect error pointers!
As an added bonus, they don't involve any operator precedence questions either.
Looks like your suggestion clears up this one anyway.
On 02/24/2018 03:46 PM, Pavel Machek wrote:
Hi!
In v4.16, AV jack support disappeared.
This one is suspect:
commit 7be4b5dc7ffa9499ac6ef33a5ffa9ff43f9b7057 Author: Andrew F. Davis afd@ti.com Date: Wed Nov 29 11:13:59 2017 -0600
ARM: dts: omap3-n900: Fix the audio CODEC's reset pin The correct DT property for specifying a GPIO used for reset is "reset-gpios", fix this here. Fixes: 14e3e295b2b9 ("ARM: dts: omap3-n900: Add TLV320AIC3X support")Signed-off-by: Andrew F. Davis afd@ti.com Signed-off-by: Tony Lindgren tony@atomide.com
How was it tested? It reverts polarity of reset pin, but sound/soc/codecs/tlv320aic3x.c treats those as aliases:
The polarity was wrong before, the AIC3x devices are active low reset and there is no logic inverters between the SoC and the reset line. GPIO_ACTIVE_LOW is therefor the correct polarity.
This does not change the driver behavior as currently it uses the old gpio_set_value() which does not respect the polarity as set in DT.
When this driver is converted to use gpiod_ style calls having the polarity correct will be important, and right now the old style gpio calls will cause this driver to fail to work if a board comes along that does have some logic inversion on the reset line as GPIO_ACTIVE is ignored.
As Pavel points out I think your trouble is elsewhere.
Andrew
ret = of_get_named_gpio(np, "reset-gpios", 0); if (ret >= 0) { aic3x->gpio_reset = ret; } else { ret = of_get_named_gpio(np, "gpio-reset", 0); if (ret > 0) { dev_warn(&i2c->dev, "Using deprecated property "gpio-r\eset", please update your DT"); aic3x->gpio_reset = ret;
Pavel
participants (6)
-
Andrew F. Davis -
Daniel Baluta -
Linus Walleij -
Pavel Machek -
Russell King - ARM Linux -
Thorsten Leemhuis