On Tue, Aug 18, 2015 at 12:48 PM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
> On Mon, Aug 17, 2015 at 6:32 PM, Vaishali Thakkar
> <vthakkar1994@xxxxxxxxx> wrote:
>> On Mon, Aug 17, 2015 at 12:47 PM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>>> On Thu, Aug 13, 2015 at 4:29 PM, Vaishali Thakkar
>>> <vthakkar1994@xxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>> 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-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
