Skip to contentSkip to conversation

ASoC: da7219: Add ACPI initialisation support to driver

PATCH In review Adam Thomson 3 participants 14 messages May 5, 2016 – Jun 10, 2016

Subsystems (3)
Sound ASoC Dialog semiconductor drivers
lore
Share & tools
lore

Adam Thomson

author
#0 lore raw

cover ASoC: da7219: Add ACPI initialisation support to driver

This patch set updates the driver to use generic device property & fwnode related functions to read in either DT and ACPI data for driver initialisation.

Changes are based on v4.6-rc6 Linux Kernel.

Adam Thomson (3):
ASoC: da7219: Convert driver to use generic device/fwnode functions
ASoC: da7219: Add ACPI parsing support
ASoC: da7219: Add initial ACPI id for device

 sound/soc/codecs/da7219-aad.c | 96 +++++++++++++++++++++++++++++++------------
 sound/soc/codecs/da7219.c     | 35 ++++++++++------
 2 files changed, 91 insertions(+), 40 deletions(-)

--
1.9.3

author
#1 lore raw

PATCH 1/3 ASoC: da7219: Convert driver to use generic device/fwnode functions

This change converts the driver from using the of_* functions to using the device_* and fwnode_* functions for accssing DT related data. This is in preparation for updates to support ACPI based initialisation.

Signed-off-by Adam Thomson <[email protected]>
Tested-by Sathyanarayana Nujella <[email protected]>
---
 sound/soc/codecs/da7219-aad.c | 64 +++++++++++++++++++++++++++----------------
 sound/soc/codecs/da7219.c     | 21 +++++++-------
 2 files changed, 52 insertions(+), 33 deletions(-)
sound/soc/codecs/da7219-aad.c +41 -23
index 9459593..c4853a8 100644
@@ -13,8 +13,9 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/i2c.h>
 #include <linux/of_device.h>
-#include <linux/of_irq.h>
+#include <linux/property.h>
 #include <linux/pm_wakeirq.h>
 #include <linux/slab.h>
 #include <linux/delay.h>

@@ -538,10 +539,30 @@ static enum da7219_aad_adc_1bit_rpt
 	}
 }
 
