[PATCH 0/2] ASoC: Intel: boards: use software node API

[PATCH 0/2] ASoC: Intel: boards: use software node API

[Pierre-Louis Bossart]

This is an update on an earlier contribution from Heikki Krogerus,
with mainly changes to avoid the use of devm_ routines for Baytrail
machine drivers.

Heikki Krogerus (2):
  ASoC: Intel: boards: use software node API in SoundWire machines
  ASoC: Intel: boards: use software node API in Atom boards

 sound/soc/intel/boards/bytcht_es8316.c      | 25 +++++++++--
 sound/soc/intel/boards/bytcr_rt5640.c       | 42 +++++++++++++++---
 sound/soc/intel/boards/bytcr_rt5651.c       | 47 +++++++++++++++++----
 sound/soc/intel/boards/sof_sdw_rt711.c      | 20 ++++++---
 sound/soc/intel/boards/sof_sdw_rt711_sdca.c | 20 ++++++---
 5 files changed, 127 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH 1/2] ASoC: Intel: boards: use software node API in SoundWire machines

[Pierre-Louis Bossart]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

The function device_add_properties() is going to be removed.
Replacing it with software node API equivalents.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/sof_sdw_rt711.c      | 20 +++++++++++++++-----
 sound/soc/intel/boards/sof_sdw_rt711_sdca.c | 20 +++++++++++++++-----
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw_rt711.c b/sound/soc/intel/boards/sof_sdw_rt711.c
index 04074c09dded..b7c635c0fadd 100644
--- a/sound/soc/intel/boards/sof_sdw_rt711.c
+++ b/sound/soc/intel/boards/sof_sdw_rt711.c
@@ -24,19 +24,29 @@
 static int rt711_add_codec_device_props(const char *sdw_dev_name)
 {
 	struct property_entry props[MAX_NO_PROPS] = {};
+	struct fwnode_handle *fwnode;
 	struct device *sdw_dev;
 	int ret;
 
+	if (!SOF_RT711_JDSRC(sof_sdw_quirk))
+		return 0;
+
 	sdw_dev = bus_find_device_by_name(&sdw_bus_type, NULL, sdw_dev_name);
 	if (!sdw_dev)
 		return -EPROBE_DEFER;
 
-	if (SOF_RT711_JDSRC(sof_sdw_quirk)) {
-		props[0] = PROPERTY_ENTRY_U32("realtek,jd-src",
-					      SOF_RT711_JDSRC(sof_sdw_quirk));
+	props[0] = PROPERTY_ENTRY_U32("realtek,jd-src",
+				      SOF_RT711_JDSRC(sof_sdw_quirk));
+
+	fwnode = fwnode_create_software_node(props, NULL);
+	if (IS_ERR(fwnode)) {
+		put_device(sdw_dev);
+		return PTR_ERR(fwnode);
 	}
 
-	ret = device_add_properties(sdw_dev, props);
+	ret = device_add_software_node(sdw_dev, to_software_node(fwnode));
+
+	fwnode_handle_put(fwnode);
 	put_device(sdw_dev);
 
 	return ret;
@@ -144,7 +154,7 @@ int sof_sdw_rt711_exit(struct device *dev, struct snd_soc_dai_link *dai_link)
 	if (!sdw_dev)
 		return -EINVAL;
 
-	device_remove_properties(sdw_dev);
+	device_remove_software_node(sdw_dev);
 	put_device(sdw_dev);
 
 	return 0;
diff --git a/sound/soc/intel/boards/sof_sdw_rt711_sdca.c b/sound/soc/intel/boards/sof_sdw_rt711_sdca.c
index 19496f0f9110..300a52d15506 100644
--- a/sound/soc/intel/boards/sof_sdw_rt711_sdca.c
+++ b/sound/soc/intel/boards/sof_sdw_rt711_sdca.c
@@ -24,19 +24,29 @@
 static int rt711_sdca_add_codec_device_props(const char *sdw_dev_name)
 {
 	struct property_entry props[MAX_NO_PROPS] = {};
+	struct fwnode_handle *fwnode;
 	struct device *sdw_dev;
 	int ret;
 
+	if (!SOF_RT711_JDSRC(sof_sdw_quirk))
+		return 0;
+
 	sdw_dev = bus_find_device_by_name(&sdw_bus_type, NULL, sdw_dev_name);
 	if (!sdw_dev)
 		return -EPROBE_DEFER;
 
-	if (SOF_RT711_JDSRC(sof_sdw_quirk)) {
-		props[0] = PROPERTY_ENTRY_U32("realtek,jd-src",
-					      SOF_RT711_JDSRC(sof_sdw_quirk));
+	props[0] = PROPERTY_ENTRY_U32("realtek,jd-src",
+				      SOF_RT711_JDSRC(sof_sdw_quirk));
+
+	fwnode = fwnode_create_software_node(props, NULL);
+	if (IS_ERR(fwnode)) {
+		put_device(sdw_dev);
+		return PTR_ERR(fwnode);
 	}
 
-	ret = device_add_properties(sdw_dev, props);
+	ret = device_add_software_node(sdw_dev, to_software_node(fwnode));
+
+	fwnode_handle_put(fwnode);
 	put_device(sdw_dev);
 
 	return ret;
@@ -144,7 +154,7 @@ int sof_sdw_rt711_sdca_exit(struct device *dev, struct snd_soc_dai_link *dai_lin
 	if (!sdw_dev)
 		return -EINVAL;
 
-	device_remove_properties(sdw_dev);
+	device_remove_software_node(sdw_dev);
 	put_device(sdw_dev);
 
 	return 0;
-- 
2.25.1

[PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Pierre-Louis Bossart]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

The function device_add_properties() is going to be removed.
Replacing it with software node API equivalents.

Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 sound/soc/intel/boards/bytcht_es8316.c | 25 ++++++++++++--
 sound/soc/intel/boards/bytcr_rt5640.c  | 42 +++++++++++++++++++----
 sound/soc/intel/boards/bytcr_rt5651.c  | 47 +++++++++++++++++++++-----
 3 files changed, 97 insertions(+), 17 deletions(-)

diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
index a0af91580184..ef8ed3ff53af 100644
--- a/sound/soc/intel/boards/bytcht_es8316.c
+++ b/sound/soc/intel/boards/bytcht_es8316.c
@@ -38,6 +38,7 @@ struct byt_cht_es8316_private {
 	struct snd_soc_jack jack;
 	struct gpio_desc *speaker_en_gpio;
 	bool speaker_en;
+	struct device *codec_dev;
 };
 
 enum {
@@ -461,6 +462,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 	const struct dmi_system_id *dmi_id;
 	struct device *dev = &pdev->dev;
 	struct snd_soc_acpi_mach *mach;
+	struct fwnode_handle *fwnode;
 	const char *platform_name;
 	struct acpi_device *adev;
 	struct device *codec_dev;
@@ -543,7 +545,16 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 		props[cnt++] = PROPERTY_ENTRY_BOOL("everest,jack-detect-inverted");
 
 	if (cnt) {
-		ret = device_add_properties(codec_dev, props);
+		fwnode = fwnode_create_software_node(props, NULL);
+		if (IS_ERR(fwnode)) {
+			put_device(codec_dev);
+			return PTR_ERR(fwnode);
+		}
+
+		ret = device_add_software_node(codec_dev, to_software_node(fwnode));
+
+		fwnode_handle_put(fwnode);
+
 		if (ret) {
 			put_device(codec_dev);
 			return ret;
@@ -556,6 +567,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 				/* see comment in byt_cht_es8316_resume */
 				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	put_device(codec_dev);
+	priv->codec_dev = codec_dev;
 
 	if (IS_ERR(priv->speaker_en_gpio)) {
 		ret = PTR_ERR(priv->speaker_en_gpio);
@@ -567,7 +579,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 			dev_err(dev, "get speaker GPIO failed: %d\n", ret);
 			fallthrough;
 		case -EPROBE_DEFER:
-			return ret;
+			goto err;
 		}
 	}
 
@@ -605,10 +617,15 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 	if (ret) {
 		gpiod_put(priv->speaker_en_gpio);
 		dev_err(dev, "snd_soc_register_card failed: %d\n", ret);
-		return ret;
+		goto err;
 	}
 	platform_set_drvdata(pdev, &byt_cht_es8316_card);
 	return 0;
+
+err:
+	device_remove_software_node(priv->codec_dev);
+
+	return ret;
 }
 
 static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
@@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
 	struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
 
 	gpiod_put(priv->speaker_en_gpio);
+	device_remove_software_node(priv->codec_dev);
+
 	return 0;
 }
 
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 91a6d712eb58..b3597b0f6836 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -87,6 +87,7 @@ enum {
 struct byt_rt5640_private {
 	struct snd_soc_jack jack;
 	struct clk *mclk;
+	struct device *codec_dev;
 };
 static bool is_bytcr;
 
@@ -912,9 +913,11 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = {
  * Note this MUST be called before snd_soc_register_card(), so that the props
  * are in place before the codec component driver's probe function parses them.
  */
-static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name)
+static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name,
+					     struct byt_rt5640_private *priv)
 {
 	struct property_entry props[MAX_NO_PROPS] = {};
+	struct fwnode_handle *fwnode;
 	struct device *i2c_dev;
 	int ret, cnt = 0;
 
@@ -960,7 +963,18 @@ static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name)
 	if (byt_rt5640_quirk & BYT_RT5640_JD_NOT_INV)
 		props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
 
-	ret = device_add_properties(i2c_dev, props);
+	fwnode = fwnode_create_software_node(props, NULL);
+	if (IS_ERR(fwnode)) {
+		/* put_device() is not handled in caller */
+		put_device(i2c_dev);
+		return PTR_ERR(fwnode);
+	}
+
+	ret = device_add_software_node(i2c_dev, to_software_node(fwnode));
+
+	fwnode_handle_put(fwnode);
+	priv->codec_dev = i2c_dev;
+
 	put_device(i2c_dev);
 
 	return ret;
@@ -1401,7 +1415,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	}
 
 	/* Must be called before register_card, also see declaration comment. */
-	ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name);
+	ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name, priv);
 	if (ret_val)
 		return ret_val;
 
@@ -1434,7 +1448,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 			 * for all other errors, including -EPROBE_DEFER
 			 */
 			if (ret_val != -ENOENT)
-				return ret_val;
+				goto err;
 			byt_rt5640_quirk &= ~BYT_RT5640_MCLK_EN;
 		}
 	}
