diff mbox

[1/2] ASoC: rt5677: Add ACPI support

Message ID 20160814111823.1782-2-john@metanate.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Keeping Aug. 14, 2016, 11:18 a.m. UTC 
The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, so
add an ACPI match table and support for reading properties from ACPI.

Signed-off-by: John Keeping <john@metanate.com>
---
 sound/soc/codecs/rt5677.c | 109 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 88 insertions(+), 21 deletions(-)

Comments

Mark Brown Aug. 16, 2016, 5:20 p.m. UTC | #1 
On Sun, Aug 14, 2016 at 12:18:22PM +0100, John Keeping wrote:

> The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, so
> add an ACPI match table and support for reading properties from ACPI.

This would be a lot easier to review with a concrete description of what
"support for reading properties from ACPI" means and probably also split
out a bit so that different things were being added separately.

> +/* GPIO indexes defined by ACPI */
> +enum {
> +	RT5677_GPIO_PLUG_DET,
> +	RT5677_GPIO_MIC_PRESENT_L,
> +	RT5677_GPIO_HOTWORD_DET_L,
> +	RT5677_GPIO_DSP_INT,
> +	RT5677_GPIO_HP_AMP_SHDN_L,
> +};

If these are an ABI you should explicitly assign the values so that they
can't get remapped by future edits.  If they're not an ABI I don't
understand the comment.

