[PATCH 1/2] [media] V4L: atmel-isi: add code to enable/disable ISI_MCK clock

[PATCH 1/2] [media] V4L: atmel-isi: add code to enable/disable ISI_MCK clock

[Josh Wu]

This patch
- add ISI_MCK clock enable/disable code.
- change field name in isi_platform_data structure

Signed-off-by: Josh Wu <josh.wu@atmel.com>
[g.liakhovetski at gmx.de: fix label names]
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hi, Guennadi

I rebase this patch to current media tree: staging/for_v3.3.
The second patch added the clk_prepare()/clk_unprepare() base on Russell King's suggestion.

Best Regards,
Josh Wu

 drivers/media/video/atmel-isi.c |   31 +++++++++++++++++++++++++++++--
 include/media/atmel-isi.h       |    4 +++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
index fbc904f..ea4eef4 100644
--- a/drivers/media/video/atmel-isi.c
+++ b/drivers/media/video/atmel-isi.c
@@ -90,7 +90,10 @@ struct atmel_isi {
 	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
 
 	struct completion		complete;
+	/* ISI peripherial clock */
 	struct clk			*pclk;
+	/* ISI_MCK, feed to camera sensor to generate pixel clock */
+	struct clk			*mck;
 	unsigned int			irq;
 
 	struct isi_platform_data	*pdata;
@@ -766,6 +769,12 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
 	if (ret)
 		return ret;
 
+	ret = clk_enable(isi->mck);
+	if (ret) {
+		clk_disable(isi->pclk);
+		return ret;
+	}
+
 	isi->icd = icd;
 	dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
 		 icd->devnum);
@@ -779,6 +788,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
 
 	BUG_ON(icd != isi->icd);
 
+	clk_disable(isi->mck);
 	clk_disable(isi->pclk);
 	isi->icd = NULL;
 
@@ -874,7 +884,7 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd, u32 pixfmt)
 
 	if (isi->pdata->has_emb_sync)
 		cfg1 |= ISI_CFG1_EMB_SYNC;
-	if (isi->pdata->isi_full_mode)
+	if (isi->pdata->full_mode)
 		cfg1 |= ISI_CFG1_FULL_MODE;
 
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
@@ -912,6 +922,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
 			isi->fb_descriptors_phys);
 
 	iounmap(isi->regs);
+	clk_put(isi->mck);
 	clk_put(isi->pclk);
 	kfree(isi);
 
@@ -930,7 +941,7 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
 	struct isi_platform_data *pdata;
 
 	pdata = dev->platform_data;
-	if (!pdata || !pdata->data_width_flags) {
+	if (!pdata || !pdata->data_width_flags || !pdata->mck_hz) {
 		dev_err(&pdev->dev,
 			"No config available for Atmel ISI\n");
 		return -EINVAL;
@@ -959,6 +970,19 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&isi->video_buffer_list);
 	INIT_LIST_HEAD(&isi->dma_desc_head);
 
+	/* Get ISI_MCK, provided by programmable clock or external clock */
+	isi->mck = clk_get(dev, "isi_mck");
+	if (IS_ERR_OR_NULL(isi->mck)) {
+		dev_err(dev, "Failed to get isi_mck\n");
+		ret = isi->mck ? PTR_ERR(isi->mck) : -EINVAL;
+		goto err_clk_get;
+	}
+
+	/* 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;
+
 	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
 				sizeof(struct fbd) * MAX_BUFFER_NUM,
 				&isi->fb_descriptors_phys,
@@ -1034,6 +1058,9 @@ err_alloc_ctx:
 			isi->p_fb_descriptors,
 			isi->fb_descriptors_phys);
 err_alloc_descriptors:
+err_set_mck_rate:
+	clk_put(isi->mck);
+err_clk_get:
 	kfree(isi);
 err_alloc_isi:
 	clk_put(pclk);
diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
index 26cece5..6568230 100644
--- a/include/media/atmel-isi.h
+++ b/include/media/atmel-isi.h
@@ -110,10 +110,12 @@ struct isi_platform_data {
 	u8 hsync_act_low;
 	u8 vsync_act_low;
 	u8 pclk_act_falling;
-	u8 isi_full_mode;
+	u8 full_mode;
 	u32 data_width_flags;
 	/* Using for ISI_CFG1 */
 	u32 frate;
+	/* Using for ISI_MCK */
+	u32 mck_hz;
 };
 
 #endif /* __ATMEL_ISI_H__ */
-- 
1.6.3.3