@@ -1467,7 +1481,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5640_card,
 							platform_name);
 	if (ret_val)
-		return ret_val;
+		goto err;
 
 	sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
 
@@ -1489,10 +1503,25 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	if (ret_val) {
 		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
 			ret_val);
-		return ret_val;
+		goto err;
 	}
 	platform_set_drvdata(pdev, &byt_rt5640_card);
 	return ret_val;
+
+err:
+	device_remove_software_node(priv->codec_dev);
+
+	return ret_val;
+}
+
+static int snd_byt_rt5640_mc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
+
+	device_remove_software_node(priv->codec_dev);
+
+	return 0;
 }
 
 static struct platform_driver snd_byt_rt5640_mc_driver = {
@@ -1500,6 +1529,7 @@ static struct platform_driver snd_byt_rt5640_mc_driver = {
 		.name = "bytcr_rt5640",
 	},
 	.probe = snd_byt_rt5640_mc_probe,
+	.remove = snd_byt_rt5640_mc_remove
 };
 
 module_platform_driver(snd_byt_rt5640_mc_driver);
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index e13c0c63a949..3066d00f3466 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -85,6 +85,7 @@ struct byt_rt5651_private {
 	struct gpio_desc *ext_amp_gpio;
 	struct gpio_desc *hp_detect;
 	struct snd_soc_jack jack;
+	struct device *codec_dev;
 };
 
 static const struct acpi_gpio_mapping *byt_rt5651_gpios;
