[PATCH 0/3] ASoC: da7219: Add ACPI initialisation support to driver

[PATCH 0/3] ASoC: da7219: Add ACPI initialisation support to driver

[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

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

--
1.9.3

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

[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.

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Tested-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com>
---
 sound/soc/codecs/da7219-aad.c | 64 +++++++++++++++++++++++++++----------------
 sound/soc/codecs/da7219.c     | 21 +++++++-------
 2 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c
index 9459593..c4853a8 100644
--- a/sound/soc/codecs/da7219-aad.c
+++ b/sound/soc/codecs/da7219-aad.c
@@ -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;
 }

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 7c7824c..6c6c8db 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -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

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

[Adam Thomson]

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

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Tested-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com>
---
 sound/soc/codecs/da7219-aad.c | 32 ++++++++++++++++++++++++++++----
 sound/soc/codecs/da7219.c     |  7 ++++---
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c
index c4853a8..e04ee4f 100644
--- a/sound/soc/codecs/da7219-aad.c
+++ b/sound/soc/codecs/da7219-aad.c
@@ -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);
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 6c6c8db..bc32322 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -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);
--
1.9.3

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

[Adam Thomson]

This adds "DLGS7219" ACPI id for the codec.

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Tested-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com>
---
 sound/soc/codecs/da7219.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index bc32322..da268b8 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Thu, May 05, 2016 at 11:53:04AM +0100, Adam Thomson wrote:

> 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.

> +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;
> +}

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Thu, May 05, 2016 at 11:53:05AM +0100, Adam Thomson wrote:

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

Random whitespace change.

>  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;
>  		}
>  	}
> 

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.

> -	/* 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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Opensource [Adam Thomson]]

On May 06, 2016, 13:27, Mark Brown wrote:

> > 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.

> 
> > +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;
> > +}
> 
> 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.

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

[Opensource [Adam Thomson]]

On May 06, 2016, 13:39, Mark Brown wrote:

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

Fair point. Will sort it.

> 
> >  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;
> >  		}
> >  	}
> >
> 
> 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.

> 
> > -	/* 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?

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Fri, May 06, 2016 at 02:33:04PM +0000, Opensource [Adam Thomson] wrote:
> On May 06, 2016, 13:27, Mark Brown wrote:

> > 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.

> > 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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Fri, May 06, 2016 at 02:45:00PM +0000, Opensource [Adam Thomson] wrote:
> On May 06, 2016, 13:39, Mark Brown wrote:

> > > -	/* 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?

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Opensource [Adam Thomson]]

On May 06, 2016, 17:06, Mark Brown wrote:

> 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.

> > 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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On Mon, May 09, 2016 at 12:05:56PM +0000, Opensource [Adam Thomson] wrote:
> On May 06, 2016, 17:06, Mark Brown wrote:

> > 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.

> > > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH 0/3] ASoC: da7219: Add ACPI initialisation support to driver

[Opensource [Adam Thomson]]

On 10 June 2016 11:16, Adam Thomson wrote:

> 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

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