Skip to contentSkip to conversation

ASoC: tegra: Convert to managed resources

PATCH In review Vaishali Thakkar 3 participants 9 messages Aug 13, 2015 – Aug 18, 2015

Subsystems (3)
Sound ASoC Tegra architecture support
lore
Share & tools
lore

Vaishali Thakkar

author
#0 lore raw

PATCH 1/1 ASoC: tegra: Convert to managed resources

Use managed resource functions devm_clk_put and devm_snd_soc_register_component to simplify error handling.

To be compatible with the change various gotos are replaced with direct returns, and unneeded labels are dropped.

Signed-off-by Vaishali Thakkar <[email protected]>
---
 sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
sound/soc/tegra/tegra20_spdif.c +13 -24
index 9141477..f69b2e4 100644
@@ -273,45 +273,40 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 			     GFP_KERNEL);
 	if (!spdif) {
 		dev_err(&pdev->dev, "Can't allocate tegra20_spdif\n");
-		ret = -ENOMEM;
-		goto err;
+		return -ENOMEM;
 	}
 	dev_set_drvdata(&pdev->dev, spdif);
 
-	spdif->clk_spdif_out = clk_get(&pdev->dev, "spdif_out");
+	spdif->clk_spdif_out = devm_clk_get(&pdev->dev, "spdif_out");
 	if (IS_ERR(spdif->clk_spdif_out)) {
 		pr_err("Can't retrieve spdif clock\n");
 		ret = PTR_ERR(spdif->clk_spdif_out);
-		goto err;
+		return ret;
 	}
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&pdev->dev, "No memory resource\n");
-		ret = -ENODEV;
-		goto err_clk_put;
+		return -ENODEV;
 	}
 
 	dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
 	if (!dmareq) {
 		dev_err(&pdev->dev, "No DMA resource\n");
-		ret = -ENODEV;
-		goto err_clk_put;
+		return -ENODEV;
 	}
 
 	memregion = devm_request_mem_region(&pdev->dev, mem->start,
 					    resource_size(mem), DRV_NAME);
 	if (!memregion) {
 		dev_err(&pdev->dev, "Memory region already claimed\n");
-		ret = -EBUSY;
-		goto err_clk_put;
+		return -EBUSY;
 	}
 
 	regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
 	if (!regs) {
 		dev_err(&pdev->dev, "ioremap failed\n");
-		ret = -ENOMEM;
-		goto err_clk_put;
+		return -ENOMEM;
 	}
 
 	spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs,

@@ -319,7 +314,7 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 	if (IS_ERR(spdif->regmap)) {
 		dev_err(&pdev->dev, "regmap init failed\n");
 		ret = PTR_ERR(spdif->regmap);
-		goto err_clk_put;
+		return ret;
 	}
 
 	spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;

@@ -334,8 +329,9 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 			goto err_pm_disable;
 	}
 
-	ret = snd_soc_register_component(&pdev->dev, &tegra20_spdif_component,
-				   &tegra20_spdif_dai, 1);
+	ret = devm_snd_soc_register_component(&pdev->dev,
+					      &tegra20_spdif_component,
+					      &tegra20_spdif_dai, 1);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
 		ret = -ENOMEM;

@@ -345,21 +341,17 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 	ret = tegra_pcm_platform_register(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
-		goto err_unregister_component;
+		return ret;
 	}
 
 	return 0;
 
-err_unregister_component:
-	snd_soc_unregister_component(&pdev->dev);
 err_suspend:
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra20_spdif_runtime_suspend(&pdev->dev);
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-err_clk_put:
-	clk_put(spdif->clk_spdif_out);
-err:
+
 	return ret;
 }
 

@@ -372,9 +364,6 @@ static int tegra20_spdif_platform_remove(struct platform_device *pdev)
 		tegra20_spdif_runtime_suspend(&pdev->dev);
 
 	tegra_pcm_platform_unregister(&pdev->dev);
-	snd_soc_unregister_component(&pdev->dev);
-
-	clk_put(spdif->clk_spdif_out);
 
 	return 0;
 }
Signature · 1 line
1.9.1
maintainer
#1 lore raw

Applied "ASoC: tegra: Convert to managed resources" to the asoc tree

The patch

ASoC: tegra: Convert to managed resources

has been applied to the asoc tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 470805eb9f31be7b0b94cb8e0cbeb5910c47ce37 Mon Sep 17 00:00:00 2001