@@ -527,10 +528,13 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = {
  * Note this MUST be called before snd_soc_register_card(), so that the props
  * are in place before the codec component driver's probe function parses them.
  */
-static int byt_rt5651_add_codec_device_props(struct device *i2c_dev)
+static int byt_rt5651_add_codec_device_props(struct device *i2c_dev,
+					     struct byt_rt5651_private *priv)
 {
 	struct property_entry props[MAX_NO_PROPS] = {};
+	struct fwnode_handle *fwnode;
 	int cnt = 0;
+	int ret;
 
 	props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
 				BYT_RT5651_JDSRC(byt_rt5651_quirk));
@@ -547,7 +551,18 @@ static int byt_rt5651_add_codec_device_props(struct device *i2c_dev)
 	if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV)
 		props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
 
-	return device_add_properties(i2c_dev, props);
+	fwnode = fwnode_create_software_node(props, NULL);
+	if (IS_ERR(fwnode)) {
+		/* put_device(i2c_dev) is handled in caller */
+		return PTR_ERR(fwnode);
+	}
+
+	ret = device_add_software_node(i2c_dev, to_software_node(fwnode));
+
+	fwnode_handle_put(fwnode);
+	priv->codec_dev = i2c_dev;
+
+	return ret;
 }
 
 static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