[PATCH 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions

[Josh Wu]

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
@@ -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;
-- 
1.6.3.3

[PATCH 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions

[Guennadi Liakhovetski]

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/

[PATCH 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions

[Wu, Josh]

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

[PATCH 1/2] [media] V4L: atmel-isi: add code to enable/disable ISI_MCK clock

[Russell King - ARM Linux]

On Wed, Nov 30, 2011 at 06:06:43PM +0800, Josh Wu wrote:
> +	/* Get ISI_MCK, provided by programmable clock or external clock */
> +	isi->mck = clk_get(dev, "isi_mck");
> +	if (IS_ERR_OR_NULL(isi->mck)) {

This should be IS_ERR()

> +		dev_err(dev, "Failed to get isi_mck\n");
> +		ret = isi->mck ? PTR_ERR(isi->mck) : -EINVAL;

		ret = PTR_ERR(isi->mck);

[PATCH 1/2] [media] V4L: atmel-isi: add code to enable/disableISI_MCK clock

[Wu, Josh]

Hi, Russell King

On Wed, Dec 07, 2011 at 4:50 PM, Russell King wrote:

> On Wed, Nov 30, 2011 at 06:06:43PM +0800, Josh Wu wrote:
>> +	/* Get ISI_MCK, provided by programmable clock or external clock
*/
>> +	isi->mck = clk_get(dev, "isi_mck");
>> +	if (IS_ERR_OR_NULL(isi->mck)) {

> This should be IS_ERR()

So it means the clk_get() will never return NULL even when clk structure
is NULL in clk lookup entry. Right?

>> +		dev_err(dev, "Failed to get isi_mck\n");
>> +		ret = isi->mck ? PTR_ERR(isi->mck) : -EINVAL;

>		ret = PTR_ERR(isi->mck);

Best Regards,
Josh Wu

[PATCH 1/2] [media] V4L: atmel-isi: add code to enable/disableISI_MCK clock

[Russell King - ARM Linux]

On Wed, Dec 07, 2011 at 06:12:52PM +0800, Wu, Josh wrote:
> Hi, Russell King
> 
> On Wed, Dec 07, 2011 at 4:50 PM, Russell King wrote:
> 
> > On Wed, Nov 30, 2011 at 06:06:43PM +0800, Josh Wu wrote:
> >> +	/* Get ISI_MCK, provided by programmable clock or external clock
> */
> >> +	isi->mck = clk_get(dev, "isi_mck");
> >> +	if (IS_ERR_OR_NULL(isi->mck)) {
> 
> > This should be IS_ERR()
> 
> So it means the clk_get() will never return NULL even when clk structure
> is NULL in clk lookup entry. Right?

It is not the drivers business to know whether NULL is valid or not.

clk_get() is defined to either return an error pointer, or a cookie
which the rest of the clk API must accept.

If an implementation decides that clk_get() can return NULL and deals
with that in the rest of the API (eg, to mean 'there is no clock but
don't fail for this') then drivers must not reject that.

If a driver rejects NULL then it is performing checks outside of the
definition of the clk API, and making assumptions about the nature of
valid cookies.

[PATCH 1/2] [media] V4L: atmel-isi: add code toenable/disableISI_MCK clock

[Wu, Josh]

On Thursday, December 08, 2011 6:40AM, Russell King wrote:

> On Wed, Dec 07, 2011 at 06:12:52PM +0800, Wu, Josh wrote:
>> Hi, Russell King
>> 
>> On Wed, Dec 07, 2011 at 4:50 PM, Russell King wrote:
>> 
>> > On Wed, Nov 30, 2011 at 06:06:43PM +0800, Josh Wu wrote:
>> >> +	/* Get ISI_MCK, provided by programmable clock or external clock
>> */
>> >> +	isi->mck = clk_get(dev, "isi_mck");
>> >> +	if (IS_ERR_OR_NULL(isi->mck)) {
>> 
>> > This should be IS_ERR()
>> 
>> So it means the clk_get() will never return NULL even when clk
structure
>> is NULL in clk lookup entry. Right?

> It is not the drivers business to know whether NULL is valid or not.

> clk_get() is defined to either return an error pointer, or a cookie
> which the rest of the clk API must accept.

> If an implementation decides that clk_get() can return NULL and deals
> with that in the rest of the API (eg, to mean 'there is no clock but
> don't fail for this') then drivers must not reject that.

> If a driver rejects NULL then it is performing checks outside of the
> definition of the clk API, and making assumptions about the nature of
> valid cookies.

Thanks for the feedback. I will send v3 patch which will not check the
null return value.

Best Regards,
Josh Wu