Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1717969 > unrolled thread

[PATCH] ASoC: rt5677: Reintroduce I2C device IDs

Back to article view | Back to linux.kernel

Contents

  [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-23 04:00 +0200
    Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-23 11:30 +0200
      Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 00:40 +0200
        Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-24 00:50 +0200
          Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 01:00 +0200
            Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-24 01:20 +0200
          Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-24 01:10 +0200
    Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-23 16:40 +0200
      Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-23 19:40 +0200
        Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 02:10 +0200
          Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-24 09:50 +0200
            Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 13:20 +0200
              Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-24 14:40 +0200
                Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 15:50 +0200
                Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-24 15:50 +0200
          Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-24 16:30 +0200
            Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-24 16:40 +0200
              Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 16:50 +0200
                Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-24 17:50 +0200
                  Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-24 18:00 +0200
                    Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 18:00 +0200
                      Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-24 18:10 +0200
                        Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-24 18:10 +0200
                        Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-24 19:20 +0200
                          Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-24 19:50 +0200
                            Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-25 15:50 +0200
                        Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Mark Brown <broonie@kernel.org> - 2017-08-25 15:20 +0200
                          Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-25 15:20 +0200
                    Re: [alsa-devel] [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Takashi Iwai <tiwai@suse.de> - 2017-08-24 18:00 +0200
    Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-25 16:00 +0200
      Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-25 16:30 +0200
        Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-25 16:50 +0200
        Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs John Keeping <john@metanate.com> - 2017-08-25 18:40 +0200
          Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-25 18:50 +0200
            Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs John Keeping <john@metanate.com> - 2017-08-25 19:10 +0200
            Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs Tom Rini <trini@konsulko.com> - 2017-08-25 21:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1720084

+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

[toc] | [prev] | [next] | [standalone]

#1720113

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1720135

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

[toc] | [prev] | [next] | [standalone]

#1720262

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.

[toc] | [prev] | [next] | [standalone]

#1720269

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

[toc] | [prev] | [next] | [standalone]

#1720285

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!

[toc] | [prev] | [next] | [standalone]

#1720378

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web