@@ -994,7 +1009,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	}
 
 	/* Must be called before register_card, also see declaration comment. */
-	ret_val = byt_rt5651_add_codec_device_props(codec_dev);
+	ret_val = byt_rt5651_add_codec_device_props(codec_dev, priv);
 	if (ret_val) {
 		put_device(codec_dev);
 		return ret_val;
@@ -1023,7 +1038,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 				fallthrough;
 			case -EPROBE_DEFER:
 				put_device(codec_dev);
-				return ret_val;
+				goto err;
 			}
 		}
 		priv->hp_detect = devm_fwnode_gpiod_get(&pdev->dev,
@@ -1043,7 +1058,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 				fallthrough;
 			case -EPROBE_DEFER:
 				put_device(codec_dev);
-				return ret_val;
+				goto err;
 			}
 		}
 	}
@@ -1073,7 +1088,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 			 * for all other errors, including -EPROBE_DEFER
 			 */
 			if (ret_val != -ENOENT)
-				return ret_val;
+				goto err;
 			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
 		}
 	}
@@ -1102,7 +1117,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5651_card,
 							platform_name);
 	if (ret_val)
-		return ret_val;
+		goto err;
 
 	sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
 
@@ -1124,10 +1139,25 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	if (ret_val) {
 		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
 			ret_val);
-		return ret_val;
+		goto err;
 	}
 	platform_set_drvdata(pdev, &byt_rt5651_card);
 	return ret_val;
+
+err:
+	device_remove_software_node(priv->codec_dev);
+
+	return ret_val;
+}
+
+static int snd_byt_rt5651_mc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card);
+
+	device_remove_software_node(priv->codec_dev);
+
+	return 0;
 }
 
 static struct platform_driver snd_byt_rt5651_mc_driver = {
@@ -1135,6 +1165,7 @@ static struct platform_driver snd_byt_rt5651_mc_driver = {
 		.name = "bytcr_rt5651",
 	},
 	.probe = snd_byt_rt5651_mc_probe,
+	.remove = snd_byt_rt5651_mc_remove,
 };
 
 module_platform_driver(snd_byt_rt5651_mc_driver);
-- 
2.25.1

Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Hans de Goede]

Hi,

