Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934636AbdHYQm4 (ORCPT ); Fri, 25 Aug 2017 12:42:56 -0400 Received: from mga11.intel.com ([192.55.52.93]:26366 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934474AbdHYQmz (ORCPT ); Fri, 25 Aug 2017 12:42:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,426,1498546800"; d="scan'208";a="1188159129" Message-ID: <1503679371.25945.110.camel@linux.intel.com> Subject: Re: [PATCH] ASoC: rt5677: Reintroduce I2C device IDs From: Andy Shevchenko To: John Keeping Cc: Tom Rini , linux-kernel@vger.kernel.org, Bard Liao , Oder Chiou , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , alsa-devel@alsa-project.org, Mark Brown , Linus Torvalds Date: Fri, 25 Aug 2017 19:42:51 +0300 In-Reply-To: <20170825170504.7d547aa4.john@metanate.com> References: <1503453106-5564-1-git-send-email-trini@konsulko.com> <1503669407.25945.102.camel@linux.intel.com> <20170825142426.GN2827@bill-the-cat> <20170825170504.7d547aa4.john@metanate.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2791 Lines: 94 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 Intel Finland Oy