[2/2,media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions
Commit Message
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
drivers/media/video/atmel-isi.c | 17 ++++++++++++++++-
1 files changed, 16 insertions(+), 1 deletions(-)
Comments
Hi Josh
Thanks for the patch, but I'll ask you to fix the same thing in it, that
I've fixed for you in the first patch in this series:
On Wed, 30 Nov 2011, Josh Wu wrote:
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> drivers/media/video/atmel-isi.c | 17 ++++++++++++++++-
> 1 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> index ea4eef4..5da4381 100644
> --- a/drivers/media/video/atmel-isi.c
> +++ b/drivers/media/video/atmel-isi.c
[snip]
> @@ -978,10 +986,14 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
> goto err_clk_get;
> }
>
> + ret = clk_prepare(isi->mck);
> + if (ret)
> + goto err_set_mck_rate;
> +
> /* Set ISI_MCK's frequency, it should be faster than pixel clock */
> ret = clk_set_rate(isi->mck, pdata->mck_hz);
> if (ret < 0)
> - goto err_set_mck_rate;
> + goto err_unprepare_mck;
>
> isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
> sizeof(struct fbd) * MAX_BUFFER_NUM,
> @@ -1058,11 +1070,14 @@ err_alloc_ctx:
> isi->p_fb_descriptors,
> isi->fb_descriptors_phys);
> err_alloc_descriptors:
> +err_unprepare_mck:
> + clk_unprepare(isi->mck);
> err_set_mck_rate:
> clk_put(isi->mck);
> err_clk_get:
> kfree(isi);
> err_alloc_isi:
> + clk_unprepare(pclk);
> clk_put(pclk);
>
> return ret;
Please, use error label names consistently. As you can see, currently the
driver uses the convention
ret = do_something();
if (ret < 0)
goto err_do_something;
i.e., the label is called after the operation, that has failed, not after
the clean up step, that the control now has to jump to. Please, update
your patch to also use this convention.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Guennadi
Thank you for explain the label name rules. I've sent the v2 version
patch out. In v2 version I modified the code and make the label name
consistent.
On 12/06/2011 5:49PM, Guennadi Liakhovetski wrote:
> Hi Josh
> Thanks for the patch, but I'll ask you to fix the same thing in it,
that
> I've fixed for you in the first patch in this series:
> On Wed, 30 Nov 2011, Josh Wu wrote:
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> drivers/media/video/atmel-isi.c | 17 ++++++++++++++++-
>> 1 files changed, 16 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/atmel-isi.c
b/drivers/media/video/atmel-isi.c
>> index ea4eef4..5da4381 100644
>> --- a/drivers/media/video/atmel-isi.c
>> +++ b/drivers/media/video/atmel-isi.c
> [snip]
>> @@ -978,10 +986,14 @@ static int __devinit atmel_isi_probe(struct
platform_device *pdev)
>> goto err_clk_get;
>> }
>>
>> + ret = clk_prepare(isi->mck);
>> + if (ret)
>> + goto err_set_mck_rate;
>> +
>> /* Set ISI_MCK's frequency, it should be faster than pixel clock
*/
>> ret = clk_set_rate(isi->mck, pdata->mck_hz);
>> if (ret < 0)
>> - goto err_set_mck_rate;
>> + goto err_unprepare_mck;
>>
>> isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>> sizeof(struct fbd) * MAX_BUFFER_NUM,
>> @@ -1058,11 +1070,14 @@ err_alloc_ctx:
>> isi->p_fb_descriptors,
>> isi->fb_descriptors_phys);
>> err_alloc_descriptors:
>> +err_unprepare_mck:
>> + clk_unprepare(isi->mck);
>> err_set_mck_rate:
>> clk_put(isi->mck);
>> err_clk_get:
>> kfree(isi);
>> err_alloc_isi:
>> + clk_unprepare(pclk);
>> clk_put(pclk);
>>
>> return ret;
> Please, use error label names consistently. As you can see, currently
the
> driver uses the convention
> ret = do_something();
> if (ret < 0)
> goto err_do_something;
> i.e., the label is called after the operation, that has failed, not
after
> the clean up step, that the control now has to jump to. Please, update
> your patch to also use this convention.
Understand it now. Thank you.
> Thanks
> Guennadi
Best Regards,
Josh Wu
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -922,7 +922,9 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
isi->fb_descriptors_phys);
iounmap(isi->regs);
+ clk_unprepare(isi->mck);
clk_put(isi->mck);
+ clk_unprepare(isi->pclk);
clk_put(isi->pclk);
kfree(isi);
@@ -955,6 +957,12 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
if (IS_ERR(pclk))
return PTR_ERR(pclk);
+ ret = clk_prepare(pclk);
+ if (ret) {
+ clk_put(pclk);
+ return ret;
+ }
+
isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
if (!isi) {
ret = -ENOMEM;
@@ -978,10 +986,14 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
goto err_clk_get;
}
+ ret = clk_prepare(isi->mck);
+ if (ret)
+ goto err_set_mck_rate;
+
/* Set ISI_MCK's frequency, it should be faster than pixel clock */
ret = clk_set_rate(isi->mck, pdata->mck_hz);
if (ret < 0)
- goto err_set_mck_rate;
+ goto err_unprepare_mck;
isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
sizeof(struct fbd) * MAX_BUFFER_NUM,
@@ -1058,11 +1070,14 @@ err_alloc_ctx:
isi->p_fb_descriptors,
isi->fb_descriptors_phys);
err_alloc_descriptors:
+err_unprepare_mck:
+ clk_unprepare(isi->mck);
err_set_mck_rate:
clk_put(isi->mck);
err_clk_get:
kfree(isi);
err_alloc_isi:
+ clk_unprepare(pclk);
clk_put(pclk);
return ret;