On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> The function device_add_properties() is going to be removed.
> Replacing it with software node API equivalents.
> 
> Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  sound/soc/intel/boards/bytcht_es8316.c | 25 ++++++++++++--
>  sound/soc/intel/boards/bytcr_rt5640.c  | 42 +++++++++++++++++++----
>  sound/soc/intel/boards/bytcr_rt5651.c  | 47 +++++++++++++++++++++-----
>  3 files changed, 97 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
> index a0af91580184..ef8ed3ff53af 100644
> --- a/sound/soc/intel/boards/bytcht_es8316.c
> +++ b/sound/soc/intel/boards/bytcht_es8316.c
> @@ -38,6 +38,7 @@ struct byt_cht_es8316_private {
>  	struct snd_soc_jack jack;
>  	struct gpio_desc *speaker_en_gpio;
>  	bool speaker_en;
> +	struct device *codec_dev;
>  };
>  
>  enum {
> @@ -461,6 +462,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  	const struct dmi_system_id *dmi_id;
>  	struct device *dev = &pdev->dev;
>  	struct snd_soc_acpi_mach *mach;
> +	struct fwnode_handle *fwnode;
>  	const char *platform_name;
>  	struct acpi_device *adev;
>  	struct device *codec_dev;
> @@ -543,7 +545,16 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  		props[cnt++] = PROPERTY_ENTRY_BOOL("everest,jack-detect-inverted");
>  
>  	if (cnt) {
> -		ret = device_add_properties(codec_dev, props);
> +		fwnode = fwnode_create_software_node(props, NULL);
> +		if (IS_ERR(fwnode)) {
> +			put_device(codec_dev);
> +			return PTR_ERR(fwnode);
> +		}
> +
> +		ret = device_add_software_node(codec_dev, to_software_node(fwnode));
> +
> +		fwnode_handle_put(fwnode);
> +
>  		if (ret) {
>  			put_device(codec_dev);
>  			return ret;
> @@ -556,6 +567,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  				/* see comment in byt_cht_es8316_resume */
>  				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
>  	put_device(codec_dev);
> +	priv->codec_dev = codec_dev;
>  
>  	if (IS_ERR(priv->speaker_en_gpio)) {
>  		ret = PTR_ERR(priv->speaker_en_gpio);
> @@ -567,7 +579,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  			dev_err(dev, "get speaker GPIO failed: %d\n", ret);
>  			fallthrough;
>  		case -EPROBE_DEFER:
> -			return ret;
> +			goto err;
>  		}
>  	}
>  
> @@ -605,10 +617,15 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  	if (ret) {
>  		gpiod_put(priv->speaker_en_gpio);
>  		dev_err(dev, "snd_soc_register_card failed: %d\n", ret);
> -		return ret;
> +		goto err;
>  	}
>  	platform_set_drvdata(pdev, &byt_cht_es8316_card);
>  	return 0;
> +
> +err:
> +	device_remove_software_node(priv->codec_dev);
> +
> +	return ret;
>  }
>  
>  static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
> @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
>  	struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
>  
>  	gpiod_put(priv->speaker_en_gpio);
> +	device_remove_software_node(priv->codec_dev);

This is a problem, nothing guarantees codec_dev not going away before
snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up
with where this happens is unbinding the i2c-controller driver I still would like
us to take this scenario into account.

I think it would be better to use device_create_managed_software_node() to tie
the lifetime of the swnode to the lifetime of the device, this also removes
the need for all the goto err cases (and introducing a remove function in
the bytcr_rt5640.c case).

This does mean that we could end up calling device_create_managed_software_node()
on the same device twice, when the machine driver gets unbound + rebound, but
that is an already existing issue with our current device_add_properties() usage.

We could fix this (in a separate commit since it is an already existing issue)
by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the
properties and checking for that being set with
device_property_read_bool(codec, "cht_es8316,swnode-created")

Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().

I've a slight preference for using device_create_managed_software_node() +
some mechanism to avoid a double adding of the properties, since I would like
to try and avoid the "goto err" additions.

Ideally device_create_managed_software_node() would detect the double-add
itself and it would return -EEXISTS. Heikki, would that be feasible ?

Regards,

Hans





> +
>  	return 0;
>  }
>  
> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
> index 91a6d712eb58..b3597b0f6836 100644
> --- a/sound/soc/intel/boards/bytcr_rt5640.c
> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> @@ -87,6 +87,7 @@ enum {
>  struct byt_rt5640_private {
>  	struct snd_soc_jack jack;
>  	struct clk *mclk;
> +	struct device *codec_dev;
>  };
>  static bool is_bytcr;
>  
> @@ -912,9 +913,11 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = {
>   * Note this MUST be called before snd_soc_register_card(), so that the props
>   * are in place before the codec component driver's probe function parses them.
>   */
> -static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name)
> +static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name,
> +					     struct byt_rt5640_private *priv)
>  {
>  	struct property_entry props[MAX_NO_PROPS] = {};
> +	struct fwnode_handle *fwnode;
>  	struct device *i2c_dev;
>  	int ret, cnt = 0;
>  
> @@ -960,7 +963,18 @@ static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name)
>  	if (byt_rt5640_quirk & BYT_RT5640_JD_NOT_INV)
>  		props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
>  
> -	ret = device_add_properties(i2c_dev, props);
> +	fwnode = fwnode_create_software_node(props, NULL);
> +	if (IS_ERR(fwnode)) {
> +		/* put_device() is not handled in caller */
> +		put_device(i2c_dev);
> +		return PTR_ERR(fwnode);
> +	}
> +
> +	ret = device_add_software_node(i2c_dev, to_software_node(fwnode));
> +
> +	fwnode_handle_put(fwnode);
> +	priv->codec_dev = i2c_dev;
> +
>  	put_device(i2c_dev);
>  
>  	return ret;
> @@ -1401,7 +1415,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Must be called before register_card, also see declaration comment. */
> -	ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name);
> +	ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name, priv);
>  	if (ret_val)
>  		return ret_val;
>  
> @@ -1434,7 +1448,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  			 * for all other errors, including -EPROBE_DEFER
>  			 */
>  			if (ret_val != -ENOENT)
> -				return ret_val;
> +				goto err;
>  			byt_rt5640_quirk &= ~BYT_RT5640_MCLK_EN;
>  		}
>  	}
> @@ -1467,7 +1481,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  	ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5640_card,
>  							platform_name);
>  	if (ret_val)
> -		return ret_val;
> +		goto err;
>  
>  	sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
>  
> @@ -1489,10 +1503,25 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  	if (ret_val) {
>  		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
>  			ret_val);
> -		return ret_val;
> +		goto err;
>  	}
>  	platform_set_drvdata(pdev, &byt_rt5640_card);
>  	return ret_val;
> +
> +err:
> +	device_remove_software_node(priv->codec_dev);
> +
> +	return ret_val;
> +}
> +
> +static int snd_byt_rt5640_mc_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
> +
> +	device_remove_software_node(priv->codec_dev);
> +
> +	return 0;
>  }
>  
>  static struct platform_driver snd_byt_rt5640_mc_driver = {
> @@ -1500,6 +1529,7 @@ static struct platform_driver snd_byt_rt5640_mc_driver = {
>  		.name = "bytcr_rt5640",
>  	},
>  	.probe = snd_byt_rt5640_mc_probe,
> +	.remove = snd_byt_rt5640_mc_remove
>  };
>  
>  module_platform_driver(snd_byt_rt5640_mc_driver);
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index e13c0c63a949..3066d00f3466 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -85,6 +85,7 @@ struct byt_rt5651_private {
>  	struct gpio_desc *ext_amp_gpio;
>  	struct gpio_desc *hp_detect;
>  	struct snd_soc_jack jack;
> +	struct device *codec_dev;
>  };
>  
>  static const struct acpi_gpio_mapping *byt_rt5651_gpios;
> @@ -527,10 +528,13 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = {
>   * Note this MUST be called before snd_soc_register_card(), so that the props
>   * are in place before the codec component driver's probe function parses them.
>   */
> -static int byt_rt5651_add_codec_device_props(struct device *i2c_dev)
> +static int byt_rt5651_add_codec_device_props(struct device *i2c_dev,
> +					     struct byt_rt5651_private *priv)
>  {
>  	struct property_entry props[MAX_NO_PROPS] = {};
> +	struct fwnode_handle *fwnode;
>  	int cnt = 0;
> +	int ret;
>  
>  	props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
>  				BYT_RT5651_JDSRC(byt_rt5651_quirk));
> @@ -547,7 +551,18 @@ static int byt_rt5651_add_codec_device_props(struct device *i2c_dev)
>  	if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV)
>  		props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
>  
> -	return device_add_properties(i2c_dev, props);
> +	fwnode = fwnode_create_software_node(props, NULL);
> +	if (IS_ERR(fwnode)) {
> +		/* put_device(i2c_dev) is handled in caller */
> +		return PTR_ERR(fwnode);
> +	}
> +
> +	ret = device_add_software_node(i2c_dev, to_software_node(fwnode));
> +
> +	fwnode_handle_put(fwnode);
> +	priv->codec_dev = i2c_dev;
> +
> +	return ret;
>  }
>  
>  static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
> @@ -994,7 +1009,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Must be called before register_card, also see declaration comment. */
> -	ret_val = byt_rt5651_add_codec_device_props(codec_dev);
> +	ret_val = byt_rt5651_add_codec_device_props(codec_dev, priv);
>  	if (ret_val) {
>  		put_device(codec_dev);
>  		return ret_val;
> @@ -1023,7 +1038,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  				fallthrough;
>  			case -EPROBE_DEFER:
>  				put_device(codec_dev);
> -				return ret_val;
> +				goto err;
>  			}
>  		}
>  		priv->hp_detect = devm_fwnode_gpiod_get(&pdev->dev,
> @@ -1043,7 +1058,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  				fallthrough;
>  			case -EPROBE_DEFER:
>  				put_device(codec_dev);
> -				return ret_val;
> +				goto err;
>  			}
>  		}
>  	}
> @@ -1073,7 +1088,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  			 * for all other errors, including -EPROBE_DEFER
>  			 */
>  			if (ret_val != -ENOENT)
> -				return ret_val;
> +				goto err;
>  			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
>  		}
>  	}
> @@ -1102,7 +1117,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  	ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5651_card,
>  							platform_name);
>  	if (ret_val)
> -		return ret_val;
> +		goto err;
>  
>  	sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
>  
> @@ -1124,10 +1139,25 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  	if (ret_val) {
>  		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
>  			ret_val);
> -		return ret_val;
> +		goto err;
>  	}
>  	platform_set_drvdata(pdev, &byt_rt5651_card);
>  	return ret_val;
> +
> +err:
> +	device_remove_software_node(priv->codec_dev);
> +
> +	return ret_val;
> +}
> +
> +static int snd_byt_rt5651_mc_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card);
> +
> +	device_remove_software_node(priv->codec_dev);
> +
> +	return 0;
>  }
>  
>  static struct platform_driver snd_byt_rt5651_mc_driver = {
> @@ -1135,6 +1165,7 @@ static struct platform_driver snd_byt_rt5651_mc_driver = {
>  		.name = "bytcr_rt5651",
>  	},
>  	.probe = snd_byt_rt5651_mc_probe,
> +	.remove = snd_byt_rt5651_mc_remove,
>  };
>  
>  module_platform_driver(snd_byt_rt5651_mc_driver);
> 

Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Andy Shevchenko]

