regression v4.16 on Nokia N900:/dev/input/event6 aka AV Jack support disappeared

regression v4.16 on Nokia N900:/dev/input/event6 aka AV Jack support disappeared

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: regression v4.16 on Nokia N900:/dev/input/event6 aka AV Jack support disappeared

[Thorsten Leemhuis]

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
> 

Re: regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Daniel Baluta]

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?

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

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/2018.1291171648263/dmesg.out

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3472 bytes --]

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/2018.1291171648263/dmesg.out
> 
> 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;


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Linus Walleij]

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

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

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 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.

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Linus Walleij]

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 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.
>
> 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

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

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 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.
> >
> > 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Linus Walleij]

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 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.
>> >
>> > 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

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Linus Walleij]

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

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3436 bytes --]

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))

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Andrew F. Davis]

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))
> 

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4070 bytes --]

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [alsa-devel] regression v4.16 on Nokia N900: sound does not work

[Andrew F. Davis]

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.

Re: regression v4.16 on Nokia N900:/dev/input/event6 aka AV Jack support disappeared

[Andrew F. Davis]

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
>