[PATCH 0/2] ASoC: fsl: audmix: fix two issues

[PATCH 0/2] ASoC: fsl: audmix: fix two issues

[Viorel Suman]

The latest audmix patch-set (v5) had the "model" attribute
removed as requested by Nicolin Chen, but looks like (v4)
version of DAI driver reached "for-next" branch - fix this.
Asside of this fix object reference leaks in machine probe reported
by Julia Lawall.

Viorel Suman (2):
  ASoC: fsl_audmix: remove "model" attribute
  ASoC: imx-audmix: fix object reference leaks in probe

 sound/soc/fsl/fsl_audmix.c | 61 ++++++++++++++++++++++++----------------------
 sound/soc/fsl/imx-audmix.c | 31 +++++++++--------------
 2 files changed, 43 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH 1/2] ASoC: fsl_audmix: remove "model" attribute

[Viorel Suman]

Use "of_device_id.data" to specify the machine driver,
instead of "model" DTS attribute.

Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
---
 sound/soc/fsl/fsl_audmix.c | 61 ++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
index dabde03..2d10d8b 100644
--- a/sound/soc/fsl/fsl_audmix.c
+++ b/sound/soc/fsl/fsl_audmix.c
@@ -445,61 +445,70 @@ static const struct regmap_config fsl_audmix_regmap_config = {
 	.cache_type = REGCACHE_FLAT,
 };
 