From: Vaishali Thakkar <[email protected]>
Date: Thu, 13 Aug 2015 12:59:21 +0530
Subject: [PATCH] ASoC: tegra: Convert to managed resources

Use managed resource functions devm_clk_put and devm_snd_soc_register_component to simplify error handling.

To be compatible with the change various gotos are replaced with direct returns, and unneeded labels are dropped.

Signed-off-by Vaishali Thakkar <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
sound/soc/tegra/tegra20_spdif.c +13 -24
index 9141477..a814da0 100644
@@ -273,45 +273,40 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 			     GFP_KERNEL);
 	if (!spdif) {
 		dev_err(&pdev->dev, "Can't allocate tegra20_spdif\n");
-		ret = -ENOMEM;
-		goto err;
+		return -ENOMEM;
 	}
 	dev_set_drvdata(&pdev->dev, spdif);
 
-	spdif->clk_spdif_out = clk_get(&pdev->dev, "spdif_out");
+	spdif->clk_spdif_out = devm_clk_get(&pdev->dev, "spdif_out");
 	if (IS_ERR(spdif->clk_spdif_out)) {
 		pr_err("Can't retrieve spdif clock\n");
 		ret = PTR_ERR(spdif->clk_spdif_out);
-		goto err;
+		return ret;
 	}
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&pdev->dev, "No memory resource\n");
-		ret = -ENODEV;
-		goto err_clk_put;
+		return -ENODEV;
 	}
 
 	dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
 	if (!dmareq) {
 		dev_err(&pdev->dev, "No DMA resource\n");
-		ret = -ENODEV;
-		goto err_clk_put;
+		return -ENODEV;
 	}
 
 	memregion = devm_request_mem_region(&pdev->dev, mem->start,
 					    resource_size(mem), DRV_NAME);
 	if (!memregion) {
 		dev_err(&pdev->dev, "Memory region already claimed\n");
-		ret = -EBUSY;
-		goto err_clk_put;
+		return -EBUSY;
 	}
 
 	regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
 	if (!regs) {
 		dev_err(&pdev->dev, "ioremap failed\n");
-		ret = -ENOMEM;
-		goto err_clk_put;
+		return -ENOMEM;
 	}
 
 	spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs,

@@ -319,7 +314,7 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 	if (IS_ERR(spdif->regmap)) {
 		dev_err(&pdev->dev, "regmap init failed\n");
 		ret = PTR_ERR(spdif->regmap);
-		goto err_clk_put;
+		return ret;
 	}
 
 	spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;

@@ -334,8 +329,9 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 			goto err_pm_disable;
 	}
 
-	ret = snd_soc_register_component(&pdev->dev, &tegra20_spdif_component,
-				   &tegra20_spdif_dai, 1);
+	ret = devm_snd_soc_register_component(&pdev->dev,
+					      &tegra20_spdif_component,
+					      &tegra20_spdif_dai, 1);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
 		ret = -ENOMEM;

@@ -345,21 +341,17 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
 	ret = tegra_pcm_platform_register(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
-		goto err_unregister_component;
+		return ret;
 	}
 
 	return 0;
 
-err_unregister_component:
-	snd_soc_unregister_component(&pdev->dev);
 err_suspend:
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra20_spdif_runtime_suspend(&pdev->dev);
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-err_clk_put:
-	clk_put(spdif->clk_spdif_out);
-err:
+
 	return ret;
 }
 

@@ -372,9 +364,6 @@ static int tegra20_spdif_platform_remove(struct platform_device *pdev)
 		tegra20_spdif_runtime_suspend(&pdev->dev);
 
 	tegra_pcm_platform_unregister(&pdev->dev);
-	snd_soc_unregister_component(&pdev->dev);
-
-	clk_put(spdif->clk_spdif_out);
 
 	return 0;
 }
Signature · 1 line
2.5.0
#2 lore raw
In reply to Vaishali Thakkar
Quoted 22 lines · Vaishali Thakkar

Use managed resource functions devm_clk_put and devm_snd_soc_register_component to simplify error handling.

To be compatible with the change various gotos are replaced with direct returns, and unneeded labels are dropped.

Signed-off-by Vaishali Thakkar <[email protected]>
---
 sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
sound/soc/tegra/tegra20_spdif.c +1 -2
index 9141477..f69b2e4 100644
@@ -273,45 +273,40 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                             GFP_KERNEL);
        if (!spdif) {
                dev_err(&pdev->dev, "Can't allocate tegra20_spdif\n");
-               ret = -ENOMEM;
-               goto err;
+               return -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 284

Quoted 10 lines
sound/soc/tegra/tegra20_spdif.c +2 -2
        }
        dev_set_drvdata(&pdev->dev, spdif);
 