+static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
+							  const char *name)
+{
+	struct fwnode_handle *child;
+	struct device_node *of_node;
+
+	/* Find first matching child node */
+	device_for_each_child_node(dev, child) {
+		if (is_of_node(child)) {
+			of_node = to_of_node(child);
+			if (of_node_cmp(of_node->name, name) == 0)
+				return child;
+		}
+	}
+
+	return NULL;
+}
+
 static struct da7219_aad_pdata *da7219_aad_of_to_pdata(struct snd_soc_codec *codec)
 {
-	struct device_node *np = codec->dev->of_node;
-	struct device_node *aad_np = of_find_node_by_name(np, "da7219_aad");
+	struct device *dev = codec->dev;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct fwnode_handle *aad_np =
+		da7219_aad_of_named_fwhandle(dev, "da7219_aad");
 	struct da7219_aad_pdata *aad_pdata;
 	const char *of_str;
 	u32 of_val32;

@@ -551,84 +572,81 @@ static struct da7219_aad_pdata *da7219_aad_of_to_pdata(struct snd_soc_codec *cod
 
 	aad_pdata = devm_kzalloc(codec->dev, sizeof(*aad_pdata), GFP_KERNEL);
 	if (!aad_pdata)
-		goto out;
+		return NULL;
 
-	aad_pdata->irq = irq_of_parse_and_map(np, 0);
+	aad_pdata->irq = i2c->irq;
 
-	if (of_property_read_u32(aad_np, "dlg,micbias-pulse-lvl",
-				 &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,micbias-pulse-lvl",
+				     &of_val32) >= 0)
 		aad_pdata->micbias_pulse_lvl =
 			da7219_aad_of_micbias_pulse_lvl(codec, of_val32);
 	else
 		aad_pdata->micbias_pulse_lvl = DA7219_AAD_MICBIAS_PULSE_LVL_OFF;
 
-	if (of_property_read_u32(aad_np, "dlg,micbias-pulse-time",
-				 &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,micbias-pulse-time",
+				     &of_val32) >= 0)
 		aad_pdata->micbias_pulse_time = of_val32;
 
-	if (of_property_read_u32(aad_np, "dlg,btn-cfg", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,btn-cfg", &of_val32) >= 0)
 		aad_pdata->btn_cfg = da7219_aad_of_btn_cfg(codec, of_val32);
 	else
 		aad_pdata->btn_cfg = DA7219_AAD_BTN_CFG_10MS;
 
-	if (of_property_read_u32(aad_np, "dlg,mic-det-thr", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,mic-det-thr", &of_val32) >= 0)
 		aad_pdata->mic_det_thr =
 			da7219_aad_of_mic_det_thr(codec, of_val32);
 	else
 		aad_pdata->mic_det_thr = DA7219_AAD_MIC_DET_THR_500_OHMS;
 
-	if (of_property_read_u32(aad_np, "dlg,jack-ins-deb", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,jack-ins-deb", &of_val32) >= 0)
 		aad_pdata->jack_ins_deb =
 			da7219_aad_of_jack_ins_deb(codec, of_val32);
 	else
 		aad_pdata->jack_ins_deb = DA7219_AAD_JACK_INS_DEB_20MS;
 
-	if (!of_property_read_string(aad_np, "dlg,jack-det-rate", &of_str))
+	if (!fwnode_property_read_string(aad_np, "dlg,jack-det-rate", &of_str))
 		aad_pdata->jack_det_rate =
 			da7219_aad_of_jack_det_rate(codec, of_str);
 	else
 		aad_pdata->jack_det_rate = DA7219_AAD_JACK_DET_RATE_256_512MS;
 
-	if (of_property_read_u32(aad_np, "dlg,jack-rem-deb", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,jack-rem-deb", &of_val32) >= 0)
 		aad_pdata->jack_rem_deb =
 			da7219_aad_of_jack_rem_deb(codec, of_val32);
 	else
 		aad_pdata->jack_rem_deb = DA7219_AAD_JACK_REM_DEB_1MS;
 
-	if (of_property_read_u32(aad_np, "dlg,a-d-btn-thr", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,a-d-btn-thr", &of_val32) >= 0)
 		aad_pdata->a_d_btn_thr = (u8) of_val32;
 	else
 		aad_pdata->a_d_btn_thr = 0xA;
 
-	if (of_property_read_u32(aad_np, "dlg,d-b-btn-thr", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,d-b-btn-thr", &of_val32) >= 0)
 		aad_pdata->d_b_btn_thr = (u8) of_val32;
 	else
 		aad_pdata->d_b_btn_thr = 0x16;
 
-	if (of_property_read_u32(aad_np, "dlg,b-c-btn-thr", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,b-c-btn-thr", &of_val32) >= 0)
 		aad_pdata->b_c_btn_thr = (u8) of_val32;
 	else
 		aad_pdata->b_c_btn_thr = 0x21;
 
-	if (of_property_read_u32(aad_np, "dlg,c-mic-btn-thr", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,c-mic-btn-thr", &of_val32) >= 0)
 		aad_pdata->c_mic_btn_thr = (u8) of_val32;
 	else
 		aad_pdata->c_mic_btn_thr = 0x3E;
 
-	if (of_property_read_u32(aad_np, "dlg,btn-avg", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,btn-avg", &of_val32) >= 0)
 		aad_pdata->btn_avg = da7219_aad_of_btn_avg(codec, of_val32);
 	else
 		aad_pdata->btn_avg = DA7219_AAD_BTN_AVG_2;
 
-	if (of_property_read_u32(aad_np, "dlg,adc-1bit-rpt", &of_val32) >= 0)
+	if (fwnode_property_read_u32(aad_np, "dlg,adc-1bit-rpt", &of_val32) >= 0)
 		aad_pdata->adc_1bit_rpt =
 			da7219_aad_of_adc_1bit_rpt(codec, of_val32);
 	else
 		aad_pdata->adc_1bit_rpt = DA7219_AAD_ADC_1BIT_RPT_1;
 
-out:
-	of_node_put(aad_np);
-
 	return aad_pdata;
 }
sound/soc/codecs/da7219.c +11 -10
index 7c7824c..6c6c8db 100644
@@ -14,6 +14,7 @@
 #include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/of_device.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/pm.h>

@@ -1427,7 +1428,7 @@ static const struct of_device_id da7219_of_match[] = {
 MODULE_DEVICE_TABLE(of, da7219_of_match);
 
 static enum da7219_micbias_voltage
-	da7219_of_micbias_lvl(struct snd_soc_codec *codec, u32 val)
+	da7219_of_micbias_lvl(struct device *dev, u32 val)
 {
 	switch (val) {
 	case 1600:

@@ -1443,13 +1444,13 @@ static enum da7219_micbias_voltage
 	case 2600:
 		return DA7219_MICBIAS_2_6V;
 	default:
-		dev_warn(codec->dev, "Invalid micbias level");
+		dev_warn(dev, "Invalid micbias level");
 		return DA7219_MICBIAS_2_2V;
 	}
 }
 
 static enum da7219_mic_amp_in_sel
-	da7219_of_mic_amp_in_sel(struct snd_soc_codec *codec, const char *str)
+	da7219_of_mic_amp_in_sel(struct device *dev, const char *str)
 {
 	if (!strcmp(str, "diff")) {
 		return DA7219_MIC_AMP_IN_SEL_DIFF;

@@ -1458,29 +1459,29 @@ static enum da7219_mic_amp_in_sel
 	} else if (!strcmp(str, "se_n")) {
 		return DA7219_MIC_AMP_IN_SEL_SE_N;
 	} else {
-		dev_warn(codec->dev, "Invalid mic input type selection");
+		dev_warn(dev, "Invalid mic input type selection");
 		return DA7219_MIC_AMP_IN_SEL_DIFF;
 	}
 }
 
 static struct da7219_pdata *da7219_of_to_pdata(struct snd_soc_codec *codec)
 {
-	struct device_node *np = codec->dev->of_node;
+	struct device *dev = codec->dev;
 	struct da7219_pdata *pdata;
 	const char *of_str;
 	u32 of_val32;
 
-	pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL);
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return NULL;
 
-	if (of_property_read_u32(np, "dlg,micbias-lvl", &of_val32) >= 0)
-		pdata->micbias_lvl = da7219_of_micbias_lvl(codec, of_val32);
+	if (device_property_read_u32(dev, "dlg,micbias-lvl", &of_val32) >= 0)
+		pdata->micbias_lvl = da7219_of_micbias_lvl(dev, of_val32);
 	else
 		pdata->micbias_lvl = DA7219_MICBIAS_2_2V;
 
-	if (!of_property_read_string(np, "dlg,mic-amp-in-sel", &of_str))
-		pdata->mic_amp_in_sel = da7219_of_mic_amp_in_sel(codec, of_str);
+	if (!device_property_read_string(dev, "dlg,mic-amp-in-sel", &of_str))
+		pdata->mic_amp_in_sel = da7219_of_mic_amp_in_sel(dev, of_str);
 	else
 		pdata->mic_amp_in_sel = DA7219_MIC_AMP_IN_SEL_DIFF;

--
1.9.3

maintainer
#2 lore raw

Re: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions

Quoted 3 lines · Adam Thomson

This change converts the driver from using the of_* functions to using the device_* and fwnode_* functions for accssing DT related data. This is in preparation for updates to support ACPI based initialisation.

Is this *really* sensible? DT idioms don't always match up with ACPI idioms well and this isn't a trivial DT binding.

Quoted 7 lines
sound/soc/codecs/da7219-aad.c +7 -0
+static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
+							  const char *name)
+{
+	struct fwnode_handle *child;
+	struct device_node *of_node;
+
+	/* Find first matching child node */

sound/soc/codecs/da7219-aad.c static enum da7219_aad_adc_1bit_rpt · line 558

Quoted 10 lines
sound/soc/codecs/da7219-aad.c +10 -0
+	device_for_each_child_node(dev, child) {
+		if (is_of_node(child)) {
+			of_node = to_of_node(child);
+			if (of_node_cmp(of_node->name, name) == 0)
+				return child;
+		}
+	}
+
+	return NULL;
+}

There's nothing device specific about this, it should go in generic code.

author
#3 lore raw

RE: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions

Quoted 6 lines · Mark Brown
Quoted 3 lines

This change converts the driver from using the of_* functions to using the device_* and fwnode_* functions for accssing DT related data. This is in preparation for updates to support ACPI based initialisation.

Is this *really* sensible? DT idioms don't always match up with ACPI idioms well and this isn't a trivial DT binding.

For what we're doing here, both DT and ACPI match up well, and so what I've implemented works on both sides. There are other examples of this already in the Linux kernel, so I don't think it's anything particularly new.

Quoted 14 lines
Quoted 2 lines

+static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device

*dev,

Quoted 11 lines
sound/soc/codecs/da7219-aad.c +11 -0
+							  const char *name)
+{
+	struct fwnode_handle *child;
+	struct device_node *of_node;
+
+	/* Find first matching child node */
+	device_for_each_child_node(dev, child) {
+		if (is_of_node(child)) {
+			of_node = to_of_node(child);
+			if (of_node_cmp(of_node->name, name) == 0)
+				return child;
Quoted 8 lines

sound/soc/codecs/da7219-aad.c static enum da7219_aad_adc_1bit_rpt · line 558

Quoted 5 lines
sound/soc/codecs/da7219-aad.c +5 -0
+		}
+	}
+
+	return NULL;
+}

There's nothing device specific about this, it should go in generic code.

The intention was to just match against DT or ACPI and nothing else, so that didn't feel generic enough to be pushed into the fwnode framework. However I will take another look.

maintainer
#4 lore raw

Re: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions

Quoted 8 lines · Opensource [Adam Thomson]
Quoted 2 lines · Mark Brown

Is this *really* sensible? DT idioms don't always match up with ACPI idioms well and this isn't a trivial DT binding.

For what we're doing here, both DT and ACPI match up well, and so what I've implemented works on both sides. There are other examples of this already in the Linux kernel, so I don't think it's anything particularly new.

No, not really - your DT is fairly unusual in how it's done and the lack of ACPI helpers is not a good sign on that side. The _DSD things are really only supposed to work for simple properties on devices.

Quoted 6 lines
Quoted 2 lines

There's nothing device specific about this, it should go in generic code.

The intention was to just match against DT or ACPI and nothing else, so that didn't feel generic enough to be pushed into the fwnode framework. However I will take another look.

That's currently the entire set of things that fwnode supports so...

author
#5 lore raw

RE: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions

Quoted 3 lines · Mark Brown

No, not really - your DT is fairly unusual in how it's done and the lack of ACPI helpers is not a good sign on that side. The _DSD things are really only supposed to work for simple properties on devices.

It's unusual in that there's a child node ("da7219_aad") of the device node, to encapsulate AAD specific information. The properties inside though are data only and simple. Actually, as I read it this is exactly the kind of thing that the 'Hierarchical Data Extension UUID For _DSD' is for.

Quoted 5 lines
Quoted 3 lines

The intention was to just match against DT or ACPI and nothing else, so that didn't feel generic enough to be pushed into the fwnode framework. However I will take another look.

That's currently the entire set of things that fwnode supports so...

I believe there's the FWNODE_PDATA type as well so 3 things, although I assume that this is to be used longer term instead of the old fashioned platform data mechanism, for built-in properties. Right now though I don't see much actual usage of this.

maintainer
#6 lore raw

Re: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions

Quoted 10 lines · Opensource [Adam Thomson]
Quoted 3 lines · Mark Brown

No, not really - your DT is fairly unusual in how it's done and the lack of ACPI helpers is not a good sign on that side. The _DSD things are really only supposed to work for simple properties on devices.

It's unusual in that there's a child node ("da7219_aad") of the device node, to encapsulate AAD specific information. The properties inside though are data only and simple. Actually, as I read it this is exactly the kind of thing that the 'Hierarchical Data Extension UUID For _DSD' is for.

If it's something that's supposed to be done I'd expect the code to be cleaner and less driver specific.

Quoted 10 lines
Quoted 5 lines
Quoted 3 lines

The intention was to just match against DT or ACPI and nothing else, so that didn't feel generic enough to be pushed into the fwnode framework. However I will take another look.

That's currently the entire set of things that fwnode supports so...

I believe there's the FWNODE_PDATA type as well so 3 things, although I assume that this is to be used longer term instead of the old fashioned platform data mechanism, for built-in properties. Right now though I don't see much actual usage of this.

Well, if you do that you don't need to check if data has been provided at all.

author
#7 lore raw

PATCH 2/3 ASoC: da7219: Add ACPI parsing support

In reply to Adam Thomson

This update allows for parsing of ACPI, in addition to existing DT support.

Signed-off-by Adam Thomson <[email protected]>
Tested-by Sathyanarayana Nujella <[email protected]>
---
 sound/soc/codecs/da7219-aad.c | 32 ++++++++++++++++++++++++++++----
 sound/soc/codecs/da7219.c     |  7 ++++---
 2 files changed, 32 insertions(+), 7 deletions(-)
sound/soc/codecs/da7219-aad.c +28 -4
index c4853a8..e04ee4f 100644
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
 #include <linux/of_device.h>
+#include <linux/acpi.h>
 #include <linux/property.h>
 #include <linux/pm_wakeirq.h>
 #include <linux/slab.h>

@@ -27,7 +28,6 @@
 #include "da7219.h"
 #include "da7219-aad.h"
 
-
 /*
  * Detection control
  */

@@ -383,7 +383,7 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
 }
 
 /*
- * DT to pdata conversion
+ * DT/ACPI to pdata conversion
  */
 
 static enum da7219_aad_micbias_pulse_lvl

@@ -539,6 +539,26 @@ static enum da7219_aad_adc_1bit_rpt
 	}
 }
 
+#ifdef CONFIG_ACPI
+static inline bool da7219_aad_of_acpi_node_matched(struct fwnode_handle *child,
+						   const char *name)
+{
+	struct acpi_data_node *acpi_node = to_acpi_data_node(child);
+
+	if (strcmp(acpi_node->name, name) == 0)
+		return true;
+	else
+		return false;
+}
+#else
+static inline bool da7219_aad_of_acpi_node_matched(struct fwnode_handle *child,
+						   const char *name)
+{
+	return false;
+}
+
+#endif /* CONFIG_ACPI */
+
 static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
 							  const char *name)
 {

@@ -551,6 +571,9 @@ static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
 			of_node = to_of_node(child);
 			if (of_node_cmp(of_node->name, name) == 0)
 				return child;
+		} else if (is_acpi_data_node(child)) {
+			if (da7219_aad_of_acpi_node_matched(child, name))
+				return child;
 		}
 	}
 

@@ -787,8 +810,9 @@ int da7219_aad_init(struct snd_soc_codec *codec)
 	da7219->aad = da7219_aad;
 	da7219_aad->codec = codec;
 
-	/* Handle any DT/platform data */
-	if ((codec->dev->of_node) && (da7219->pdata))
+	/* Handle any DT/ACPI/platform data */
+	if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) &&
+	    (da7219->pdata))
 		da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec);
 
 	da7219_aad_handle_pdata(codec);
sound/soc/codecs/da7219.c +4 -3
index 6c6c8db..bc32322 100644
@@ -14,6 +14,7 @@
 #include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/of_device.h>
+#include <linux/acpi.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>

@@ -1418,7 +1419,7 @@ static struct snd_soc_dai_driver da7219_dai = {
 
 
 /*
- * DT
+ * DT/ACPI
  */
 
 static const struct of_device_id da7219_of_match[] = {

@@ -1656,8 +1657,8 @@ static int da7219_probe(struct snd_soc_codec *codec)
 		break;
 	}
 
-	/* Handle DT/Platform data */
-	if (codec->dev->of_node)
+	/* Handle DT/ACPI/Platform data */
+	if (codec->dev->of_node || is_acpi_node(codec->dev->fwnode))
 		da7219->pdata = da7219_of_to_pdata(codec);
 	else
 		da7219->pdata = dev_get_platdata(codec->dev);
maintainer
#8 lore raw

Re: [PATCH 2/3] ASoC: da7219: Add ACPI parsing support

sound/soc/codecs/da7219-aad.c line 33

Quoted 8 lines · Adam Thomson
sound/soc/codecs/da7219-aad.c +0 -1
@@ -27,7 +28,6 @@
 #include "da7219.h"
 #include "da7219-aad.h"
 
-
 /*
  * Detection control
  */

Random whitespace change.

sound/soc/codecs/da7219-aad.c static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev, · line 579

Quoted 13 lines

static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,

sound/soc/codecs/da7219-aad.c +3 -0
 							  const char *name)
 {

@@ -551,6 +571,9 @@ static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
 			of_node = to_of_node(child);
 			if (of_node_cmp(of_node->name, name) == 0)
 				return child;
+		} else if (is_acpi_data_node(child)) {
+			if (da7219_aad_of_acpi_node_matched(child, name))
+				return child;
 		}
 	}

This seems messy. It is a function with a DT specific name that's matching ACPI stuff and the fwnode API isn't hiding anything for us which suggests this isn't something that's expected to work transparently. At least the naming needs to be corrected, and if this *is* supposed to be something we do in ACPI I'd expect the handling to be pushed into the fwnode API rather than open coded in a driver - at the minute I'm unsure if this is messy because it's a bad idea to do this at all or if it's just the naming and so on.

sound/soc/codecs/da7219-aad.c int da7219_aad_init(struct snd_soc_codec *codec) · line 818

Quoted 8 lines
sound/soc/codecs/da7219-aad.c +3 -2
-	/* Handle any DT/platform data */
-	if ((codec->dev->of_node) && (da7219->pdata))
+	/* Handle any DT/ACPI/platform data */
+	if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) &&
+	    (da7219->pdata))
 		da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec);
 
 	da7219_aad_handle_pdata(codec);

Surely we should be able to check if there's firmware data without enumerating every possible firmware type?

author
#9 lore raw

RE: [PATCH 2/3] ASoC: da7219: Add ACPI parsing support

Quoted 10 lines · Mark Brown

sound/soc/codecs/da7219-aad.c line 33

Quoted 8 lines
sound/soc/codecs/da7219-aad.c +0 -1
@@ -27,7 +28,6 @@
 #include "da7219.h"
 #include "da7219-aad.h"
 
-
 /*
  * Detection control
  */

Random whitespace change.

Fair point. Will sort it.

Quoted 17 lines
Quoted 2 lines

static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device

*dev,

sound/soc/codecs/da7219-aad.c static struct fwnode_handle

Quoted 3 lines
sound/soc/codecs/da7219-aad.c +0 -0
 							  const char *name)
 {

@@ -551,6 +571,9 @@ static struct fwnode_handle

*da7219_aad_of_named_fwhandle(struct device *dev,

sound/soc/codecs/da7219-aad.c static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev, · line 579

Quoted 9 lines
sound/soc/codecs/da7219-aad.c +3 -0
 			of_node = to_of_node(child);
 			if (of_node_cmp(of_node->name, name) == 0)
 				return child;
+		} else if (is_acpi_data_node(child)) {
+			if (da7219_aad_of_acpi_node_matched(child, name))
+				return child;
 		}
 	}
Quoted 8 lines

This seems messy. It is a function with a DT specific name that's matching ACPI stuff and the fwnode API isn't hiding anything for us which suggests this isn't something that's expected to work transparently. At least the naming needs to be corrected, and if this *is* supposed to be something we do in ACPI I'd expect the handling to be pushed into the fwnode API rather than open coded in a driver - at the minute I'm unsure if this is messy because it's a bad idea to do this at all or if it's just the naming and so on.

ACPI allows for data only child nodes, like DT, so I do believe this is a valid case. Also, this kind of functionality is already used in a similar-ish manner in the leds-gpio code. I agree that maybe the name should be changed though as it's misleading. Will also look at the fwnode API and see if it makes sense to move this there.

Quoted 8 lines
Quoted 8 lines
sound/soc/codecs/da7219-aad.c +3 -2
-	/* Handle any DT/platform data */
-	if ((codec->dev->of_node) && (da7219->pdata))
+	/* Handle any DT/ACPI/platform data */
+	if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) &&
+	    (da7219->pdata))
 		da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec);
Quoted 4 lines
Quoted 1 line

da7219_aad_handle_pdata(codec);

Surely we should be able to check if there's firmware data without enumerating every possible firmware type?

There doesn't seem to be a unified check for this. Also, Given these are the only two types the driver expects and supports right now, I don't see a problem being explicit here in the checking.

maintainer
#10 lore raw

Re: [PATCH 2/3] ASoC: da7219: Add ACPI parsing support

Quoted 9 lines · Opensource [Adam Thomson]
Quoted 6 lines · Mark Brown
Quoted 6 lines
sound/soc/codecs/da7219-aad.c +3 -2
-	/* Handle any DT/platform data */
-	if ((codec->dev->of_node) && (da7219->pdata))
+	/* Handle any DT/ACPI/platform data */
+	if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) &&
+	    (da7219->pdata))
 		da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec);