+static const struct of_device_id fsl_audmix_ids[] = {
+	{
+		.compatible = "fsl,imx8qm-audmix",
+		.data = "imx-audmix",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, fsl_audmix_ids);
+
 static int fsl_audmix_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct fsl_audmix *priv;
 	struct resource *res;
+	const char *mdrv;
+	const struct of_device_id *of_id;
 	void __iomem *regs;
 	int ret;
-	const char *sprop;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	of_id = of_match_device(fsl_audmix_ids, dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+
+	mdrv = of_id->data;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	/* Get the addresses */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	regs = devm_ioremap_resource(&pdev->dev, res);
+	regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	priv->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "ipg", regs,
+	priv->regmap = devm_regmap_init_mmio_clk(dev, "ipg", regs,
 						 &fsl_audmix_regmap_config);
 	if (IS_ERR(priv->regmap)) {
-		dev_err(&pdev->dev, "failed to init regmap\n");
+		dev_err(dev, "failed to init regmap\n");
 		return PTR_ERR(priv->regmap);
 	}
 
-	priv->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+	priv->ipg_clk = devm_clk_get(dev, "ipg");
 	if (IS_ERR(priv->ipg_clk)) {
-		dev_err(&pdev->dev, "failed to get ipg clock\n");
+		dev_err(dev, "failed to get ipg clock\n");
 		return PTR_ERR(priv->ipg_clk);
 	}
 
 	platform_set_drvdata(pdev, priv);
-	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(dev);
 
-	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_audmix_component,
+	ret = devm_snd_soc_register_component(dev, &fsl_audmix_component,
 					      fsl_audmix_dai,
 					      ARRAY_SIZE(fsl_audmix_dai));
 	if (ret) {
-		dev_err(&pdev->dev, "failed to register ASoC DAI\n");
+		dev_err(dev, "failed to register ASoC DAI\n");
 		return ret;
 	}
 
-	sprop = of_get_property(pdev->dev.of_node, "model", NULL);
-	if (sprop) {
-		priv->pdev = platform_device_register_data(&pdev->dev, sprop, 0,
-							   NULL, 0);
-		if (IS_ERR(priv->pdev)) {
-			ret = PTR_ERR(priv->pdev);
-			dev_err(&pdev->dev,
-				"failed to register platform %s: %d\n", sprop,
-				 ret);
-		}
-	} else {
-		dev_err(&pdev->dev, "[model] attribute missing.\n");
-		ret = -EINVAL;
+	priv->pdev = platform_device_register_data(dev, mdrv, 0, NULL, 0);
+	if (IS_ERR(priv->pdev)) {
+		ret = PTR_ERR(priv->pdev);
+		dev_err(dev, "failed to register platform %s: %d\n",
+			mdrv, ret);
 	}
 
 	return ret;
@@ -553,12 +562,6 @@ static const struct dev_pm_ops fsl_audmix_pm = {
 				pm_runtime_force_resume)
 };
 
-static const struct of_device_id fsl_audmix_ids[] = {
-	{ .compatible = "fsl,imx8qm-audmix", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, fsl_audmix_ids);
-
 static struct platform_driver fsl_audmix_driver = {
 	.probe = fsl_audmix_probe,
 	.remove = fsl_audmix_remove,
-- 
2.7.4

Re: [PATCH 1/2] ASoC: fsl_audmix: remove "model" attribute

[Daniel Baluta]

Hi Viorel,

Few comments inline.

On Tue, Apr 9, 2019 at 11:36 AM Viorel Suman <viorel.suman@nxp.com> wrote:
>
> Use "of_device_id.data" to specify the machine driver,
> instead of "model" DTS attribute.

<snip>

>  static int fsl_audmix_probe(struct platform_device *pdev)
>  {
> +       struct device *dev = &pdev->dev;

You might want to remove this change from the patch because it touches
a lot of lines
and it makes the review harder.

I don't see any reason to have it at all.

thanks,
Daniel.

[PATCH 2/2] ASoC: imx-audmix: fix object reference leaks in probe

[Viorel Suman]

Release the reference to the underlying device taken
by of_find_device_by_node() call.

Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
---
 sound/soc/fsl/imx-audmix.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/sound/soc/fsl/imx-audmix.c b/sound/soc/fsl/imx-audmix.c
index 7983bd3..7c24095 100644
--- a/sound/soc/fsl/imx-audmix.c
+++ b/sound/soc/fsl/imx-audmix.c
@@ -20,10 +20,7 @@
 #include "fsl_audmix.h"
 
 struct imx_audmix {
-	struct platform_device *pdev;
 	struct snd_soc_card card;
-	struct platform_device *audmix_pdev;
-	struct platform_device *out_pdev;
 	struct clk *cpu_mclk;
 	int num_dai;
 	struct snd_soc_dai_link *dai;
@@ -144,7 +141,7 @@ static struct snd_soc_ops imx_audmix_be_ops = {
 static int imx_audmix_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *audmix_np = NULL, *out_cpu_np = NULL;
+	struct device_node *audmix_np = NULL;
 	struct platform_device *audmix_pdev = NULL;
 	struct platform_device *cpu_pdev;
 	struct of_phandle_args args;
@@ -171,6 +168,7 @@ static int imx_audmix_probe(struct platform_device *pdev)
 			np->full_name);
 		return -EINVAL;
 	}
+	put_device(&audmix_pdev->dev);
 
 	num_dai = of_count_phandle_with_args(audmix_np, "dais", NULL);
 	if (num_dai != FSL_AUDMIX_MAX_DAIS) {
@@ -216,6 +214,7 @@ static int imx_audmix_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "failed to find SAI platform device\n");
 			return -EINVAL;
 		}
+		put_device(&cpu_pdev->dev);
 
 		dai_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s%s",
 					  fe_name_pref, args.np->full_name + 1);
@@ -223,7 +222,14 @@ static int imx_audmix_probe(struct platform_device *pdev)
 		dev_info(pdev->dev.parent, "DAI FE name:%s\n", dai_name);
 
 		if (i == 0) {
-			out_cpu_np = args.np;
+			priv->cpu_mclk = devm_clk_get(&cpu_pdev->dev, "mclk1");
+			if (IS_ERR(priv->cpu_mclk)) {
+				ret = PTR_ERR(priv->cpu_mclk);
+				dev_err(&cpu_pdev->dev,
+					"failed to get DAI mclk1: %d\n", ret);
+				return -EINVAL;
+			}
+
 			capture_dai_name =
 				devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s %s",
 					       dai_name, "CPU-Capture");
@@ -275,21 +281,6 @@ static int imx_audmix_probe(struct platform_device *pdev)
 		priv->dapm_routes[2 * num_dai + i].sink   = capture_dai_name;
 	}
 
-	cpu_pdev = of_find_device_by_node(out_cpu_np);
-	if (!cpu_pdev) {
-		dev_err(&pdev->dev, "failed to find SAI platform device\n");
-		return -EINVAL;
-	}
-	priv->cpu_mclk = devm_clk_get(&cpu_pdev->dev, "mclk1");
-	if (IS_ERR(priv->cpu_mclk)) {
-		ret = PTR_ERR(priv->cpu_mclk);
-		dev_err(&cpu_pdev->dev, "failed to get DAI mclk1: %d\n", ret);
-		return -EINVAL;
-	}
-
-	priv->audmix_pdev = audmix_pdev;
-	priv->out_pdev  = cpu_pdev;
-
 	priv->card.dai_link = priv->dai;
 	priv->card.num_links = priv->num_dai;
 	priv->card.codec_conf = priv->dai_conf;
-- 
2.7.4

Re: [PATCH 2/2] ASoC: imx-audmix: fix object reference leaks in probe

[Daniel Baluta]

On Tue, Apr 9, 2019 at 11:36 AM Viorel Suman <viorel.suman@nxp.com> wrote:
>
> Release the reference to the underlying device taken
> by of_find_device_by_node() call.
>
> Signed-off-by: Viorel Suman <viorel.suman@nxp.com>

Please add here the Reported-by tag pointing to Julia.

> ---
>  sound/soc/fsl/imx-audmix.c | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/sound/soc/fsl/imx-audmix.c b/sound/soc/fsl/imx-audmix.c
> index 7983bd3..7c24095 100644
> --- a/sound/soc/fsl/imx-audmix.c
> +++ b/sound/soc/fsl/imx-audmix.c
> @@ -20,10 +20,7 @@
>  #include "fsl_audmix.h"
>
>  struct imx_audmix {
> -       struct platform_device *pdev;
>         struct snd_soc_card card;
> -       struct platform_device *audmix_pdev;
> -       struct platform_device *out_pdev;

I am not sure why are you removing these members here. It doesn't seem to match
with patch description. If these are needed to simplify the code please do it in
another patch.

This patch should only fix one problem and that is the refleak.

thanks,
Daniel.

Re: [PATCH 0/2] ASoC: fsl: audmix: fix two issues

[Viorel Suman]

Please ignore this series, the device bindings documentation, [1],
still contains "model" attribute. Will send V2.

[1] Documentation/devicetree/bindings/sound/fsl,audmix.txt

Regards,
Viorel

On Ma, 2019-04-09 at 08:35 +0000, Viorel Suman wrote:
> The latest audmix patch-set (v5) had the "model" attribute
> removed as requested by Nicolin Chen, but looks like (v4)
> version of DAI driver reached "for-next" branch - fix this.
> Asside of this fix object reference leaks in machine probe reported
> by Julia Lawall.
> 
> Viorel Suman (2):
>   ASoC: fsl_audmix: remove "model" attribute
>   ASoC: imx-audmix: fix object reference leaks in probe
> 
>  sound/soc/fsl/fsl_audmix.c | 61 ++++++++++++++++++++++++----------
> ------------
>  sound/soc/fsl/imx-audmix.c | 31 +++++++++--------------
>  2 files changed, 43 insertions(+), 49 deletions(-)
> 
> -- 
> 2.7.4
>