> +	if (ACPI_HANDLE(dev)) {
> +		u32 val;
> +
> +		if (!device_property_read_u32(dev, "DCLK", &val))
> +			rt5677->pdata.dmic2_clk_pin = val;
> +
> +		rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1");
> +		rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2");

What happens if someone makes a machine which uses the DT<->ACPI
mappings (especially given that this is currently undocumented)?  That
would not work which defeats the whole purpose of using the device
property APIs.  Shouldn't we be accepting either property?
John Keeping Aug. 17, 2016, 10:05 a.m. UTC | #2 
On Tue, 16 Aug 2016 18:20:06 +0100, Mark Brown wrote:

> On Sun, Aug 14, 2016 at 12:18:22PM +0100, John Keeping wrote:
> 
> > The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, so
> > add an ACPI match table and support for reading properties from ACPI.  
> 
> This would be a lot easier to review with a concrete description of what
> "support for reading properties from ACPI" means and probably also split
> out a bit so that different things were being added separately.

OK, I'll have a think about this, although I'm not sure how much it can
be split up.  We can add the ACPI device property names in a separate
patch and maybe add the match table as a final step, but the GPIO
mapping and probe changes depend on each other.

> > +/* GPIO indexes defined by ACPI */
> > +enum {
> > +	RT5677_GPIO_PLUG_DET,
> > +	RT5677_GPIO_MIC_PRESENT_L,
> > +	RT5677_GPIO_HOTWORD_DET_L,
> > +	RT5677_GPIO_DSP_INT,
> > +	RT5677_GPIO_HP_AMP_SHDN_L,
> > +};  
> 
> If these are an ABI you should explicitly assign the values so that they
> can't get remapped by future edits.  If they're not an ABI I don't
> understand the comment.

Yes, these are ABI.  I'll add explicit values.

> > +	if (ACPI_HANDLE(dev)) {
> > +		u32 val;
> > +
> > +		if (!device_property_read_u32(dev, "DCLK", &val))
> > +			rt5677->pdata.dmic2_clk_pin = val;
> > +
> > +		rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1");
> > +		rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2");  
> 
> What happens if someone makes a machine which uses the DT<->ACPI
> mappings (especially given that this is currently undocumented)?  That
> would not work which defeats the whole purpose of using the device
> property APIs.  Shouldn't we be accepting either property?

I'm not sure we want to accept undocumented properties in the DT case.
I went looking for other drivers that have different property names for
ACPI and DT and the only example I could find is:

    drivers/net/ethernet/amd/xgbe/xgbe-main.c

In that case there are separate functions for parsing DT and ACPI
properties.  Having separate functions would have made this patch a lot
clearer, so if we keep the separation between DT and ACPI I'll make that
change for the next round.

The ideal might be to have something similar to the ACPI GPIO mapping
support so that the ACPI device property code could handle the different
names, but that feels like overkill if there are only two drivers that
use different property names in DT and ACPI.
diff mbox

Patch

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index da9483c1c6fb..72c4e15a0cc8 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -9,6 +9,7 @@ 
  * published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/fs.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -40,6 +41,15 @@ 
 
 #define RT5677_PR_BASE (RT5677_PR_RANGE_BASE + (0 * RT5677_PR_SPACING))
 
+/* GPIO indexes defined by ACPI */
+enum {
+	RT5677_GPIO_PLUG_DET,
+	RT5677_GPIO_MIC_PRESENT_L,
+	RT5677_GPIO_HOTWORD_DET_L,
+	RT5677_GPIO_DSP_INT,
+	RT5677_GPIO_HP_AMP_SHDN_L,
+};
+
 static const struct regmap_range_cfg rt5677_ranges[] = {
 	{
 		.name = "PR",
@@ -5022,29 +5032,57 @@  static const struct i2c_device_id rt5677_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id);
 
+static const struct acpi_gpio_params plug_det_gpio = { RT5677_GPIO_PLUG_DET, 0, false };
+static const struct acpi_gpio_params mic_present_gpio = { RT5677_GPIO_MIC_PRESENT_L, 0, false };
+static const struct acpi_gpio_params headphone_enable_gpio = { RT5677_GPIO_HP_AMP_SHDN_L, 0, false };
+
+static const struct acpi_gpio_mapping bdw_rt5677_gpios[] = {
+	{ "plug-det-gpios", &plug_det_gpio, 1 },
+	{ "mic-present-gpios", &mic_present_gpio, 1 },
+	{ "headphone-enable-gpios", &headphone_enable_gpio, 1 },
+	{ NULL },
+};
+
 static void rt5677_read_device_properties(struct rt5677_priv *rt5677,
 		struct device *dev)
 {
-	rt5677->pdata.in1_diff = device_property_read_bool(dev,
-			"realtek,in1-differential");
-	rt5677->pdata.in2_diff = device_property_read_bool(dev,
-			"realtek,in2-differential");
-	rt5677->pdata.lout1_diff = device_property_read_bool(dev,
-			"realtek,lout1-differential");
-	rt5677->pdata.lout2_diff = device_property_read_bool(dev,
-			"realtek,lout2-differential");
-	rt5677->pdata.lout3_diff = device_property_read_bool(dev,
-			"realtek,lout3-differential");
-
-	device_property_read_u8_array(dev, "realtek,gpio-config",
-			rt5677->pdata.gpio_config, RT5677_GPIO_NUM);
-
-	device_property_read_u32(dev, "realtek,jd1-gpio",
-			&rt5677->pdata.jd1_gpio);
-	device_property_read_u32(dev, "realtek,jd2-gpio",
-			&rt5677->pdata.jd2_gpio);
-	device_property_read_u32(dev, "realtek,jd3-gpio",
-			&rt5677->pdata.jd3_gpio);
+	if (ACPI_HANDLE(dev)) {
+		u32 val;
+
+		if (!device_property_read_u32(dev, "DCLK", &val))
+			rt5677->pdata.dmic2_clk_pin = val;
+
+		rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1");
+		rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2");
+		rt5677->pdata.lout1_diff = device_property_read_bool(dev, "OUT1");
+		rt5677->pdata.lout2_diff = device_property_read_bool(dev, "OUT2");
+		rt5677->pdata.lout3_diff = device_property_read_bool(dev, "OUT3");
+
+		device_property_read_u32(dev, "JD1", &rt5677->pdata.jd1_gpio);
+		device_property_read_u32(dev, "JD2", &rt5677->pdata.jd2_gpio);
+		device_property_read_u32(dev, "JD3", &rt5677->pdata.jd3_gpio);
+	} else {
+		rt5677->pdata.in1_diff = device_property_read_bool(dev,
+				"realtek,in1-differential");
+		rt5677->pdata.in2_diff = device_property_read_bool(dev,
+				"realtek,in2-differential");
+		rt5677->pdata.lout1_diff = device_property_read_bool(dev,
+				"realtek,lout1-differential");
+		rt5677->pdata.lout2_diff = device_property_read_bool(dev,
+				"realtek,lout2-differential");
+		rt5677->pdata.lout3_diff = device_property_read_bool(dev,
+				"realtek,lout3-differential");
+
+		device_property_read_u8_array(dev, "realtek,gpio-config",
+				rt5677->pdata.gpio_config, RT5677_GPIO_NUM);
+
+		device_property_read_u32(dev, "realtek,jd1-gpio",
+				&rt5677->pdata.jd1_gpio);
+		device_property_read_u32(dev, "realtek,jd2-gpio",
+				&rt5677->pdata.jd2_gpio);
+		device_property_read_u32(dev, "realtek,jd3-gpio",
+				&rt5677->pdata.jd3_gpio);
+	}
 }
 
 static struct regmap_irq rt5677_irqs[] = {
@@ -5123,7 +5161,27 @@  static int rt5677_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, rt5677);
 
-	rt5677->type = id->driver_data;
+	if (ACPI_HANDLE(&i2c->dev)) {
+		const struct acpi_device_id *acpi_id;
+
+		acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table,
+					    &i2c->dev);
+		if (!acpi_id) {
+			dev_err(&i2c->dev, "No driver data\n");
+			return -EINVAL;
+		}
+		rt5677->type = acpi_id->driver_data;
+
+		ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(&i2c->dev),
+						bdw_rt5677_gpios);
+		if (ret) {
+			dev_err(&i2c->dev, "Failed to add driver gpios\n");
+			return ret;
+		}
+
+	} else if (id) {
+		rt5677->type = id->driver_data;
+	}
 
 	if (pdata)
 		rt5677->pdata = *pdata;
@@ -5238,9 +5296,18 @@  static int rt5677_i2c_remove(struct i2c_client *i2c)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rt5677_acpi_id[] = {
+	{ "RT5677CE", RT5677 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, rt5677_acpi_id);
+#endif
+
 static struct i2c_driver rt5677_i2c_driver = {
 	.driver = {
 		.name = "rt5677",
+		.acpi_match_table = ACPI_PTR(rt5677_acpi_id),
 	},
 	.probe = rt5677_i2c_probe,
 	.remove   = rt5677_i2c_remove,