| Message ID | 1503679371.25945.110.camel@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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!
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 :)
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);