On Tue, Jun 08, 2021 at 10:18:08AM +0200, Hans de Goede wrote:
> On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > The function device_add_properties() is going to be removed.
> > Replacing it with software node API equivalents.

...

> > +	device_remove_software_node(priv->codec_dev);
> 
> This is a problem, nothing guarantees codec_dev not going away before
> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up
> with where this happens is unbinding the i2c-controller driver I still would like
> us to take this scenario into account.
> 
> I think it would be better to use device_create_managed_software_node() to tie
> the lifetime of the swnode to the lifetime of the device, this also removes
> the need for all the goto err cases (and introducing a remove function in
> the bytcr_rt5640.c case).

Which device? If you are telling about codec, the unload-load of the machine
driver won't be successful or will produce a leak / warning / etc.

If you are telling about machine related device, it simply doesn't belong to it.

Am I got all this right?

> This does mean that we could end up calling device_create_managed_software_node()
> on the same device twice, when the machine driver gets unbound + rebound, but
> that is an already existing issue with our current device_add_properties() usage.

Yep.

> We could fix this (in a separate commit since it is an already existing issue)
> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the
> properties and checking for that being set with
> device_property_read_bool(codec, "cht_es8316,swnode-created")

Not sure it's a good idea, this sounds like a hack.

> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().

This sounds better.

