[PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

Not all devices with ACPI and this combination of sound devices will
have the required information provided via ACPI.  Reintroduce the I2C
device ID to restore sound functionality on on the Chromebook 'Samus'
model.

Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI enumeration")
Cc: Bard Liao <bardliao@realtek.com>
Cc: Oder Chiou <oder_chiou@realtek.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: alsa-devel@alsa-project.org
CC: linux-kernel@vger.kernel.org
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
that's not running ChromeOS).  My fault for getting out of the habit of
trying -rc1 when it comes out and not spotting this sooner.  I'm not
100% sure if this fix is correct for all cases as I'm only able to test
my hardware here, and this does fix my laptop.
---
 sound/soc/codecs/rt5677.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 36e530a36c82..6f629278d982 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -5021,6 +5021,7 @@ static int rt5677_write(void *context, unsigned int reg, unsigned int val)
 static const struct i2c_device_id rt5677_i2c_id[] = {
 	{ "rt5677", RT5677 },
 	{ "rt5676", RT5676 },
+	{ "RT5677CE:00", RT5677 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);
-- 
1.9.1

[PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

Not all devices with ACPI and this combination of sound devices will
have the required information provided via ACPI.  Reintroduce the I2C
device ID to restore sound functionality on on the Chromebook 'Samus'
model.

Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI enumeration")
Cc: Bard Liao <bardliao@realtek.com>
Cc: Oder Chiou <oder_chiou@realtek.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: alsa-devel@alsa-project.org
CC: linux-kernel@vger.kernel.org
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
that's not running ChromeOS).  My fault for getting out of the habit of
trying -rc1 when it comes out and not spotting this sooner.  I'm not
100% sure if this fix is correct for all cases as I'm only able to test
my hardware here, and this does fix my laptop.
---
 sound/soc/codecs/rt5677.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 36e530a36c82..6f629278d982 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -5021,6 +5021,7 @@ static int rt5677_write(void *context, unsigned int reg, unsigned int val)
 static const struct i2c_device_id rt5677_i2c_id[] = {
 	{ "rt5677", RT5677 },
 	{ "rt5676", RT5676 },
+	{ "RT5677CE:00", RT5677 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);
-- 
1.9.1

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Tue, Aug 22, 2017 at 09:51:46PM -0400, Tom Rini wrote:

> Not all devices with ACPI and this combination of sound devices will
> have the required information provided via ACPI.  Reintroduce the I2C
> device ID to restore sound functionality on on the Chromebook 'Samus'
> model.

>  	{ "rt5677", RT5677 },
>  	{ "rt5676", RT5676 },
> +	{ "RT5677CE:00", RT5677 },
>  	{ }
>  };

You're going to have to provide a clearer changelog here, that's
obviously an ACPI ID...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

[stupid google spam filtered _this_ as spam, I don't know why]

On Wed, Aug 23, 2017 at 10:28:28AM +0100, Mark Brown wrote:
> On Tue, Aug 22, 2017 at 09:51:46PM -0400, Tom Rini wrote:
> 
> > Not all devices with ACPI and this combination of sound devices will
> > have the required information provided via ACPI.  Reintroduce the I2C
> > device ID to restore sound functionality on on the Chromebook 'Samus'
> > model.
> 
> >  	{ "rt5677", RT5677 },
> >  	{ "rt5676", RT5676 },
> > +	{ "RT5677CE:00", RT5677 },
> >  	{ }
> >  };
> 
> You're going to have to provide a clearer changelog here, that's
> obviously an ACPI ID...

After looking at 89128534f925 (which introduced the above line, and thus
support for the Chromebook), I think that 36afb0ab648 and 55e59aa0525a
are wrong and should be reverted.  It seems like they're an attempt to
make 89128534f925 be done 'properly' but it also seems like the
Chromebook is the only platform in question that triggers that code and
it results in a NULL pointer dereference, so it's a regression on the
only platform that makes use of the code paths in question.  I'd also be
happy to try a patch on top of master to see if that resolves things.
The oops in question looks like:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: rt5677_i2c_probe+0x4a/0x730 [snd_soc_rt5677]
PGD 0 
P4D 0 

Oops: 0000 [#1] SMP
Modules linked in: snd_soc_rt5677(+) btusb(+) btrtl iwlwifi(+) snd_soc_rl6231 btbcm snd_
CPU: 1 PID: 403 Comm: systemd-udevd Not tainted 4.12.0-rc1ph+ #119
Hardware name: GOOGLE Samus, BIOS          04/02/2015
task: ffff947de8ca1cc0 task.stack: ffffaf5641fe0000
RIP: 0010:rt5677_i2c_probe+0x4a/0x730 [snd_soc_rt5677]
RSP: 0000:ffffaf5641fe3b30 EFLAGS: 00010246
RAX: ffff947de8e02618 RBX: ffff947de8e02618 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000286 RDI: ffff947dec7ece98
RBP: ffffaf5641fe3b68 R08: ffff947dfec9bda0 R09: ffff947de8e02600
R10: ffff947dfec9bd20 R11: ffffd7ef91a12180 R12: ffff947dec7ecc20
R13: ffff947dec7ecc00 R14: 0000000000000000 R15: 0000000000000000
FS:  00007f9bd7fdb8c0(0000) GS:ffff947dfec80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000018 CR3: 000000046872b000 CR4: 00000000003406e0
Call Trace:
 ? acpi_dev_pm_attach+0xa4/0xe0
 ? rt5677_to_irq+0xe0/0xe0 [snd_soc_rt5677]
 i2c_device_probe+0x17c/0x230
 driver_probe_device+0x2a1/0x460
 __driver_attach+0xd8/0xe0
 ? driver_probe_device+0x460/0x460
 bus_for_each_dev+0x58/0x90
 driver_attach+0x19/0x20
 bus_add_driver+0x40/0x270
 driver_register+0x5b/0xe0
 i2c_register_driver+0x3b/0x80
 ? 0xffffffffc072b000
 rt5677_i2c_driver_init+0x17/0x1000 [snd_soc_rt5677]
 do_one_initcall+0x4c/0x1a0
 ? kmem_cache_alloc+0xf7/0x150
 do_init_module+0x56/0x1e8
 load_module+0x1f54/0x2710
 ? symbol_put_addr+0x30/0x30
 SyS_finit_module+0x96/0xd0
 do_syscall_64+0x4e/0xb0
 entry_SYSCALL64_slow_path+0x25/0x25

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Wed, Aug 23, 2017 at 06:35:24PM -0400, Tom Rini wrote:

> After looking at 89128534f925 (which introduced the above line, and thus

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

> support for the Chromebook), I think that 36afb0ab648 and 55e59aa0525a
> are wrong and should be reverted.  It seems like they're an attempt to
> make 89128534f925 be done 'properly' but it also seems like the

Please be more specific.  The only obvious issue with the original patch
"ASoC: rt5677: Add ACPI support" is that it adds an I2C ID instead of an
ACPI ID.  I don't have 36afb0ab648 so I've no idea what it is and
55e59aa0525a is "ASoC: rt5677: Move platform code to board file" which
is a code motion patch and looks more like stylistic faff around the
shambles that is ACPI than anything else.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Wed, Aug 23, 2017 at 11:47:52PM +0100, Mark Brown wrote:
> On Wed, Aug 23, 2017 at 06:35:24PM -0400, Tom Rini wrote:
> 
> > After looking at 89128534f925 (which introduced the above line, and thus
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

Sorry, let me re-phrase.  The commit:
commit 89128534f925711eea1653c264683b7d14a46530
Author: John Keeping <john@metanate.com>
Date:   Wed Aug 24 22:06:35 2016 +0100

    ASoC: rt5677: Add ACPI support

    The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, but
    does not use the standard DT property names so add a new function to
    parse the codec properties from these ACPI properties.

    Also, the GPIOs are only available by index, so we need to register a
    mapping to allow machine drivers to access the GPIOs by name.

Adds all of the code which "ASoC: rt5677: Move platform code to board file"
and "ASoC: rt5677: Introduce proper table for ACPI enumeration" move
around and re-work.

> > support for the Chromebook), I think that 36afb0ab648 and 55e59aa0525a
> > are wrong and should be reverted.  It seems like they're an attempt to
> > make 89128534f925 be done 'properly' but it also seems like the
> 
> Please be more specific.  The only obvious issue with the original patch
> "ASoC: rt5677: Add ACPI support" is that it adds an I2C ID instead of an
> ACPI ID.  I don't have 36afb0ab648 so I've no idea what it is and
> 55e59aa0525a is "ASoC: rt5677: Move platform code to board file" which
> is a code motion patch and looks more like stylistic faff around the
> shambles that is ACPI than anything else.

Ugh, typo on my part proving your point :)  I meant _a_36afb... which is
"ASoC: rt5677: Introduce proper table for ACPI enumeration".  My gut
feeling (and I'd be happy to be told how to poke ACPI to confirm this..)
is that the ACPI table is in fact not including some information that
the code expects and that's why the original patch added an I2C ID not
an ACPI ID.

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 2374 bytes --]

On Wed, Aug 23, 2017 at 11:47:52PM +0100, Mark Brown wrote:
> On Wed, Aug 23, 2017 at 06:35:24PM -0400, Tom Rini wrote:
> 
> > After looking at 89128534f925 (which introduced the above line, and thus
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

Sorry, let me re-phrase.  The commit:
commit 89128534f925711eea1653c264683b7d14a46530
Author: John Keeping <john@metanate.com>
Date:   Wed Aug 24 22:06:35 2016 +0100

    ASoC: rt5677: Add ACPI support

    The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, but
    does not use the standard DT property names so add a new function to
    parse the codec properties from these ACPI properties.

    Also, the GPIOs are only available by index, so we need to register a
    mapping to allow machine drivers to access the GPIOs by name.

Adds all of the code which "ASoC: rt5677: Move platform code to board file"
and "ASoC: rt5677: Introduce proper table for ACPI enumeration" move
around and re-work.

> > support for the Chromebook), I think that 36afb0ab648 and 55e59aa0525a
> > are wrong and should be reverted.  It seems like they're an attempt to
> > make 89128534f925 be done 'properly' but it also seems like the
> 
> Please be more specific.  The only obvious issue with the original patch
> "ASoC: rt5677: Add ACPI support" is that it adds an I2C ID instead of an
> ACPI ID.  I don't have 36afb0ab648 so I've no idea what it is and
> 55e59aa0525a is "ASoC: rt5677: Move platform code to board file" which
> is a code motion patch and looks more like stylistic faff around the
> shambles that is ACPI than anything else.

Ugh, typo on my part proving your point :)  I meant _a_36afb... which is
"ASoC: rt5677: Introduce proper table for ACPI enumeration".  My gut
feeling (and I'd be happy to be told how to poke ACPI to confirm this..)
is that the ACPI table is in fact not including some information that
the code expects and that's why the original patch added an I2C ID not
an ACPI ID.

-- 
Tom

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Wed, Aug 23, 2017 at 06:54:52PM -0400, Tom Rini wrote:

> Ugh, typo on my part proving your point :)  I meant _a_36afb... which is
> "ASoC: rt5677: Introduce proper table for ACPI enumeration".  My gut

The code that's causing issues looks like it's generic ACPI code which
is worrying me, it looks like it's dying setting up the PM.  It could be
that the trace is a bit misleading or that it's the result of earlier
damage though.

> feeling (and I'd be happy to be told how to poke ACPI to confirm this..)
> is that the ACPI table is in fact not including some information that
> the code expects and that's why the original patch added an I2C ID not
> an ACPI ID.

I'm pretty sure it's just that the people writing the code didn't really
know how ACPI is supposed to work in Linux so used the fallback path
which appears to have been copied from OF for some reason.  It makes
sense with OF because the IDs OF uses include the name of the part like
the Linux internal IDs rather than just some random line noise like is
apparently idiomatic for ACPI (this one is one of the better ones!) so
there's a decent chance that a driver that gets found might do something
sensible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Wed, Aug 23, 2017 at 11:47:52PM +0100, Mark Brown wrote:

> > support for the Chromebook), I think that 36afb0ab648 and 55e59aa0525a
> > are wrong and should be reverted.  It seems like they're an attempt to
> > make 89128534f925 be done 'properly' but it also seems like the

> Please be more specific.  The only obvious issue with the original patch
> "ASoC: rt5677: Add ACPI support" is that it adds an I2C ID instead of an
> ACPI ID.  I don't have 36afb0ab648 so I've no idea what it is and
> 55e59aa0525a is "ASoC: rt5677: Move platform code to board file" which
> is a code motion patch and looks more like stylistic faff around the
> shambles that is ACPI than anything else.

My best guess if you're using mainline is that this is triggered by
a36afb0ab6488ea (ASoC: rt5677: Introduce proper table for ACPI
enumeration) causing the ACPI core code to run and explode on whatever
you've got in the tables on that system.  Someone who knows what ACPI is
up to should probably dig into what's going on, even if reverting fixes
it that looks worryingly like we might explode on other devices.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> Not all devices with ACPI and this combination of sound devices will
> have the required information provided via ACPI.  Reintroduce the I2C
> device ID to restore sound functionality on on the Chromebook 'Samus'
> model.

> This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> that's not running ChromeOS).  My fault for getting out of the habit
> of
> trying -rc1 when it comes out and not spotting this sooner.  I'm not
> 100% sure if this fix is correct for all cases as I'm only able to
> test
> my hardware here, and this does fix my laptop.

Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
in the module sources") does not fix your issue?

> diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
> index 36e530a36c82..6f629278d982 100644
> --- a/sound/soc/codecs/rt5677.c
> +++ b/sound/soc/codecs/rt5677.c
> @@ -5021,6 +5021,7 @@ static int rt5677_write(void *context, unsigned
> int reg, unsigned int val)
>  static const struct i2c_device_id rt5677_i2c_id[] = {
>  	{ "rt5677", RT5677 },
>  	{ "rt5676", RT5676 },
> +	{ "RT5677CE:00", RT5677 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);

This one looks weird.

The board code has this 

sound/soc/intel/boards/bdw-rt5677.c:285:                .codec_name =
"i2c-RT5677CE:00",

It's clearly a match to ACPI enumerated I2C slave device.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:

> On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > Not all devices with ACPI and this combination of sound devices will
> > have the required information provided via ACPI.  Reintroduce the I2C
> > device ID to restore sound functionality on on the Chromebook 'Samus'
> > model.
> 
> > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > that's not running ChromeOS).  My fault for getting out of the habit
> > of
> > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > 100% sure if this fix is correct for all cases as I'm only able to
> > test
> > my hardware here, and this does fix my laptop.
> 
> Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> in the module sources") does not fix your issue?

As that's not in master yet I can't tell.  Can you give me a pointer to
somewhere?  Thanks!

> > diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
> > index 36e530a36c82..6f629278d982 100644
> > --- a/sound/soc/codecs/rt5677.c
> > +++ b/sound/soc/codecs/rt5677.c
> > @@ -5021,6 +5021,7 @@ static int rt5677_write(void *context, unsigned
> > int reg, unsigned int val)
> >  static const struct i2c_device_id rt5677_i2c_id[] = {
> >  	{ "rt5677", RT5677 },
> >  	{ "rt5676", RT5676 },
> > +	{ "RT5677CE:00", RT5677 },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);
> 
> This one looks weird.
> 
> The board code has this 
> 
> sound/soc/intel/boards/bdw-rt5677.c:285:                .codec_name =
> "i2c-RT5677CE:00",
> 
> It's clearly a match to ACPI enumerated I2C slave device.

I suspect that the ACPI data here is less-than-optimal (but it does have
the latest underlying chromeos update).  If you tell me what to run I
can poke the data and confirm.  Thanks!

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> 
> > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > Not all devices with ACPI and this combination of sound devices will
> > > have the required information provided via ACPI.  Reintroduce the I2C
> > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > model.
> > 
> > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > that's not running ChromeOS).  My fault for getting out of the habit
> > > of
> > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > 100% sure if this fix is correct for all cases as I'm only able to
> > > test
> > > my hardware here, and this does fix my laptop.
> > 
> > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > in the module sources") does not fix your issue?
> 
> As that's not in master yet I can't tell.  Can you give me a pointer to
> somewhere?  Thanks!

OK, my bad, it has a different hash upstream, but no, that change
doesn't fix things as I see the problem on top of Linus' tree.  Thanks!

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Wed, 2017-08-23 at 20:05 -0400, Tom Rini wrote:
> On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > 
> > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > Not all devices with ACPI and this combination of sound devices
> > > > will
> > > > have the required information provided via ACPI.  Reintroduce
> > > > the I2C
> > > > device ID to restore sound functionality on on the Chromebook
> > > > 'Samus'
> > > > model.
> > > > This is a regression from v4.12 on my laptop (a Chromebook
> > > > 'Samus'
> > > > that's not running ChromeOS).  My fault for getting out of the
> > > > habit
> > > > of
> > > > trying -rc1 when it comes out and not spotting this sooner.  I'm
> > > > not
> > > > 100% sure if this fix is correct for all cases as I'm only able
> > > > to
> > > > test
> > > > my hardware here, and this does fix my laptop.
> > > 
> > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform
> > > data
> > > in the module sources") does not fix your issue?
> > 
> > As that's not in master yet I can't tell.  Can you give me a pointer
> > to
> > somewhere?

It's in ASoC next at least.

> >   Thanks!
> 
> OK, my bad, it has a different hash upstream, but no, that change
> doesn't fix things as I see the problem on top of Linus'
> tree.  Thanks!

Interesting...

The only bug so far I saw is the following one

https://bugzilla.kernel.org/show_bug.cgi?id=196397

...and above commit fixes it.

Can you place somewhere the bundle of the following:

1. Output file (tables.dat) of
	% acpidump -o tables.dat
2. Output of
	% cat /proc/interrupts
3. Output of
	% lspci -vv -xx
4. Output of
	% grep -H 15 /sys/bus/acpi/devices/*/status

?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Wed, 2017-08-23 at 20:05 -0400, Tom Rini wrote:
> On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > 
> > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > Not all devices with ACPI and this combination of sound devices
> > > > will
> > > > have the required information provided via ACPI.  Reintroduce
> > > > the I2C
> > > > device ID to restore sound functionality on on the Chromebook
> > > > 'Samus'
> > > > model.
> > > > This is a regression from v4.12 on my laptop (a Chromebook
> > > > 'Samus'
> > > > that's not running ChromeOS).  My fault for getting out of the
> > > > habit
> > > > of
> > > > trying -rc1 when it comes out and not spotting this sooner.  I'm
> > > > not
> > > > 100% sure if this fix is correct for all cases as I'm only able
> > > > to
> > > > test
> > > > my hardware here, and this does fix my laptop.
> > > 
> > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform
> > > data
> > > in the module sources") does not fix your issue?
> > 
> > As that's not in master yet I can't tell.  Can you give me a pointer
> > to
> > somewhere?

It's in ASoC next at least.

> >   Thanks!
> 
> OK, my bad, it has a different hash upstream, but no, that change
> doesn't fix things as I see the problem on top of Linus'
> tree.  Thanks!

Interesting...

The only bug so far I saw is the following one

https://bugzilla.kernel.org/show_bug.cgi?id=196397

...and above commit fixes it.

Can you place somewhere the bundle of the following:

1. Output file (tables.dat) of
	% acpidump -o tables.dat
2. Output of
	% cat /proc/interrupts
3. Output of
	% lspci -vv -xx
4. Output of
	% grep -H 15 /sys/bus/acpi/devices/*/status

?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Thu, Aug 24, 2017 at 10:39:09AM +0300, Andy Shevchenko wrote:
> On Wed, 2017-08-23 at 20:05 -0400, Tom Rini wrote:
> > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > 
> > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > Not all devices with ACPI and this combination of sound devices
> > > > > will
> > > > > have the required information provided via ACPI.  Reintroduce
> > > > > the I2C
> > > > > device ID to restore sound functionality on on the Chromebook
> > > > > 'Samus'
> > > > > model.
> > > > > This is a regression from v4.12 on my laptop (a Chromebook
> > > > > 'Samus'
> > > > > that's not running ChromeOS).  My fault for getting out of the
> > > > > habit
> > > > > of
> > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm
> > > > > not
> > > > > 100% sure if this fix is correct for all cases as I'm only able
> > > > > to
> > > > > test
> > > > > my hardware here, and this does fix my laptop.
> > > > 
> > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform
> > > > data
> > > > in the module sources") does not fix your issue?
> > > 
> > > As that's not in master yet I can't tell.  Can you give me a pointer
> > > to
> > > somewhere?
> 
> It's in ASoC next at least.
> 
> > >   Thanks!
> > 
> > OK, my bad, it has a different hash upstream, but no, that change
> > doesn't fix things as I see the problem on top of Linus'
> > tree.  Thanks!
> 
> Interesting...
> 
> The only bug so far I saw is the following one
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=196397
> 
> ...and above commit fixes it.
> 
> Can you place somewhere the bundle of the following:
> 
> 1. Output file (tables.dat) of
> 	% acpidump -o tables.dat

https://gist.github.com/trini/b9a9a547e37cb812506a22e18cd2aff5

> 2. Output of
> 	% cat /proc/interrupts

https://gist.github.com/trini/39a5d88a5ecb9b71336ff5fe9da7c720

> 3. Output of
> 	% lspci -vv -xx

https://gist.github.com/trini/e81ab21909eeaa05ea50dc0596d23c14

> 4. Output of
> 	% grep -H 15 /sys/bus/acpi/devices/*/status

https://gist.github.com/trini/698dc284634ad9e1cfbafad8e705de5f

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 2272 bytes --]

On Thu, Aug 24, 2017 at 10:39:09AM +0300, Andy Shevchenko wrote:
> On Wed, 2017-08-23 at 20:05 -0400, Tom Rini wrote:
> > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > 
> > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > Not all devices with ACPI and this combination of sound devices
> > > > > will
> > > > > have the required information provided via ACPI.  Reintroduce
> > > > > the I2C
> > > > > device ID to restore sound functionality on on the Chromebook
> > > > > 'Samus'
> > > > > model.
> > > > > This is a regression from v4.12 on my laptop (a Chromebook
> > > > > 'Samus'
> > > > > that's not running ChromeOS).  My fault for getting out of the
> > > > > habit
> > > > > of
> > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm
> > > > > not
> > > > > 100% sure if this fix is correct for all cases as I'm only able
> > > > > to
> > > > > test
> > > > > my hardware here, and this does fix my laptop.
> > > > 
> > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform
> > > > data
> > > > in the module sources") does not fix your issue?
> > > 
> > > As that's not in master yet I can't tell.  Can you give me a pointer
> > > to
> > > somewhere?
> 
> It's in ASoC next at least.
> 
> > >   Thanks!
> > 
> > OK, my bad, it has a different hash upstream, but no, that change
> > doesn't fix things as I see the problem on top of Linus'
> > tree.  Thanks!
> 
> Interesting...
> 
> The only bug so far I saw is the following one
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=196397
> 
> ...and above commit fixes it.
> 
> Can you place somewhere the bundle of the following:
> 
> 1. Output file (tables.dat) of
> 	% acpidump -o tables.dat

https://gist.github.com/trini/b9a9a547e37cb812506a22e18cd2aff5

> 2. Output of
> 	% cat /proc/interrupts

https://gist.github.com/trini/39a5d88a5ecb9b71336ff5fe9da7c720

> 3. Output of
> 	% lspci -vv -xx

https://gist.github.com/trini/e81ab21909eeaa05ea50dc0596d23c14

> 4. Output of
> 	% grep -H 15 /sys/bus/acpi/devices/*/status

https://gist.github.com/trini/698dc284634ad9e1cfbafad8e705de5f

-- 
Tom

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Thu, 2017-08-24 at 07:15 -0400, Tom Rini wrote:
> On Thu, Aug 24, 2017 at 10:39:09AM +0300, Andy Shevchenko wrote:
> > On Wed, 2017-08-23 at 20:05 -0400, Tom Rini wrote:
> > > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > > 
> > > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide
> > > > > platform
> > > > > data
> > > > > in the module sources") does not fix your issue?
> > > > 
> > > > As that's not in master yet I can't tell.  Can you give me a
> > > > pointer
> > > > to
> > > > somewhere?
> > 
> > It's in ASoC next at least.
> > 
> > > >   Thanks!
> > > 
> > > OK, my bad, it has a different hash upstream, but no, that change
> > > doesn't fix things as I see the problem on top of Linus'
> > > tree.  Thanks!
> > 
> > Interesting...
> > 
> > The only bug so far I saw is the following one
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=196397
> > 
> > ...and above commit fixes it.
> > 
> > Can you place somewhere the bundle of the following:
> > 
> > 1. Output file (tables.dat) of
> > 	% acpidump -o tables.dat
> 
> https://gist.github.com/trini/b9a9a547e37cb812506a22e18cd2aff5
> 
> > 2. Output of
> > 	% cat /proc/interrupts
> 
> https://gist.github.com/trini/39a5d88a5ecb9b71336ff5fe9da7c720
> 
> > 3. Output of
> > 	% lspci -vv -xx
> 
> https://gist.github.com/trini/e81ab21909eeaa05ea50dc0596d23c14
> 
> > 4. Output of
> > 	% grep -H 15 /sys/bus/acpi/devices/*/status
> 
> https://gist.github.com/trini/698dc284634ad9e1cfbafad8e705de5f

Thanks!

I have checked those files and found that device is in the table and
pretty much properly defined ACPI I2C slave.

The change in the driver you referred to makes the change to modalias.

Thus, I'm suspecting that your user space helper either has some hard
coded values for previous case, or just broken.

Can you also check is the codec module loaded (lsmod) and, if it's not,
load it manually and check if sound works again.

P.S. In any case you need the patch mentioned in bug #196397

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Thu, Aug 24, 2017 at 03:26:00PM +0300, Andy Shevchenko wrote:

> Thus, I'm suspecting that your user space helper either has some hard
> coded values for previous case, or just broken.
> 
> Can you also check is the codec module loaded (lsmod) and, if it's not,
> load it manually and check if sound works again.

Not sure what userspace helper you mean here?  The kernel is oopsing,
userspace shouldn't be able to do that.

> P.S. In any case you need the patch mentioned in bug #196397

What is that?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Thu, Aug 24, 2017 at 03:26:00PM +0300, Andy Shevchenko wrote:
> On Thu, 2017-08-24 at 07:15 -0400, Tom Rini wrote:
> > On Thu, Aug 24, 2017 at 10:39:09AM +0300, Andy Shevchenko wrote:
> > > On Wed, 2017-08-23 at 20:05 -0400, Tom Rini wrote:
> > > > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > > > 
> > > > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide
> > > > > > platform
> > > > > > data
> > > > > > in the module sources") does not fix your issue?
> > > > > 
> > > > > As that's not in master yet I can't tell.  Can you give me a
> > > > > pointer
> > > > > to
> > > > > somewhere?
> > > 
> > > It's in ASoC next at least.
> > > 
> > > > >   Thanks!
> > > > 
> > > > OK, my bad, it has a different hash upstream, but no, that change
> > > > doesn't fix things as I see the problem on top of Linus'
> > > > tree.  Thanks!
> > > 
> > > Interesting...
> > > 
> > > The only bug so far I saw is the following one
> > > 
> > > https://bugzilla.kernel.org/show_bug.cgi?id=196397
> > > 
> > > ...and above commit fixes it.
> > > 
> > > Can you place somewhere the bundle of the following:
> > > 
> > > 1. Output file (tables.dat) of
> > > 	% acpidump -o tables.dat
> > 
> > https://gist.github.com/trini/b9a9a547e37cb812506a22e18cd2aff5
> > 
> > > 2. Output of
> > > 	% cat /proc/interrupts
> > 
> > https://gist.github.com/trini/39a5d88a5ecb9b71336ff5fe9da7c720
> > 
> > > 3. Output of
> > > 	% lspci -vv -xx
> > 
> > https://gist.github.com/trini/e81ab21909eeaa05ea50dc0596d23c14
> > 
> > > 4. Output of
> > > 	% grep -H 15 /sys/bus/acpi/devices/*/status
> > 
> > https://gist.github.com/trini/698dc284634ad9e1cfbafad8e705de5f
> 
> Thanks!
> 
> I have checked those files and found that device is in the table and
> pretty much properly defined ACPI I2C slave.
> 
> The change in the driver you referred to makes the change to modalias.
> 
> Thus, I'm suspecting that your user space helper either has some hard
> coded values for previous case, or just broken.
> 
> Can you also check is the codec module loaded (lsmod) and, if it's not,
> load it manually and check if sound works again.
> 
> P.S. In any case you need the patch mentioned in bug #196397

I've applied "ASoC: rt5677: Hide platform data in the module sources"
manually to Linus' tree and still see the same problem.  Perhaps there's
now some missing alias information that really needs to still be
provided?  snd-soc-sst-bdw-rt5677-mach is not loaded and I can't
manually load it later on.  What user space helper configuration am I
supposed to need to have now, for this to work?  Thanks!

-- 
Tom

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

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 02:05:25 +0200,
Tom Rini wrote:
> 
> On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > 
> > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > Not all devices with ACPI and this combination of sound devices will
> > > > have the required information provided via ACPI.  Reintroduce the I2C
> > > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > > model.
> > > 
> > > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > > that's not running ChromeOS).  My fault for getting out of the habit
> > > > of
> > > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > > 100% sure if this fix is correct for all cases as I'm only able to
> > > > test
> > > > my hardware here, and this does fix my laptop.
> > > 
> > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > > in the module sources") does not fix your issue?
> > 
> > As that's not in master yet I can't tell.  Can you give me a pointer to
> > somewhere?  Thanks!
> 
> OK, my bad, it has a different hash upstream, but no, that change
> doesn't fix things as I see the problem on top of Linus' tree.  Thanks!

Could you double-check?  Your Oops likely comes from the NULL
id->driver_type reference that is removed by the commit ddc9e69b9dc2.

If you still get an Oops, we need to decode it properly now to
understand what's going on.


thanks,

Takashi

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 16:28:29 +0200,
Takashi Iwai wrote:
> 
> On Thu, 24 Aug 2017 02:05:25 +0200,
> Tom Rini wrote:
> > 
> > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > 
> > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > Not all devices with ACPI and this combination of sound devices will
> > > > > have the required information provided via ACPI.  Reintroduce the I2C
> > > > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > > > model.
> > > > 
> > > > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > > > that's not running ChromeOS).  My fault for getting out of the habit
> > > > > of
> > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > > > 100% sure if this fix is correct for all cases as I'm only able to
> > > > > test
> > > > > my hardware here, and this does fix my laptop.
> > > > 
> > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > > > in the module sources") does not fix your issue?
> > > 
> > > As that's not in master yet I can't tell.  Can you give me a pointer to
> > > somewhere?  Thanks!
> > 
> > OK, my bad, it has a different hash upstream,

BTW, the hash above is correct.  It's in Mark's asoc tree (and in
linux-next).  You might have cherry-picked a wrong one, I suppose?


Takashi

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 16:28:29 +0200,
Takashi Iwai wrote:
> 
> On Thu, 24 Aug 2017 02:05:25 +0200,
> Tom Rini wrote:
> > 
> > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > 
> > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > Not all devices with ACPI and this combination of sound devices will
> > > > > have the required information provided via ACPI.  Reintroduce the I2C
> > > > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > > > model.
> > > > 
> > > > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > > > that's not running ChromeOS).  My fault for getting out of the habit
> > > > > of
> > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > > > 100% sure if this fix is correct for all cases as I'm only able to
> > > > > test
> > > > > my hardware here, and this does fix my laptop.
> > > > 
> > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > > > in the module sources") does not fix your issue?
> > > 
> > > As that's not in master yet I can't tell.  Can you give me a pointer to
> > > somewhere?  Thanks!
> > 
> > OK, my bad, it has a different hash upstream,

BTW, the hash above is correct.  It's in Mark's asoc tree (and in
linux-next).  You might have cherry-picked a wrong one, I suppose?


Takashi

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Thu, Aug 24, 2017 at 04:31:25PM +0200, Takashi Iwai wrote:
> On Thu, 24 Aug 2017 16:28:29 +0200,
> Takashi Iwai wrote:
> > 
> > On Thu, 24 Aug 2017 02:05:25 +0200,
> > Tom Rini wrote:
> > > 
> > > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > > 
> > > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > > Not all devices with ACPI and this combination of sound devices will
> > > > > > have the required information provided via ACPI.  Reintroduce the I2C
> > > > > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > > > > model.
> > > > > 
> > > > > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > > > > that's not running ChromeOS).  My fault for getting out of the habit
> > > > > > of
> > > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > > > > 100% sure if this fix is correct for all cases as I'm only able to
> > > > > > test
> > > > > > my hardware here, and this does fix my laptop.
> > > > > 
> > > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > > > > in the module sources") does not fix your issue?
> > > > 
> > > > As that's not in master yet I can't tell.  Can you give me a pointer to
> > > > somewhere?  Thanks!
> > > 
> > > OK, my bad, it has a different hash upstream,
> 
> BTW, the hash above is correct.  It's in Mark's asoc tree (and in
> linux-next).  You might have cherry-picked a wrong one, I suppose?

Alright, I read-things to quickly, and to be clear:
(a) "ASoC: rt5677: Hide platform data in the module sources" is _not_ in
Linus' tree (I confused this with a different commit) and _is_ in Mark's
ASoC for-next branch currently.
(b) Applying just that patch on top of Linus' tree _does_ fix my
regression (an Oops and non-functional audio) with audio and now sound
works well, as expected.

Can we please get that as a fix for this release?  Thanks!

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 2063 bytes --]

On Thu, Aug 24, 2017 at 04:31:25PM +0200, Takashi Iwai wrote:
> On Thu, 24 Aug 2017 16:28:29 +0200,
> Takashi Iwai wrote:
> > 
> > On Thu, 24 Aug 2017 02:05:25 +0200,
> > Tom Rini wrote:
> > > 
> > > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > > 
> > > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > > Not all devices with ACPI and this combination of sound devices will
> > > > > > have the required information provided via ACPI.  Reintroduce the I2C
> > > > > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > > > > model.
> > > > > 
> > > > > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > > > > that's not running ChromeOS).  My fault for getting out of the habit
> > > > > > of
> > > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > > > > 100% sure if this fix is correct for all cases as I'm only able to
> > > > > > test
> > > > > > my hardware here, and this does fix my laptop.
> > > > > 
> > > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > > > > in the module sources") does not fix your issue?
> > > > 
> > > > As that's not in master yet I can't tell.  Can you give me a pointer to
> > > > somewhere?  Thanks!
> > > 
> > > OK, my bad, it has a different hash upstream,
> 
> BTW, the hash above is correct.  It's in Mark's asoc tree (and in
> linux-next).  You might have cherry-picked a wrong one, I suppose?

Alright, I read-things to quickly, and to be clear:
(a) "ASoC: rt5677: Hide platform data in the module sources" is _not_ in
Linus' tree (I confused this with a different commit) and _is_ in Mark's
ASoC for-next branch currently.
(b) Applying just that patch on top of Linus' tree _does_ fix my
regression (an Oops and non-functional audio) with audio and now sound
works well, as expected.

Can we please get that as a fix for this release?  Thanks!

-- 
Tom

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 16:41:52 +0200,
Tom Rini wrote:
> 
> On Thu, Aug 24, 2017 at 04:31:25PM +0200, Takashi Iwai wrote:
> > On Thu, 24 Aug 2017 16:28:29 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Thu, 24 Aug 2017 02:05:25 +0200,
> > > Tom Rini wrote:
> > > > 
> > > > On Wed, Aug 23, 2017 at 01:39:12PM -0400, Tom Rini wrote:
> > > > > On Wed, Aug 23, 2017 at 05:29:33PM +0300, Andy Shevchenko wrote:
> > > > > 
> > > > > > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > > > > > Not all devices with ACPI and this combination of sound devices will
> > > > > > > have the required information provided via ACPI.  Reintroduce the I2C
> > > > > > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > > > > > model.
> > > > > > 
> > > > > > > This is a regression from v4.12 on my laptop (a Chromebook 'Samus'
> > > > > > > that's not running ChromeOS).  My fault for getting out of the habit
> > > > > > > of
> > > > > > > trying -rc1 when it comes out and not spotting this sooner.  I'm not
> > > > > > > 100% sure if this fix is correct for all cases as I'm only able to
> > > > > > > test
> > > > > > > my hardware here, and this does fix my laptop.
> > > > > > 
> > > > > > Are you sure the commit ddc9e69b9dc2 ("ASoC: rt5677: Hide platform data
> > > > > > in the module sources") does not fix your issue?
> > > > > 
> > > > > As that's not in master yet I can't tell.  Can you give me a pointer to
> > > > > somewhere?  Thanks!
> > > > 
> > > > OK, my bad, it has a different hash upstream,
> > 
> > BTW, the hash above is correct.  It's in Mark's asoc tree (and in
> > linux-next).  You might have cherry-picked a wrong one, I suppose?
> 
> Alright, I read-things to quickly, and to be clear:
> (a) "ASoC: rt5677: Hide platform data in the module sources" is _not_ in
> Linus' tree (I confused this with a different commit) and _is_ in Mark's
> ASoC for-next branch currently.
> (b) Applying just that patch on top of Linus' tree _does_ fix my
> regression (an Oops and non-functional audio) with audio and now sound
> works well, as expected.
> 
> Can we please get that as a fix for this release?  Thanks!

OK, so the fix for 4.13 would be either to cherry-pick this commit, or
just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
fix (and remove again in 4.14).

The former is cleaner, but it's bigger, while the latter is a safer
oneliner at the late RC stage.

I leave the decision to Mark.


Takashi

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:

> OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> fix (and remove again in 4.14).

> The former is cleaner, but it's bigger, while the latter is a safer
> oneliner at the late RC stage.

> I leave the decision to Mark.

I'm happier with the oneline change TBH, like you say it's pretty late
in the release cycle.  Can you just apply the patch directly and send it
to Linus with my ack or should I put together a pull request?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 587 bytes --]

On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:

> OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> fix (and remove again in 4.14).

> The former is cleaner, but it's bigger, while the latter is a safer
> oneliner at the late RC stage.

> I leave the decision to Mark.

I'm happier with the oneline change TBH, like you say it's pretty late
in the release cycle.  Can you just apply the patch directly and send it
to Linus with my ack or should I put together a pull request?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 17:52:35 +0200,
Mark Brown wrote:
> 
> On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> 
> > OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> > fix (and remove again in 4.14).
> 
> > The former is cleaner, but it's bigger, while the latter is a safer
> > oneliner at the late RC stage.
> 
> > I leave the decision to Mark.
> 
> I'm happier with the oneline change TBH, like you say it's pretty late
> in the release cycle.  Can you just apply the patch directly and send it
> to Linus with my ack or should I put together a pull request?

OK, no problem, I'll add Tom's patch with a bit more explanations.


Takashi

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> 
> > OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> > fix (and remove again in 4.14).
> 
> > The former is cleaner, but it's bigger, while the latter is a safer
> > oneliner at the late RC stage.
> 
> > I leave the decision to Mark.
> 
> I'm happier with the oneline change TBH, like you say it's pretty late
> in the release cycle.  Can you just apply the patch directly and send it
> to Linus with my ack or should I put together a pull request?

FWIW, I'd be happy to give the change a quick spin and Tested-by it.

-- 
Tom

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

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 17:54:37 +0200,
Tom Rini wrote:
> 
> On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> > On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> > 
> > > OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> > > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> > > fix (and remove again in 4.14).
> > 
> > > The former is cleaner, but it's bigger, while the latter is a safer
> > > oneliner at the late RC stage.
> > 
> > > I leave the decision to Mark.
> > 
> > I'm happier with the oneline change TBH, like you say it's pretty late
> > in the release cycle.  Can you just apply the patch directly and send it
> > to Linus with my ack or should I put together a pull request?
> 
> FWIW, I'd be happy to give the change a quick spin and Tested-by it.

Well, it's your patch, after all :)
Below is the patch I'm going to queue.


Takashi

-- 8< --
From: Tom Rini <trini@konsulko.com>
Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

Not all devices with ACPI and this combination of sound devices will
have the required information provided via ACPI.  Reintroduce the I2C
device ID to restore sound functionality on on the Chromebook 'Samus'
model.

[ More background note:
 the commit a36afb0ab648 ("ASoC: rt5677: Introduce proper table...")
 moved the i2c ID probed via ACPI ("RT5677CE:00") to a proper
 acpi_device_id table.  Although the action itself is correct per se,
 the overseen issue is the reference id->driver_data at
 rt5677_i2c_probe() for retrieving the corresponding chip model for
 the given id.  Since id=NULL is passed for ACPI matching case, we get
 an Oops now.

 We already have queued more fixes for 4.14 and they already address
 the issue, but they are bigger changes that aren't preferable for the
 late 4.13-rc stage.  So, this patch just papers over the bug as a
 once-off quick fix for a particular ACPI matching.  -- tiwai ]

Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI enumeration")
Signed-off-by: Tom Rini <trini@konsulko.com>
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/rt5677.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 36e530a36c82..6f629278d982 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -5021,6 +5021,7 @@ static const struct regmap_config rt5677_regmap = {
 static const struct i2c_device_id rt5677_i2c_id[] = {
 	{ "rt5677", RT5677 },
 	{ "rt5676", RT5676 },
+	{ "RT5677CE:00", RT5677 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);
-- 
2.14.0

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 17:54:37 +0200,
Tom Rini wrote:
> 
> On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> > On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> > 
> > > OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> > > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> > > fix (and remove again in 4.14).
> > 
> > > The former is cleaner, but it's bigger, while the latter is a safer
> > > oneliner at the late RC stage.
> > 
> > > I leave the decision to Mark.
> > 
> > I'm happier with the oneline change TBH, like you say it's pretty late
> > in the release cycle.  Can you just apply the patch directly and send it
> > to Linus with my ack or should I put together a pull request?
> 
> FWIW, I'd be happy to give the change a quick spin and Tested-by it.

Well, it's your patch, after all :)
Below is the patch I'm going to queue.


Takashi

-- 8< --
From: Tom Rini <trini@konsulko.com>
Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

Not all devices with ACPI and this combination of sound devices will
have the required information provided via ACPI.  Reintroduce the I2C
device ID to restore sound functionality on on the Chromebook 'Samus'
model.

[ More background note:
 the commit a36afb0ab648 ("ASoC: rt5677: Introduce proper table...")
 moved the i2c ID probed via ACPI ("RT5677CE:00") to a proper
 acpi_device_id table.  Although the action itself is correct per se,
 the overseen issue is the reference id->driver_data at
 rt5677_i2c_probe() for retrieving the corresponding chip model for
 the given id.  Since id=NULL is passed for ACPI matching case, we get
 an Oops now.

 We already have queued more fixes for 4.14 and they already address
 the issue, but they are bigger changes that aren't preferable for the
 late 4.13-rc stage.  So, this patch just papers over the bug as a
 once-off quick fix for a particular ACPI matching.  -- tiwai ]

Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI enumeration")
Signed-off-by: Tom Rini <trini@konsulko.com>
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/rt5677.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 36e530a36c82..6f629278d982 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -5021,6 +5021,7 @@ static const struct regmap_config rt5677_regmap = {
 static const struct i2c_device_id rt5677_i2c_id[] = {
 	{ "rt5677", RT5677 },
 	{ "rt5676", RT5676 },
+	{ "RT5677CE:00", RT5677 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);
-- 
2.14.0

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Thu, Aug 24, 2017 at 06:06:20PM +0200, Takashi Iwai wrote:
> On Thu, 24 Aug 2017 17:54:37 +0200,
> Tom Rini wrote:
> > 
> > On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> > > On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> > > 
> > > > OK, so the fix for 4.13 would be either to cherry-pick this commit, or
> > > > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick band-aid
> > > > fix (and remove again in 4.14).
> > > 
> > > > The former is cleaner, but it's bigger, while the latter is a safer
> > > > oneliner at the late RC stage.
> > > 
> > > > I leave the decision to Mark.
> > > 
> > > I'm happier with the oneline change TBH, like you say it's pretty late
> > > in the release cycle.  Can you just apply the patch directly and send it
> > > to Linus with my ack or should I put together a pull request?
> > 
> > FWIW, I'd be happy to give the change a quick spin and Tested-by it.
> 
> Well, it's your patch, after all :)
> Below is the patch I'm going to queue.
> 
> 
> Takashi
> 
> -- 8< --
> From: Tom Rini <trini@konsulko.com>
> Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs
> 
> Not all devices with ACPI and this combination of sound devices will
> have the required information provided via ACPI.  Reintroduce the I2C
> device ID to restore sound functionality on on the Chromebook 'Samus'
> model.
> 
> [ More background note:
>  the commit a36afb0ab648 ("ASoC: rt5677: Introduce proper table...")
>  moved the i2c ID probed via ACPI ("RT5677CE:00") to a proper
>  acpi_device_id table.  Although the action itself is correct per se,
>  the overseen issue is the reference id->driver_data at
>  rt5677_i2c_probe() for retrieving the corresponding chip model for
>  the given id.  Since id=NULL is passed for ACPI matching case, we get
>  an Oops now.
> 
>  We already have queued more fixes for 4.14 and they already address
>  the issue, but they are bigger changes that aren't preferable for the
>  late 4.13-rc stage.  So, this patch just papers over the bug as a
>  once-off quick fix for a particular ACPI matching.  -- tiwai ]
> 
> Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI enumeration")
> Signed-off-by: Tom Rini <trini@konsulko.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/soc/codecs/rt5677.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
> index 36e530a36c82..6f629278d982 100644
> --- a/sound/soc/codecs/rt5677.c
> +++ b/sound/soc/codecs/rt5677.c
> @@ -5021,6 +5021,7 @@ static const struct regmap_config rt5677_regmap = {
>  static const struct i2c_device_id rt5677_i2c_id[] = {
>  	{ "rt5677", RT5677 },
>  	{ "rt5676", RT5676 },
> +	{ "RT5677CE:00", RT5677 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);

Looks good, thanks for rewording things!

-- 
Tom

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

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Thu, 2017-08-24 at 18:06 +0200, Takashi Iwai wrote:
> On Thu, 24 Aug 2017 17:54:37 +0200,
> Tom Rini wrote:
> > 
> > On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> > > On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> > > 
> > > > OK, so the fix for 4.13 would be either to cherry-pick this
> > > > commit, or
> > > > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick
> > > > band-aid
> > > > fix (and remove again in 4.14).
> > > > The former is cleaner, but it's bigger, while the latter is a
> > > > safer
> > > > oneliner at the late RC stage.
> > > > I leave the decision to Mark.
> > > 
> > > I'm happier with the oneline change TBH, like you say it's pretty
> > > late
> > > in the release cycle.  Can you just apply the patch directly and
> > > send it
> > > to Linus with my ack or should I put together a pull request?
> > 
> > FWIW, I'd be happy to give the change a quick spin and Tested-by it.
> 
> Well, it's your patch, after all :)
> Below is the patch I'm going to queue.
> 
> 
> Takashi
> 
> -- 8< --
> From: Tom Rini <trini@konsulko.com>
> Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs
> 
> Not all devices with ACPI and this combination of sound devices will
> have the required information provided via ACPI.  Reintroduce the I2C
> device ID to restore sound functionality on on the Chromebook 'Samus'
> model.
> 
> [ More background note:
>  the commit a36afb0ab648 ("ASoC: rt5677: Introduce proper table...")
>  moved the i2c ID probed via ACPI ("RT5677CE:00") to a proper
>  acpi_device_id table.  Although the action itself is correct per se,
>  the overseen issue is the reference id->driver_data at
>  rt5677_i2c_probe() for retrieving the corresponding chip model for
>  the given id.  Since id=NULL is passed for ACPI matching case, we get
>  an Oops now.
> 
>  We already have queued more fixes for 4.14 and they already address
>  the issue, but they are bigger changes that aren't preferable for the
>  late 4.13-rc stage.  So, this patch just papers over the bug as a
>  once-off quick fix for a particular ACPI matching.  -- tiwai ]
> 
> Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI
> enumeration")
> Signed-off-by: Tom Rini <trini@konsulko.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Thanks for this and sorry for bisectability issue. I didn't noticed it
before Takashi got my attention to the bug report.

I'm fine with this quick fix for v4.13 only.

> ---
>  sound/soc/codecs/rt5677.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
> index 36e530a36c82..6f629278d982 100644
> --- a/sound/soc/codecs/rt5677.c
> +++ b/sound/soc/codecs/rt5677.c
> @@ -5021,6 +5021,7 @@ static const struct regmap_config rt5677_regmap
> = {
>  static const struct i2c_device_id rt5677_i2c_id[] = {
>  	{ "rt5677", RT5677 },
>  	{ "rt5676", RT5676 },
> +	{ "RT5677CE:00", RT5677 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 19:16:11 +0200,
Andy Shevchenko wrote:
> 
> On Thu, 2017-08-24 at 18:06 +0200, Takashi Iwai wrote:
> > On Thu, 24 Aug 2017 17:54:37 +0200,
> > Tom Rini wrote:
> > > 
> > > On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> > > > On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> > > > 
> > > > > OK, so the fix for 4.13 would be either to cherry-pick this
> > > > > commit, or
> > > > > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick
> > > > > band-aid
> > > > > fix (and remove again in 4.14).
> > > > > The former is cleaner, but it's bigger, while the latter is a
> > > > > safer
> > > > > oneliner at the late RC stage.
> > > > > I leave the decision to Mark.
> > > > 
> > > > I'm happier with the oneline change TBH, like you say it's pretty
> > > > late
> > > > in the release cycle.  Can you just apply the patch directly and
> > > > send it
> > > > to Linus with my ack or should I put together a pull request?
> > > 
> > > FWIW, I'd be happy to give the change a quick spin and Tested-by it.
> > 
> > Well, it's your patch, after all :)
> > Below is the patch I'm going to queue.
> > 
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Tom Rini <trini@konsulko.com>
> > Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs
> > 
> > Not all devices with ACPI and this combination of sound devices will
> > have the required information provided via ACPI.  Reintroduce the I2C
> > device ID to restore sound functionality on on the Chromebook 'Samus'
> > model.
> > 
> > [ More background note:
> >  the commit a36afb0ab648 ("ASoC: rt5677: Introduce proper table...")
> >  moved the i2c ID probed via ACPI ("RT5677CE:00") to a proper
> >  acpi_device_id table.  Although the action itself is correct per se,
> >  the overseen issue is the reference id->driver_data at
> >  rt5677_i2c_probe() for retrieving the corresponding chip model for
> >  the given id.  Since id=NULL is passed for ACPI matching case, we get
> >  an Oops now.
> > 
> >  We already have queued more fixes for 4.14 and they already address
> >  the issue, but they are bigger changes that aren't preferable for the
> >  late 4.13-rc stage.  So, this patch just papers over the bug as a
> >  once-off quick fix for a particular ACPI matching.  -- tiwai ]
> > 
> > Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI
> > enumeration")
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Thanks for this and sorry for bisectability issue. I didn't noticed it
> before Takashi got my attention to the bug report.
> 
> I'm fine with this quick fix for v4.13 only.

Good to hear.  Now I merged to for-linus branch and pushed out.

Mark, could you pull my for-linus branch into your rt5677 branch,
before Stephen grumbles on the merge conflicts?


thanks,

Takashi

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Thu, 24 Aug 2017 19:16:11 +0200,
Andy Shevchenko wrote:
> 
> On Thu, 2017-08-24 at 18:06 +0200, Takashi Iwai wrote:
> > On Thu, 24 Aug 2017 17:54:37 +0200,
> > Tom Rini wrote:
> > > 
> > > On Thu, Aug 24, 2017 at 04:52:35PM +0100, Mark Brown wrote:
> > > > On Thu, Aug 24, 2017 at 05:42:11PM +0200, Takashi Iwai wrote:
> > > > 
> > > > > OK, so the fix for 4.13 would be either to cherry-pick this
> > > > > commit, or
> > > > > just to re-add "RT5677CE:00" to i2c_id temporarily as a quick
> > > > > band-aid
> > > > > fix (and remove again in 4.14).
> > > > > The former is cleaner, but it's bigger, while the latter is a
> > > > > safer
> > > > > oneliner at the late RC stage.
> > > > > I leave the decision to Mark.
> > > > 
> > > > I'm happier with the oneline change TBH, like you say it's pretty
> > > > late
> > > > in the release cycle.  Can you just apply the patch directly and
> > > > send it
> > > > to Linus with my ack or should I put together a pull request?
> > > 
> > > FWIW, I'd be happy to give the change a quick spin and Tested-by it.
> > 
> > Well, it's your patch, after all :)
> > Below is the patch I'm going to queue.
> > 
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Tom Rini <trini@konsulko.com>
> > Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs
> > 
> > Not all devices with ACPI and this combination of sound devices will
> > have the required information provided via ACPI.  Reintroduce the I2C
> > device ID to restore sound functionality on on the Chromebook 'Samus'
> > model.
> > 
> > [ More background note:
> >  the commit a36afb0ab648 ("ASoC: rt5677: Introduce proper table...")
> >  moved the i2c ID probed via ACPI ("RT5677CE:00") to a proper
> >  acpi_device_id table.  Although the action itself is correct per se,
> >  the overseen issue is the reference id->driver_data at
> >  rt5677_i2c_probe() for retrieving the corresponding chip model for
> >  the given id.  Since id=NULL is passed for ACPI matching case, we get
> >  an Oops now.
> > 
> >  We already have queued more fixes for 4.14 and they already address
> >  the issue, but they are bigger changes that aren't preferable for the
> >  late 4.13-rc stage.  So, this patch just papers over the bug as a
> >  once-off quick fix for a particular ACPI matching.  -- tiwai ]
> > 
> > Fixes: a36afb0ab648 ("ASoC: rt5677: Introduce proper table for ACPI
> > enumeration")
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Thanks for this and sorry for bisectability issue. I didn't noticed it
> before Takashi got my attention to the bug report.
> 
> I'm fine with this quick fix for v4.13 only.

Good to hear.  Now I merged to for-linus branch and pushed out.

Mark, could you pull my for-linus branch into your rt5677 branch,
before Stephen grumbles on the merge conflicts?


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Thu, Aug 24, 2017 at 07:44:07PM +0200, Takashi Iwai wrote:

> Good to hear.  Now I merged to for-linus branch and pushed out.

> Mark, could you pull my for-linus branch into your rt5677 branch,
> before Stephen grumbles on the merge conflicts?

Done now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 259 bytes --]

On Thu, Aug 24, 2017 at 07:44:07PM +0200, Takashi Iwai wrote:

> Good to hear.  Now I merged to for-linus branch and pushed out.

> Mark, could you pull my for-linus branch into your rt5677 branch,
> before Stephen grumbles on the merge conflicts?

Done now.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

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

On Thu, Aug 24, 2017 at 06:06:20PM +0200, Takashi Iwai wrote:

> From: Tom Rini <trini@konsulko.com>
> Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

Acked-by: Mark Brown <broonie@kernel.org>

Can you send me a pull request for -next so I can revert it with the
better fix being there please?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 305 bytes --]

On Thu, Aug 24, 2017 at 06:06:20PM +0200, Takashi Iwai wrote:

> From: Tom Rini <trini@konsulko.com>
> Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

Acked-by: Mark Brown <broonie@kernel.org>

Can you send me a pull request for -next so I can revert it with the
better fix being there please?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Fri, 25 Aug 2017 15:09:00 +0200,
Mark Brown wrote:
> 
> On Thu, Aug 24, 2017 at 06:06:20PM +0200, Takashi Iwai wrote:
> 
> > From: Tom Rini <trini@konsulko.com>
> > Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs
> 
> Acked-by: Mark Brown <broonie@kernel.org>
> 
> Can you send me a pull request for -next so I can revert it with the
> better fix being there please?

Could you just pull for-linus branch of my tree?
Or you can pull tags/sound-4.13-rc7 tag instead, too.


thanks,

Takashi

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Takashi Iwai]

On Fri, 25 Aug 2017 15:09:00 +0200,
Mark Brown wrote:
> 
> On Thu, Aug 24, 2017 at 06:06:20PM +0200, Takashi Iwai wrote:
> 
> > From: Tom Rini <trini@konsulko.com>
> > Subject: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs
> 
> Acked-by: Mark Brown <broonie@kernel.org>
> 
> Can you send me a pull request for -next so I can revert it with the
> better fix being there please?

Could you just pull for-linus branch of my tree?
Or you can pull tags/sound-4.13-rc7 tag instead, too.


thanks,

Takashi

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

+John

On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> Not all devices with ACPI and this combination of sound devices will
> have the required information provided via ACPI.  Reintroduce the I2C
> device ID to restore sound functionality on on the Chromebook 'Samus'
> model.

Tom, one more question.

Apparently you are the one who tested the commit
	89128534f925 ("ASoC: rt5677: Add ACPI support")
year ago.

The commit states that ACPI properties that are used in Chromebook Pixel
2015 is non-standard (not the same as for DT).

However, DSDT shows the opposite!

I would like to ask yuo and John what is the status of that currently?
Do we have any publicly available laptop with non-standard properties?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:
> +John
> 
> On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > Not all devices with ACPI and this combination of sound devices will
> > have the required information provided via ACPI.  Reintroduce the I2C
> > device ID to restore sound functionality on on the Chromebook 'Samus'
> > model.
> 
> Tom, one more question.
> 
> Apparently you are the one who tested the commit
> 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> year ago.

Yes.

> The commit states that ACPI properties that are used in Chromebook Pixel
> 2015 is non-standard (not the same as for DT).
> 
> However, DSDT shows the opposite!

Interesting.  I'm not an ACPI person, I just tested what John came up
with.

> I would like to ask yuo and John what is the status of that currently?
> Do we have any publicly available laptop with non-standard properties?

Is there any sort of "build date" or similar in the dump I provided
yesterday?  Every once in a while my laptop accidentally books into
ChromeOS and then it might grab and apply some updates and it's not
impossible that Google updated things in the interim.

I'm quite happy to test patches or provide further dumps / etc from my
system.  You might want to start by talking with the person behind
https://github.com/raphael/linux-samus to see if they know more about
different versions of the hardware or at least point you towards more
testers.  Thanks!

-- 
Tom

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

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 1507 bytes --]

On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:
> +John
> 
> On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > Not all devices with ACPI and this combination of sound devices will
> > have the required information provided via ACPI.  Reintroduce the I2C
> > device ID to restore sound functionality on on the Chromebook 'Samus'
> > model.
> 
> Tom, one more question.
> 
> Apparently you are the one who tested the commit
> 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> year ago.

Yes.

> The commit states that ACPI properties that are used in Chromebook Pixel
> 2015 is non-standard (not the same as for DT).
> 
> However, DSDT shows the opposite!

Interesting.  I'm not an ACPI person, I just tested what John came up
with.

> I would like to ask yuo and John what is the status of that currently?
> Do we have any publicly available laptop with non-standard properties?

Is there any sort of "build date" or similar in the dump I provided
yesterday?  Every once in a while my laptop accidentally books into
ChromeOS and then it might grab and apply some updates and it's not
impossible that Google updated things in the interim.

I'm quite happy to test patches or provide further dumps / etc from my
system.  You might want to start by talking with the person behind
https://github.com/raphael/linux-samus to see if they know more about
different versions of the hardware or at least point you towards more
testers.  Thanks!

-- 
Tom

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Fri, 2017-08-25 at 10:24 -0400, Tom Rini wrote:
> On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:
> > +John
> > 
> > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:
> > > Not all devices with ACPI and this combination of sound devices
> > > will
> > > have the required information provided via ACPI.  Reintroduce the
> > > I2C
> > > device ID to restore sound functionality on on the Chromebook
> > > 'Samus'
> > > model.
> > 
> > Tom, one more question.

Just to be clear, the below has nothing to do with this patch or my
patches against rt5677.c. It points to a possible separate issue.

> > 
> > Apparently you are the one who tested the commit
> > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > year ago.
> 
> Yes.
> 
> > The commit states that ACPI properties that are used in Chromebook
> > Pixel
> > 2015 is non-standard (not the same as for DT).
> > 
> > However, DSDT shows the opposite!
> 
> Interesting.  I'm not an ACPI person, I just tested what John came up
> with.
> 
> > I would like to ask yuo and John what is the status of that
> > currently?
> > Do we have any publicly available laptop with non-standard
> > properties?
> 
> Is there any sort of "build date" or similar in the dump I provided
> yesterday?

Header has this

 *     Signature        "DSDT"
 *     Length           0x00004720 (18208)
 *     Revision         0x02
 *     Checksum         0x6E
 *     OEM ID           "COREv4"
 *     OEM Table ID     "COREBOOT"
 *     OEM Revision     0x20110725 (537986853)
 *     Compiler ID      "INTL"
 *     Compiler Version 0x20130117 (538116375)

...if it's ever changed.

>   Every once in a while my laptop accidentally books into
> ChromeOS and then it might grab and apply some updates and it's not
> impossible that Google updated things in the interim.
> 
> I'm quite happy to test patches or provide further dumps / etc from my
> system.  You might want to start by talking with the person behind
> https://github.com/raphael/linux-samus to see if they know more about
> different versions of the hardware or at least point you towards more
> testers.  Thanks!

It's just a heads up to point to a potential problem with this board. I
suspect Google would take care of this.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[John Keeping]

On Fri, 25 Aug 2017 10:24:26 -0400, Tom Rini wrote:

> On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:
> > +John
> > 
> > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:  
> > > Not all devices with ACPI and this combination of sound devices will
> > > have the required information provided via ACPI.  Reintroduce the I2C
> > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > model.  
> > 
> > Tom, one more question.
> > 
> > Apparently you are the one who tested the commit
> > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > year ago.  
> 
> Yes.
> 
> > The commit states that ACPI properties that are used in Chromebook Pixel
> > 2015 is non-standard (not the same as for DT).
> > 
> > However, DSDT shows the opposite!  
> 
> Interesting.  I'm not an ACPI person, I just tested what John came up
> with.

And the patch adding this was the first (and still only) time I've
really looked at ACPI, so it's quite possible that I misunderstood
something at the time.

>From memory, I think the particular problem I was referring to in the
commit message was that certain GPIOs were only defined by index and not
by property name (specifically "plug-det-gpios", "mic-present-gpios" and
"headphone-enable-gpios"), and having dumped DSDT just now I do not see
those strings appearing anywhere.

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[John Keeping]

On Fri, 25 Aug 2017 10:24:26 -0400, Tom Rini wrote:

> On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:
> > +John
> > 
> > On Tue, 2017-08-22 at 21:51 -0400, Tom Rini wrote:  
> > > Not all devices with ACPI and this combination of sound devices will
> > > have the required information provided via ACPI.  Reintroduce the I2C
> > > device ID to restore sound functionality on on the Chromebook 'Samus'
> > > model.  
> > 
> > Tom, one more question.
> > 
> > Apparently you are the one who tested the commit
> > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > year ago.  
> 
> Yes.
> 
> > The commit states that ACPI properties that are used in Chromebook Pixel
> > 2015 is non-standard (not the same as for DT).
> > 
> > However, DSDT shows the opposite!  
> 
> Interesting.  I'm not an ACPI person, I just tested what John came up
> with.

And the patch adding this was the first (and still only) time I've
really looked at ACPI, so it's quite possible that I misunderstood
something at the time.

From memory, I think the particular problem I was referring to in the
commit message was that certain GPIOs were only defined by index and not
by property name (specifically "plug-det-gpios", "mic-present-gpios" and
"headphone-enable-gpios"), and having dumped DSDT just now I do not see
those strings appearing anywhere.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Fri, 2017-08-25 at 17:05 +0100, John Keeping wrote:
> On Fri, 25 Aug 2017 10:24:26 -0400, Tom Rini wrote:
> > On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:

> > > Apparently you are the one who tested the commit
> > > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > > year ago.  
> > 
> > Yes.
> > 
> > > The commit states that ACPI properties that are used in Chromebook
> > > Pixel
> > > 2015 is non-standard (not the same as for DT).
> > > 
> > > However, DSDT shows the opposite!  
> > 
> > Interesting.  I'm not an ACPI person, I just tested what John came
> > up
> > with.
> 
> And the patch adding this was the first (and still only) time I've
> really looked at ACPI, so it's quite possible that I misunderstood
> something at the time.

Maybe.

> From memory, I think the particular problem I was referring to in the
> commit message was that certain GPIOs were only defined by index and
> not
> by property name (specifically "plug-det-gpios", "mic-present-gpios"
> and
> "headphone-enable-gpios"), and having dumped DSDT just now I do not
> see
> those strings appearing anywhere.

Exactly, and this part of the patch I'm _not_ talking about (it's pretty
much good and working).

What I'm talking about is a specific function called

rt5677_read_acpi_properties()

in the rt5677.c codec driver.

The question is do we have _real publicly available_ hardware with that
kind of properties?

Because now it's a mess (wrt to real DSDT attached to the thread).

The proposed change to fix this is like

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 6448b7a00203..28bde5f50ed9 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -5145,20 +5145,18 @@ static int rt5677_i2c_probe(struct i2c_client
*i2c)
 		match_id = of_match_device(rt5677_of_match, &i2c->dev);
 		if (match_id)
 			rt5677->type = (enum rt5677_type)match_id-
>data;
-
-		rt5677_read_device_properties(rt5677, &i2c->dev);
 	} else if (ACPI_HANDLE(&i2c->dev)) {
 		const struct acpi_device_id *acpi_id;
 
 		acpi_id = acpi_match_device(rt5677_acpi_match, &i2c-
>dev);
 		if (acpi_id)
 			rt5677->type = (enum rt5677_type)acpi_id-
>driver_data;
-
-		rt5677_read_acpi_properties(rt5677, &i2c->dev);
 	} else {
 		return -EINVAL;
 	}
 
+	rt5677_read_device_properties(rt5677, &i2c->dev);
+
 	/* pow-ldo2 and reset are optional. The codec pins may be
statically
 	 * connected on the board without gpios. If the gpio device
property
 	 * isn't specified, devm_gpiod_get_optional returns NULL.

+ removing rt5677_read_acpi_properties() completely.

Tom, if you can test it (basic test + might be quality of sound) on your
Chromebook, it would be nice!


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Andy Shevchenko]

On Fri, 2017-08-25 at 17:05 +0100, John Keeping wrote:
> On Fri, 25 Aug 2017 10:24:26 -0400, Tom Rini wrote:
> > On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:

> > > Apparently you are the one who tested the commit
> > > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > > year ago.  
> > 
> > Yes.
> > 
> > > The commit states that ACPI properties that are used in Chromebook
> > > Pixel
> > > 2015 is non-standard (not the same as for DT).
> > > 
> > > However, DSDT shows the opposite!  
> > 
> > Interesting.  I'm not an ACPI person, I just tested what John came
> > up
> > with.
> 
> And the patch adding this was the first (and still only) time I've
> really looked at ACPI, so it's quite possible that I misunderstood
> something at the time.

Maybe.

> From memory, I think the particular problem I was referring to in the
> commit message was that certain GPIOs were only defined by index and
> not
> by property name (specifically "plug-det-gpios", "mic-present-gpios"
> and
> "headphone-enable-gpios"), and having dumped DSDT just now I do not
> see
> those strings appearing anywhere.

Exactly, and this part of the patch I'm _not_ talking about (it's pretty
much good and working).

What I'm talking about is a specific function called

rt5677_read_acpi_properties()

in the rt5677.c codec driver.

The question is do we have _real publicly available_ hardware with that
kind of properties?

Because now it's a mess (wrt to real DSDT attached to the thread).

The proposed change to fix this is like

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 6448b7a00203..28bde5f50ed9 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -5145,20 +5145,18 @@ static int rt5677_i2c_probe(struct i2c_client
*i2c)
 		match_id = of_match_device(rt5677_of_match, &i2c->dev);
 		if (match_id)
 			rt5677->type = (enum rt5677_type)match_id-
>data;
-
-		rt5677_read_device_properties(rt5677, &i2c->dev);
 	} else if (ACPI_HANDLE(&i2c->dev)) {
 		const struct acpi_device_id *acpi_id;
 
 		acpi_id = acpi_match_device(rt5677_acpi_match, &i2c-
>dev);
 		if (acpi_id)
 			rt5677->type = (enum rt5677_type)acpi_id-
>driver_data;
-
-		rt5677_read_acpi_properties(rt5677, &i2c->dev);
 	} else {
 		return -EINVAL;
 	}
 
+	rt5677_read_device_properties(rt5677, &i2c->dev);
+
 	/* pow-ldo2 and reset are optional. The codec pins may be
statically
 	 * connected on the board without gpios. If the gpio device
property
 	 * isn't specified, devm_gpiod_get_optional returns NULL.

+ removing rt5677_read_acpi_properties() completely.

Tom, if you can test it (basic test + might be quality of sound) on your
Chromebook, it would be nice!


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[John Keeping]

On Fri, 25 Aug 2017 19:42:51 +0300, Andy Shevchenko wrote:

> On Fri, 2017-08-25 at 17:05 +0100, John Keeping wrote:
> > On Fri, 25 Aug 2017 10:24:26 -0400, Tom Rini wrote:  
> > > On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:  
> 
> > > > Apparently you are the one who tested the commit
> > > > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > > > year ago.    
> > > 
> > > Yes.
> > >   
> > > > The commit states that ACPI properties that are used in Chromebook
> > > > Pixel
> > > > 2015 is non-standard (not the same as for DT).
> > > > 
> > > > However, DSDT shows the opposite!    
> > > 
> > > Interesting.  I'm not an ACPI person, I just tested what John came
> > > up
> > > with.  
> > 
> > And the patch adding this was the first (and still only) time I've
> > really looked at ACPI, so it's quite possible that I misunderstood
> > something at the time.  
> 
> Maybe.
> 
> > From memory, I think the particular problem I was referring to in the
> > commit message was that certain GPIOs were only defined by index and
> > not
> > by property name (specifically "plug-det-gpios", "mic-present-gpios"
> > and
> > "headphone-enable-gpios"), and having dumped DSDT just now I do not
> > see
> > those strings appearing anywhere.  
> 
> Exactly, and this part of the patch I'm _not_ talking about (it's pretty
> much good and working).
> 
> What I'm talking about is a specific function called
> 
> rt5677_read_acpi_properties()
> 
> in the rt5677.c codec driver.

Right, I don't see any reason why it shouldn't be possible to replace
that with rt5677_read_device_properties() given the DSDT I have.

I expect that exists because I was using the chromeos-3.14 tree as a
reference (which was the supported kernel on this hardware at the time),
but it looks like the unified device property API was added in 3.18 so
of course was not used there, and I did not realise that the device
property versions could be used here.

I'll try to find time to test this change over the weekend, if Tom
doesn't beat me to it!

Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs

[Tom Rini]

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

On Fri, Aug 25, 2017 at 07:42:51PM +0300, Andy Shevchenko wrote:
> On Fri, 2017-08-25 at 17:05 +0100, John Keeping wrote:
> > On Fri, 25 Aug 2017 10:24:26 -0400, Tom Rini wrote:
> > > On Fri, Aug 25, 2017 at 04:56:47PM +0300, Andy Shevchenko wrote:
> 
> > > > Apparently you are the one who tested the commit
> > > > 	89128534f925 ("ASoC: rt5677: Add ACPI support")
> > > > year ago.  
> > > 
> > > Yes.
> > > 
> > > > The commit states that ACPI properties that are used in Chromebook
> > > > Pixel
> > > > 2015 is non-standard (not the same as for DT).
> > > > 
> > > > However, DSDT shows the opposite!  
> > > 
> > > Interesting.  I'm not an ACPI person, I just tested what John came
> > > up
> > > with.
> > 
> > And the patch adding this was the first (and still only) time I've
> > really looked at ACPI, so it's quite possible that I misunderstood
> > something at the time.
> 
> Maybe.
> 
> > From memory, I think the particular problem I was referring to in the
> > commit message was that certain GPIOs were only defined by index and
> > not
> > by property name (specifically "plug-det-gpios", "mic-present-gpios"
> > and
> > "headphone-enable-gpios"), and having dumped DSDT just now I do not
> > see
> > those strings appearing anywhere.
> 
> Exactly, and this part of the patch I'm _not_ talking about (it's pretty
> much good and working).
> 
> What I'm talking about is a specific function called
> 
> rt5677_read_acpi_properties()
> 
> in the rt5677.c codec driver.
> 
> The question is do we have _real publicly available_ hardware with that
> kind of properties?
> 
> Because now it's a mess (wrt to real DSDT attached to the thread).
> 
> The proposed change to fix this is like
> 
> diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
> index 6448b7a00203..28bde5f50ed9 100644
> --- a/sound/soc/codecs/rt5677.c
> +++ b/sound/soc/codecs/rt5677.c
> @@ -5145,20 +5145,18 @@ static int rt5677_i2c_probe(struct i2c_client
> *i2c)
>  		match_id = of_match_device(rt5677_of_match, &i2c->dev);
>  		if (match_id)
>  			rt5677->type = (enum rt5677_type)match_id-
> >data;
> -
> -		rt5677_read_device_properties(rt5677, &i2c->dev);
>  	} else if (ACPI_HANDLE(&i2c->dev)) {
>  		const struct acpi_device_id *acpi_id;
>  
>  		acpi_id = acpi_match_device(rt5677_acpi_match, &i2c-
> >dev);
>  		if (acpi_id)
>  			rt5677->type = (enum rt5677_type)acpi_id-
> >driver_data;
> -
> -		rt5677_read_acpi_properties(rt5677, &i2c->dev);
>  	} else {
>  		return -EINVAL;
>  	}
>  
> +	rt5677_read_device_properties(rt5677, &i2c->dev);
> +
>  	/* pow-ldo2 and reset are optional. The codec pins may be
> statically
>  	 * connected on the board without gpios. If the gpio device
> property
>  	 * isn't specified, devm_gpiod_get_optional returns NULL.
> 
> + removing rt5677_read_acpi_properties() completely.
> 
> Tom, if you can test it (basic test + might be quality of sound) on your
> Chromebook, it would be nice!

OK.  I did the above manually (on top of the correct fix for the problem
I originally reported from asoc-next), also removed
rt5677_read_acpi_properties and gave the various THX/Dolby sound tests a
spin and they sound good.

As an unrelated request for help, the headphone jack isn't automatically
detected and used, but I assume this is a user configuration issue.
This is unchanged with your patch :)

-- 
Tom

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