Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1206565 > unrolled thread
| Started by | Vaishali Thakkar <vthakkar1994@gmail.com> |
|---|---|
| First post | 2015-08-13 09:30 +0200 |
| Last post | 2015-08-18 14:00 +0200 |
| Articles | 8 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] ASoC: tegra: Convert to managed resources Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-08-13 09:30 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Alexandre Courbot <gnurou@gmail.com> - 2015-08-17 09:20 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-08-17 11:40 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Mark Brown <broonie@kernel.org> - 2015-08-17 22:50 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-08-18 04:50 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Mark Brown <broonie@kernel.org> - 2015-08-18 07:20 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Alexandre Courbot <gnurou@gmail.com> - 2015-08-18 09:20 +0200
Re: [PATCH] ASoC: tegra: Convert to managed resources Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-08-18 14:00 +0200
| From | Vaishali Thakkar <vthakkar1994@gmail.com> |
|---|---|
| Date | 2015-08-13 09:30 +0200 |
| Subject | [PATCH] ASoC: tegra: Convert to managed resources |
| Message-ID | <pWV7B-4Ss-25@gated-at.bofh.it> |
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 <vthakkar1994@gmail.com>
---
sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
index 9141477..f69b2e4 100644
--- a/sound/soc/tegra/tegra20_spdif.c
+++ b/sound/soc/tegra/tegra20_spdif.c
@@ -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;
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Alexandre Courbot <gnurou@gmail.com> |
|---|---|
| Date | 2015-08-17 09:20 +0200 |
| Message-ID | <pYmS6-i1-13@gated-at.bofh.it> |
| In reply to | #1206565 |
On Thu, Aug 13, 2015 at 4:29 PM, Vaishali Thakkar
<vthakkar1994@gmail.com> wrote:
> 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 <vthakkar1994@gmail.com>
> ---
> sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
> 1 file changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
> index 9141477..f69b2e4 100644
> --- a/sound/soc/tegra/tegra20_spdif.c
> +++ b/sound/soc/tegra/tegra20_spdif.c
> @@ -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;
Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with
the other error cases of this function?
> }
>
> 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;
Same here.
> }
>
> 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;
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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Vaishali Thakkar <vthakkar1994@gmail.com> |
|---|---|
| Date | 2015-08-17 11:40 +0200 |
| Message-ID | <pYp3B-3mN-31@gated-at.bofh.it> |
| In reply to | #1208415 |
On Mon, Aug 17, 2015 at 12:47 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Thu, Aug 13, 2015 at 4:29 PM, Vaishali Thakkar
> <vthakkar1994@gmail.com> wrote:
>> 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 <vthakkar1994@gmail.com>
>> ---
>> sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
>> 1 file changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
>> index 9141477..f69b2e4 100644
>> --- a/sound/soc/tegra/tegra20_spdif.c
>> +++ b/sound/soc/tegra/tegra20_spdif.c
>> @@ -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;
>
> Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with
> the other error cases of this function?
>
>> }
>>
>> 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;
>
> 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.
>> }
>>
>> 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;
>
> 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.
--
Vaishali
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Mark Brown <broonie@kernel.org> |
|---|---|
| Date | 2015-08-17 22:50 +0200 |
| Message-ID | <pYzvY-1vo-9@gated-at.bofh.it> |
| In reply to | #1208501 |
[Multipart message — attachments visible in raw view] — view raw
On Mon, Aug 17, 2015 at 03:02:29PM +0530, Vaishali Thakkar wrote: > 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.
[toc] | [prev] | [next] | [standalone]
| From | Vaishali Thakkar <vthakkar1994@gmail.com> |
|---|---|
| Date | 2015-08-18 04:50 +0200 |
| Message-ID | <pYF8l-1nE-1@gated-at.bofh.it> |
| In reply to | #1208868 |
On Tue, Aug 18, 2015 at 2:17 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Aug 17, 2015 at 03:02:29PM +0530, Vaishali Thakkar wrote: > >> 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. 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? -- Vaishali -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Mark Brown <broonie@kernel.org> |
|---|---|
| Date | 2015-08-18 07:20 +0200 |
| Message-ID | <pYHtw-4T9-5@gated-at.bofh.it> |
| In reply to | #1209012 |
[Multipart message — attachments visible in raw view] — view raw
On Tue, Aug 18, 2015 at 08:15:35AM +0530, Vaishali Thakkar wrote: > 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.
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Courbot <gnurou@gmail.com> |
|---|---|
| Date | 2015-08-18 09:20 +0200 |
| Message-ID | <pYJlE-7Ap-15@gated-at.bofh.it> |
| In reply to | #1208501 |
On Mon, Aug 17, 2015 at 6:32 PM, Vaishali Thakkar
<vthakkar1994@gmail.com> wrote:
> On Mon, Aug 17, 2015 at 12:47 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Thu, Aug 13, 2015 at 4:29 PM, Vaishali Thakkar
>> <vthakkar1994@gmail.com> wrote:
>>> 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 <vthakkar1994@gmail.com>
>>> ---
>>> sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
>>> 1 file changed, 13 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
>>> index 9141477..f69b2e4 100644
>>> --- a/sound/soc/tegra/tegra20_spdif.c
>>> +++ b/sound/soc/tegra/tegra20_spdif.c
>>> @@ -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;
>>
>> Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with
>> the other error cases of this function?
>>
>>> }
>>>
>>> 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;
>>
>> 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.
Ok, I don't feel too strongly about this, so your call.
>
>>> }
>>>
>>> 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;
>>
>> 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
Ah, that will teach me to check my patches queue more often. :(
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Vaishali Thakkar <vthakkar1994@gmail.com> |
|---|---|
| Date | 2015-08-18 14:00 +0200 |
| Message-ID | <pYNIC-5h0-29@gated-at.bofh.it> |
| In reply to | #1209085 |
On Tue, Aug 18, 2015 at 12:48 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Mon, Aug 17, 2015 at 6:32 PM, Vaishali Thakkar
> <vthakkar1994@gmail.com> wrote:
>> On Mon, Aug 17, 2015 at 12:47 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> On Thu, Aug 13, 2015 at 4:29 PM, Vaishali Thakkar
>>> <vthakkar1994@gmail.com> wrote:
>>>> 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 <vthakkar1994@gmail.com>
>>>> ---
>>>> sound/soc/tegra/tegra20_spdif.c | 37 +++++++++++++------------------------
>>>> 1 file changed, 13 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
>>>> index 9141477..f69b2e4 100644
>>>> --- a/sound/soc/tegra/tegra20_spdif.c
>>>> +++ b/sound/soc/tegra/tegra20_spdif.c
>>>> @@ -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;
>>>
>>> Maybe do "return PTR_ERR(spdif->clk_spdif_out);" for consistency with
>>> the other error cases of this function?
>>>
>>>> }
>>>>
>>>> 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;
>>>
>>> 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.
>
> Ok, I don't feel too strongly about this, so your call.
>
>>
>>>> }
>>>>
>>>> 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;
>>>
>>> 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
>
> Ah, that will teach me to check my patches queue more often. :(
>
>>
>> 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.
--
Vaishali
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web