> I've a slight preference for using device_create_managed_software_node() +
> some mechanism to avoid a double adding of the properties, since I would like
> to try and avoid the "goto err" additions.
> 
> Ideally device_create_managed_software_node() would detect the double-add
> itself and it would return -EEXISTS. Heikki, would that be feasible ?

If I got the big picture correct, the SOF needs to switch to use fwnode graphs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Hans de Goede]

Hi,

On 6/8/21 11:02 AM, Andy Shevchenko wrote:
> On Tue, Jun 08, 2021 at 10:18:08AM +0200, Hans de Goede wrote:
>> On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote:
>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>
>>> The function device_add_properties() is going to be removed.
>>> Replacing it with software node API equivalents.
> 
> ...
> 
>>> +	device_remove_software_node(priv->codec_dev);
>>
>> This is a problem, nothing guarantees codec_dev not going away before
>> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up
>> with where this happens is unbinding the i2c-controller driver I still would like
>> us to take this scenario into account.
>>
>> I think it would be better to use device_create_managed_software_node() to tie
>> the lifetime of the swnode to the lifetime of the device, this also removes
>> the need for all the goto err cases (and introducing a remove function in
>> the bytcr_rt5640.c case).
> 
> Which device? If you are telling about codec, the unload-load of the machine
> driver won't be successful or will produce a leak / warning / etc.