-       spdif->clk_spdif_out = clk_get(&pdev->dev, "spdif_out");
+       spdif->clk_spdif_out = devm_clk_get(&pdev->dev, "spdif_out");
        if (IS_ERR(spdif->clk_spdif_out)) {
                pr_err("Can't retrieve spdif clock\n");
                ret = PTR_ERR(spdif->clk_spdif_out);
-               goto err;
+               return ret;

Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with the other error cases of this function?

Quoted 37 lines
sound/soc/tegra/tegra20_spdif.c +4 -8
        }
 
        mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!mem) {
                dev_err(&pdev->dev, "No memory resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
        if (!dmareq) {
                dev_err(&pdev->dev, "No DMA resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        memregion = devm_request_mem_region(&pdev->dev, mem->start,
                                            resource_size(mem), DRV_NAME);
        if (!memregion) {
                dev_err(&pdev->dev, "Memory region already claimed\n");
-               ret = -EBUSY;
-               goto err_clk_put;
+               return -EBUSY;
        }
 
        regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
        if (!regs) {
                dev_err(&pdev->dev, "ioremap failed\n");
-               ret = -ENOMEM;
-               goto err_clk_put;
+               return -ENOMEM;
        }
 
        spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs,

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 317

Quoted 5 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
@@ -319,7 +314,7 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        if (IS_ERR(spdif->regmap)) {
                dev_err(&pdev->dev, "regmap init failed\n");
                ret = PTR_ERR(spdif->regmap);
-               goto err_clk_put;
+               return ret;

Same here.

Quoted 16 lines
sound/soc/tegra/tegra20_spdif.c +3 -2
        }
 
        spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;

@@ -334,8 +329,9 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                        goto err_pm_disable;
        }
 
-       ret = snd_soc_register_component(&pdev->dev, &tegra20_spdif_component,
-                                  &tegra20_spdif_dai, 1);
+       ret = devm_snd_soc_register_component(&pdev->dev,
+                                             &tegra20_spdif_component,
+                                             &tegra20_spdif_dai, 1);
        if (ret) {
                dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
                ret = -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 344

Quoted 5 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
@@ -345,21 +341,17 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        ret = tegra_pcm_platform_register(&pdev->dev);
        if (ret) {
                dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
-               goto err_unregister_component;
+               return ret;

In the previous code, PM cleanup was also performed after the component was unregistered. If you return directly, this is not performed anymore - I think you should "goto err_suspend;" here.

This will change the ordering of cleanup operations though - e.g. snd_soc_unregister_component() will now be called *after* PM cleanup. Are things still working ok after this? (I suppose they do considering how simple the PM ops are, but it might be worth testing).

author
#3 lore raw
Quoted 73 lines · Alexandre Courbot
Quoted 22 lines · Vaishali Thakkar

Use managed resource functions devm_clk_put and devm_snd_soc_register_component to simplify error handling.

To be compatible with the change various gotos are replaced with direct returns, and unneeded labels are dropped.

Signed-off-by Vaishali Thakkar <[email protected]>
---
 sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
sound/soc/tegra/tegra20_spdif.c +1 -2
index 9141477..f69b2e4 100644
@@ -273,45 +273,40 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                             GFP_KERNEL);
        if (!spdif) {
                dev_err(&pdev->dev, "Can't allocate tegra20_spdif\n");
-               ret = -ENOMEM;
-               goto err;
+               return -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 284

Quoted 10 lines
sound/soc/tegra/tegra20_spdif.c +2 -2
        }
        dev_set_drvdata(&pdev->dev, spdif);
 
-       spdif->clk_spdif_out = clk_get(&pdev->dev, "spdif_out");
+       spdif->clk_spdif_out = devm_clk_get(&pdev->dev, "spdif_out");
        if (IS_ERR(spdif->clk_spdif_out)) {
                pr_err("Can't retrieve spdif clock\n");
                ret = PTR_ERR(spdif->clk_spdif_out);
-               goto err;
+               return ret;

Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with the other error cases of this function?

Quoted 35 lines
sound/soc/tegra/tegra20_spdif.c +4 -8
        }
 
        mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!mem) {
                dev_err(&pdev->dev, "No memory resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
        if (!dmareq) {
                dev_err(&pdev->dev, "No DMA resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        memregion = devm_request_mem_region(&pdev->dev, mem->start,
                                            resource_size(mem), DRV_NAME);
        if (!memregion) {
                dev_err(&pdev->dev, "Memory region already claimed\n");
-               ret = -EBUSY;
-               goto err_clk_put;
+               return -EBUSY;
        }
 
        regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
        if (!regs) {
                dev_err(&pdev->dev, "ioremap failed\n");
-               ret = -ENOMEM;
-               goto err_clk_put;
+               return -ENOMEM;
        }
Quoted 9 lines

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 317

Quoted 7 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
        spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs,

@@ -319,7 +314,7 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        if (IS_ERR(spdif->regmap)) {
                dev_err(&pdev->dev, "regmap init failed\n");
                ret = PTR_ERR(spdif->regmap);
-               goto err_clk_put;
+               return ret;

Same here.

Actually people prefer to write this way when they are calling PTR_ERR more than one time for the same value. But as for this file at both places we are calling PTR_ERR for different values, may be we can directly call it in return.

Quoted 22 lines
Quoted 16 lines
sound/soc/tegra/tegra20_spdif.c +3 -2
        }
 
        spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;

@@ -334,8 +329,9 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                        goto err_pm_disable;
        }
 
-       ret = snd_soc_register_component(&pdev->dev, &tegra20_spdif_component,
-                                  &tegra20_spdif_dai, 1);
+       ret = devm_snd_soc_register_component(&pdev->dev,
+                                             &tegra20_spdif_component,
+                                             &tegra20_spdif_dai, 1);
        if (ret) {
                dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
                ret = -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 344

Quoted 5 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
@@ -345,21 +341,17 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        ret = tegra_pcm_platform_register(&pdev->dev);
        if (ret) {
                dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
-               goto err_unregister_component;
+               return ret;
Quoted 7 lines

In the previous code, PM cleanup was also performed after the component was unregistered. If you return directly, this is not performed anymore - I think you should "goto err_suspend;" here. This will change the ordering of cleanup operations though - e.g. snd_soc_unregister_component() will now be called *after* PM cleanup. Are things still working ok after this? (I suppose they do considering how simple the PM ops are, but it might be worth testing).

I think you are right. I missed that. But now thing is , this patch is already applied here:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=topic/tegra

I am not sure if now I can change version 2 with the changes you suggested or not. Although I will not be able to test it after changing 'goto err_suspend'
as I don't have hardware but may be someone else can test it.

Signature · 1 line
Vaishali
maintainer
#4 lore raw
Quoted 8 lines · Vaishali Thakkar

I think you are right. I missed that. But now thing is , this patch is already applied here:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=topic/tegra

I am not sure if now I can change version 2 with the changes you suggested or not. Although I will not be able to test it after changing 'goto err_suspend'
as I don't have hardware but may be someone else can test it.

You can send an incremental update on top of what's there, or it may be more sensible to just discard the change entirely. I suspect we're safe with either PM vs component ordering though since we should support !PM configurations anyway.

author
#5 lore raw
Quoted 11 lines · Mark Brown
Quoted 8 lines · Vaishali Thakkar

I think you are right. I missed that. But now thing is , this patch is already applied here:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=topic/tegra

I am not sure if now I can change version 2 with the changes you suggested or not. Although I will not be able to test it after changing 'goto err_suspend'
as I don't have hardware but may be someone else can test it.

Quoted 4 lines

You can send an incremental update on top of what's there, or it may be more sensible to just discard the change entirely. I suspect we're safe with either PM vs component ordering though since we should support !PM configurations anyway.

I think yes may be code was good before this. But devm_clk_put part should work fine as call of it is before PM stuff. So, can I send a patch reverting the change
of component part or do you want me to send a patch reverting whole change?

Signature · 1 line
Vaishali
maintainer
#6 lore raw
Quoted 4 lines · Vaishali Thakkar

I think yes may be code was good before this. But devm_clk_put part should work fine as call of it is before PM stuff. So, can I send a patch reverting the change
of component part or do you want me to send a patch reverting whole change?

Just fixing the problematic part is fine.

#7 lore raw
In reply to Vaishali Thakkar
Quoted 84 lines · Vaishali Thakkar
Quoted 73 lines · Alexandre Courbot
Quoted 22 lines · Vaishali Thakkar

Use managed resource functions devm_clk_put and devm_snd_soc_register_component to simplify error handling.

To be compatible with the change various gotos are replaced with direct returns, and unneeded labels are dropped.

Signed-off-by Vaishali Thakkar <[email protected]>
---
 sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
sound/soc/tegra/tegra20_spdif.c +1 -2
index 9141477..f69b2e4 100644
@@ -273,45 +273,40 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                             GFP_KERNEL);
        if (!spdif) {
                dev_err(&pdev->dev, "Can't allocate tegra20_spdif\n");
-               ret = -ENOMEM;
-               goto err;
+               return -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 284

Quoted 10 lines
sound/soc/tegra/tegra20_spdif.c +2 -2
        }
        dev_set_drvdata(&pdev->dev, spdif);
 
-       spdif->clk_spdif_out = clk_get(&pdev->dev, "spdif_out");
+       spdif->clk_spdif_out = devm_clk_get(&pdev->dev, "spdif_out");
        if (IS_ERR(spdif->clk_spdif_out)) {
                pr_err("Can't retrieve spdif clock\n");
                ret = PTR_ERR(spdif->clk_spdif_out);
-               goto err;
+               return ret;

Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with the other error cases of this function?

Quoted 35 lines
sound/soc/tegra/tegra20_spdif.c +4 -8
        }
 
        mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!mem) {
                dev_err(&pdev->dev, "No memory resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
        if (!dmareq) {
                dev_err(&pdev->dev, "No DMA resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        memregion = devm_request_mem_region(&pdev->dev, mem->start,
                                            resource_size(mem), DRV_NAME);
        if (!memregion) {
                dev_err(&pdev->dev, "Memory region already claimed\n");
-               ret = -EBUSY;
-               goto err_clk_put;
+               return -EBUSY;
        }
 
        regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
        if (!regs) {
                dev_err(&pdev->dev, "ioremap failed\n");
-               ret = -ENOMEM;
-               goto err_clk_put;
+               return -ENOMEM;
        }
Quoted 9 lines

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 317

Quoted 7 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
        spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs,

@@ -319,7 +314,7 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        if (IS_ERR(spdif->regmap)) {
                dev_err(&pdev->dev, "regmap init failed\n");
                ret = PTR_ERR(spdif->regmap);
-               goto err_clk_put;
+               return ret;

Same here.

Quoted 4 lines

Actually people prefer to write this way when they are calling PTR_ERR more than one time for the same value. But as for this file at both places we are calling PTR_ERR for different values, may be we can directly call it in return.

Ok, I don't feel too strongly about this, so your call.

Quoted 31 lines
Quoted 23 lines
Quoted 17 lines
sound/soc/tegra/tegra20_spdif.c +3 -2
        }
 
        spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;

@@ -334,8 +329,9 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                        goto err_pm_disable;
        }
 
-       ret = snd_soc_register_component(&pdev->dev, &tegra20_spdif_component,
-                                  &tegra20_spdif_dai, 1);
+       ret = devm_snd_soc_register_component(&pdev->dev,
+                                             &tegra20_spdif_component,
+                                             &tegra20_spdif_dai, 1);
        if (ret) {
                dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
                ret = -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 344

Quoted 5 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
@@ -345,21 +341,17 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        ret = tegra_pcm_platform_register(&pdev->dev);
        if (ret) {
                dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
-               goto err_unregister_component;
+               return ret;
Quoted 7 lines

In the previous code, PM cleanup was also performed after the component was unregistered. If you return directly, this is not performed anymore - I think you should "goto err_suspend;" here. This will change the ordering of cleanup operations though - e.g. snd_soc_unregister_component() will now be called *after* PM cleanup. Are things still working ok after this? (I suppose they do considering how simple the PM ops are, but it might be worth testing).

Quoted 3 lines

I think you are right. I missed that. But now thing is , this patch is already applied here:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=topic/tegra

Ah, that will teach me to check my patches queue more often. :(

Quoted 5 lines

I am not sure if now I can change version 2 with the changes you suggested or not. Although I will not be able to test it after changing 'goto err_suspend'
as I don't have hardware but may be someone else can test it.

I think you can send a fixup patch since Mark already merged this one, this is an error code path (which by definition should not be taken too often), and there should not be any resulting breakage.

author
#8 lore raw
Quoted 131 lines · Alexandre Courbot
Quoted 84 lines · Vaishali Thakkar
Quoted 73 lines · Alexandre Courbot
Quoted 22 lines · Vaishali Thakkar

Use managed resource functions devm_clk_put and devm_snd_soc_register_component to simplify error handling.

To be compatible with the change various gotos are replaced with direct returns, and unneeded labels are dropped.

Signed-off-by Vaishali Thakkar <[email protected]>
---
 sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
sound/soc/tegra/tegra20_spdif.c +1 -2
index 9141477..f69b2e4 100644
@@ -273,45 +273,40 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                             GFP_KERNEL);
        if (!spdif) {
                dev_err(&pdev->dev, "Can't allocate tegra20_spdif\n");
-               ret = -ENOMEM;
-               goto err;
+               return -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 284

Quoted 10 lines
sound/soc/tegra/tegra20_spdif.c +2 -2
        }
        dev_set_drvdata(&pdev->dev, spdif);
 
-       spdif->clk_spdif_out = clk_get(&pdev->dev, "spdif_out");
+       spdif->clk_spdif_out = devm_clk_get(&pdev->dev, "spdif_out");
        if (IS_ERR(spdif->clk_spdif_out)) {
                pr_err("Can't retrieve spdif clock\n");
                ret = PTR_ERR(spdif->clk_spdif_out);
-               goto err;
+               return ret;

Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with the other error cases of this function?

Quoted 35 lines
sound/soc/tegra/tegra20_spdif.c +4 -8
        }
 
        mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!mem) {
                dev_err(&pdev->dev, "No memory resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
        if (!dmareq) {
                dev_err(&pdev->dev, "No DMA resource\n");
-               ret = -ENODEV;
-               goto err_clk_put;
+               return -ENODEV;
        }
 
        memregion = devm_request_mem_region(&pdev->dev, mem->start,
                                            resource_size(mem), DRV_NAME);
        if (!memregion) {
                dev_err(&pdev->dev, "Memory region already claimed\n");
-               ret = -EBUSY;
-               goto err_clk_put;
+               return -EBUSY;
        }
 
        regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
        if (!regs) {
                dev_err(&pdev->dev, "ioremap failed\n");
-               ret = -ENOMEM;
-               goto err_clk_put;
+               return -ENOMEM;
        }
Quoted 9 lines

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 317

Quoted 7 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
        spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs,

@@ -319,7 +314,7 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        if (IS_ERR(spdif->regmap)) {
                dev_err(&pdev->dev, "regmap init failed\n");
                ret = PTR_ERR(spdif->regmap);
-               goto err_clk_put;
+               return ret;

Same here.

Quoted 4 lines

Actually people prefer to write this way when they are calling PTR_ERR more than one time for the same value. But as for this file at both places we are calling PTR_ERR for different values, may be we can directly call it in return.

Ok, I don't feel too strongly about this, so your call.

Quoted 31 lines
Quoted 23 lines
Quoted 17 lines
sound/soc/tegra/tegra20_spdif.c +3 -2
        }
 
        spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;

@@ -334,8 +329,9 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
                        goto err_pm_disable;
        }
 
-       ret = snd_soc_register_component(&pdev->dev, &tegra20_spdif_component,
-                                  &tegra20_spdif_dai, 1);
+       ret = devm_snd_soc_register_component(&pdev->dev,
+                                             &tegra20_spdif_component,
+                                             &tegra20_spdif_dai, 1);
        if (ret) {
                dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
                ret = -ENOMEM;

sound/soc/tegra/tegra20_spdif.c static int tegra20_spdif_platform_probe(struct platform_device *pdev) · line 344

Quoted 5 lines
sound/soc/tegra/tegra20_spdif.c +1 -1
@@ -345,21 +341,17 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev)
        ret = tegra_pcm_platform_register(&pdev->dev);
        if (ret) {
                dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
-               goto err_unregister_component;
+               return ret;
Quoted 7 lines

In the previous code, PM cleanup was also performed after the component was unregistered. If you return directly, this is not performed anymore - I think you should "goto err_suspend;" here. This will change the ordering of cleanup operations though - e.g. snd_soc_unregister_component() will now be called *after* PM cleanup. Are things still working ok after this? (I suppose they do considering how simple the PM ops are, but it might be worth testing).

Quoted 3 lines

I think you are right. I missed that. But now thing is , this patch is already applied here:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=topic/tegra

Ah, that will teach me to check my patches queue more often. :(

Quoted 1 line
Quoted 8 lines
Quoted 4 lines

I am not sure if now I can change version 2 with the changes you suggested or not. Although I will not be able to test it after changing 'goto err_suspend'
as I don't have hardware but may be someone else can test it.

I think you can send a fixup patch since Mark already merged this one, this is an error code path (which by definition should not be taken too often), and there should not be any resulting breakage.

Yes. As per the discussion with Mark, I am sending a patch reverting the change of component part.

Thanks for your review.

Signature · 1 line
Vaishali