Quoted 8 lines
Quoted 4 lines
Quoted 1 line

da7219_aad_handle_pdata(codec);

Surely we should be able to check if there's firmware data without enumerating every possible firmware type?

There doesn't seem to be a unified check for this. Also, Given these are the only two types the driver expects and supports right now, I don't see a problem being explicit here in the checking.

Again it's pointing out something that looks like it's missing from the fwnode API - if people are supposed to be able to write firmware neutral drivers they should be able to do everything they need at the fwnode level.

author
#11 lore raw

PATCH 3/3 ASoC: da7219: Add initial ACPI id for device

In reply to Adam Thomson

This adds "DLGS7219" ACPI id for the codec.

Signed-off-by Adam Thomson <[email protected]>
Tested-by Sathyanarayana Nujella <[email protected]>
---
 sound/soc/codecs/da7219.c | 7 +++++++
 1 file changed, 7 insertions(+)
sound/soc/codecs/da7219.c +7 -0
index bc32322..da268b8 100644
@@ -1428,6 +1428,12 @@ static const struct of_device_id da7219_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, da7219_of_match);
 
+static const struct acpi_device_id da7219_acpi_match[] = {
+	{ .id = "DLGS7219", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, da7219_acpi_match);
+
 static enum da7219_micbias_voltage
 	da7219_of_micbias_lvl(struct device *dev, u32 val)
 {

@@ -1957,6 +1963,7 @@ static struct i2c_driver da7219_i2c_driver = {
 	.driver = {
 		.name = "da7219",
 		.of_match_table = of_match_ptr(da7219_of_match),
+		.acpi_match_table = ACPI_PTR(da7219_acpi_match),
 	},
 	.probe		= da7219_i2c_probe,
 	.remove		= da7219_i2c_remove,
#12 lore raw

Re: [PATCH 3/3] ASoC: da7219: Add initial ACPI id for device

Quoted 1 line · Adam Thomson

This adds "DLGS7219" ACPI id for the codec.

A valid ACPI ID, the first in a long time on this mailing list :-)

Quoted 40 lines
Signed-off-by Adam Thomson <[email protected]>
Tested-by Sathyanarayana Nujella <[email protected]>
---
 sound/soc/codecs/da7219.c | 7 +++++++
 1 file changed, 7 insertions(+)
sound/soc/codecs/da7219.c +7 -1
index bc32322..da268b8 100644
@@ -1428,6 +1428,12 @@ static const struct of_device_id da7219_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, da7219_of_match);
 
+static const struct acpi_device_id da7219_acpi_match[] = {
+	{ .id = "DLGS7219", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, da7219_acpi_match);
+
 static enum da7219_micbias_voltage
 	da7219_of_micbias_lvl(struct device *dev, u32 val)
 {

@@ -1957,6 +1963,7 @@ static struct i2c_driver da7219_i2c_driver = {
 	.driver = {
 		.name = "da7219",
 		.of_match_table = of_match_ptr(da7219_of_match),
+		.acpi_match_table = ACPI_PTR(da7219_acpi_match),
 	},
 	.probe		= da7219_i2c_probe,
 	.remove		= da7219_i2c_remove,
--

1.9.3

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

author
#13 lore raw
In reply to Adam Thomson
Quoted 10 lines · Adam Thomson

This patch set updates the driver to use generic device property & fwnode related functions to read in either DT and ACPI data for driver initialisation.

Changes are based on v4.6-rc6 Linux Kernel.

Adam Thomson (3):
ASoC: da7219: Convert driver to use generic device/fwnode functions
ASoC: da7219: Add ACPI parsing support
ASoC: da7219: Add initial ACPI id for device

Quoted 7 lines
 sound/soc/codecs/da7219-aad.c | 96 +++++++++++++++++++++++++++++++---------
---
 sound/soc/codecs/da7219.c     | 35 ++++++++++------
 2 files changed, 91 insertions(+), 40 deletions(-)

--
1.9.3

PLEASE IGNORE THIS PATCH SET MAIL. Apologies, finger trouble from my end :(