Yes I'm talking about the codec, and yes if the codec device goes away before
the machine-driver is unbound things will likely already break. But the
machine driver does not hold any explicit reference on the codec-device,
so this might happen (I guess there might be a reference somewhere inside the
ASoC code once devm_snd_soc_register_card() has returned successfully).

> If you are telling about machine related device, it simply doesn't belong to it.
> 
> Am I got all this right?
> 
>> This does mean that we could end up calling device_create_managed_software_node()
>> on the same device twice, when the machine driver gets unbound + rebound, but
>> that is an already existing issue with our current device_add_properties() usage.
> 
> Yep.
> 
>> We could fix this (in a separate commit since it is an already existing issue)
>> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the
>> properties and checking for that being set with
>> device_property_read_bool(codec, "cht_es8316,swnode-created")
> 
> Not sure it's a good idea, this sounds like a hack.

Right, which is why I also suggested that device_create_managed_software_node()
could be modified to fail when called a second time on the same device, this
is a check which probably would be good to add regardless. More specifically
I guess that set_secondary_fwnode() could be made to return an error when replacing
an existing secondary fwnode with a non NULL value, rather then just replacing it.

>> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().
> 
> This sounds better.

As I already mentioned I'm not a fan of all the goto err-s these patches
introduce, this won't fix that.

Regards,

Hans

Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Pierre-Louis Bossart]

>>   static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
>> @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
>>   	struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
>>   
>>   	gpiod_put(priv->speaker_en_gpio);
>> +	device_remove_software_node(priv->codec_dev);
> 
> This is a problem, nothing guarantees codec_dev not going away before
> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up
> with where this happens is unbinding the i2c-controller driver I still would like
> us to take this scenario into account.

Is this possible really? the codec driver will register a component 
that's used by the ASoC card, I was assuming that there was some sort of 
reference count already to prevent this unbinding from happening.

> I think it would be better to use device_create_managed_software_node() to tie
> the lifetime of the swnode to the lifetime of the device, this also removes
> the need for all the goto err cases (and introducing a remove function in
> the bytcr_rt5640.c case).
> 
> This does mean that we could end up calling device_create_managed_software_node()
> on the same device twice, when the machine driver gets unbound + rebound, but
> that is an already existing issue with our current device_add_properties() usage.
> 
> We could fix this (in a separate commit since it is an already existing issue)
> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the
> properties and checking for that being set with
> device_property_read_bool(codec, "cht_es8316,swnode-created")
> 
> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().

that sounds like a better plan if you want to manage explicit 
dependencies - regardless of how which API is used to manage properties.

Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Hans de Goede]

Hi,

On 6/8/21 1:22 PM, Pierre-Louis Bossart wrote:
> 
>>>   static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
>>> @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev)
>>>       struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
>>>         gpiod_put(priv->speaker_en_gpio);
>>> +    device_remove_software_node(priv->codec_dev);
>>
>> This is a problem, nothing guarantees codec_dev not going away before
>> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up
>> with where this happens is unbinding the i2c-controller driver I still would like
>> us to take this scenario into account.
> 
> Is this possible really? the codec driver will register a component that's used by the ASoC card, I was assuming that there was some sort of reference count already to prevent this unbinding from happening.

There might very well be some reference count elsewhere, but IMHO if the
machine-driver is going to keep a pointer to the device around it should
keep its own reference.

>> I think it would be better to use device_create_managed_software_node() to tie
>> the lifetime of the swnode to the lifetime of the device, this also removes
>> the need for all the goto err cases (and introducing a remove function in
>> the bytcr_rt5640.c case).
>>
>> This does mean that we could end up calling device_create_managed_software_node()
>> on the same device twice, when the machine driver gets unbound + rebound, but
>> that is an already existing issue with our current device_add_properties() usage.
>>
>> We could fix this (in a separate commit since it is an already existing issue)
>> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the
>> properties and checking for that being set with
>> device_property_read_bool(codec, "cht_es8316,swnode-created")
>>
>> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().
> 
> that sounds like a better plan if you want to manage explicit dependencies - regardless of how which API is used to manage properties.

Ok, so lets go with that then.

Regards,

Hans