diff mbox

[1/3] media: atmel-isi: move the peripheral clock to start/stop_stream() function

Message ID 1425531661-20040-2-git-send-email-josh.wu@atmel.com (mailing list archive)
State Superseded, archived
Delegated to: Guennadi Liakhovetski
Headers

Commit Message

Josh Wu March 5, 2015, 5 a.m. UTC 
  As the clock_start/stop() use to control the mclk for the sensor not the
ISI peripheral clock.
So we move them to start/stop_stream() function.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart March 5, 2015, 10:39 a.m. UTC | #1 
Hi Josh,

Thank you for the patch.

On Thursday 05 March 2015 13:00:59 Josh Wu wrote:
> As the clock_start/stop() use to control the mclk for the sensor not the
> ISI peripheral clock.
> So we move them to start/stop_stream() function.

Then the driver will access registers with the peripheral clock disabled, for 
instance in isi_camera_set_fmt() (calling configure_geometry), 
isi_camera_set_bus_param() or atmel_isi_probe(). Isn't that a problem ? Or are 
all registers guaranteed to be accessible (and retained) when the clock is 
disabled ?

> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index 1208818..eb179e7
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -386,6 +386,10 @@ static int start_streaming(struct vb2_queue *vq,
> unsigned int count) struct atmel_isi *isi = ici->priv;
>  	int ret;
> 
> +	ret = clk_prepare_enable(isi->pclk);
> +	if (ret)
> +		return ret;
> +
>  	/* Reset ISI */
>  	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>  	if (ret < 0) {
> @@ -445,6 +449,8 @@ static void stop_streaming(struct vb2_queue *vq)
>  	ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
>  	if (ret < 0)
>  		dev_err(icd->parent, "Disable ISI timed out\n");
> +
> +	clk_disable_unprepare(isi->pclk);
>  }
> 
>  static struct vb2_ops isi_video_qops = {
> @@ -723,14 +729,9 @@ static int isi_camera_clock_start(struct
> soc_camera_host *ici) struct atmel_isi *isi = ici->priv;
>  	int ret;
> 
> -	ret = clk_prepare_enable(isi->pclk);
> -	if (ret)
> -		return ret;
> -
>  	if (!IS_ERR(isi->mck)) {
>  		ret = clk_prepare_enable(isi->mck);
>  		if (ret) {
> -			clk_disable_unprepare(isi->pclk);
>  			return ret;
>  		}
>  	}
> @@ -745,7 +746,6 @@ static void isi_camera_clock_stop(struct soc_camera_host
> *ici)
> 
>  	if (!IS_ERR(isi->mck))
>  		clk_disable_unprepare(isi->mck);
> -	clk_disable_unprepare(isi->pclk);
>  }
> 
>  static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
Josh Wu March 6, 2015, 10:16 a.m. UTC | #2 
On 3/5/2015 6:39 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Thursday 05 March 2015 13:00:59 Josh Wu wrote:
>> As the clock_start/stop() use to control the mclk for the sensor not the
>> ISI peripheral clock.
>> So we move them to start/stop_stream() function.
> Then the driver will access registers with the peripheral clock disabled, for
> instance in isi_camera_set_fmt() (calling configure_geometry),
> isi_camera_set_bus_param() or atmel_isi_probe(). Isn't that a problem ? Or are
> all registers guaranteed to be accessible (and retained) when the clock is
> disabled ?

So far, I don't see any problem yet. But I'd like to be sure of that. 
I'll give you the feedback after more test.
Thanks for make me head up.

Best Regards,
Josh Wu

>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index 1208818..eb179e7
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -386,6 +386,10 @@ static int start_streaming(struct vb2_queue *vq,
>> unsigned int count) struct atmel_isi *isi = ici->priv;
>>   	int ret;
>>
>> +	ret = clk_prepare_enable(isi->pclk);
>> +	if (ret)
>> +		return ret;
>> +
>>   	/* Reset ISI */
>>   	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>>   	if (ret < 0) {
>> @@ -445,6 +449,8 @@ static void stop_streaming(struct vb2_queue *vq)
>>   	ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
>>   	if (ret < 0)
>>   		dev_err(icd->parent, "Disable ISI timed out\n");
>> +
>> +	clk_disable_unprepare(isi->pclk);
>>   }
>>
>>   static struct vb2_ops isi_video_qops = {
>> @@ -723,14 +729,9 @@ static int isi_camera_clock_start(struct
>> soc_camera_host *ici) struct atmel_isi *isi = ici->priv;
>>   	int ret;
>>
>> -	ret = clk_prepare_enable(isi->pclk);
>> -	if (ret)
>> -		return ret;
>> -
>>   	if (!IS_ERR(isi->mck)) {
>>   		ret = clk_prepare_enable(isi->mck);
>>   		if (ret) {
>> -			clk_disable_unprepare(isi->pclk);
>>   			return ret;
>>   		}
>>   	}
>> @@ -745,7 +746,6 @@ static void isi_camera_clock_stop(struct soc_camera_host
>> *ici)
>>
>>   	if (!IS_ERR(isi->mck))
>>   		clk_disable_unprepare(isi->mck);
>> -	clk_disable_unprepare(isi->pclk);
>>   }
>>
>>   static unsigned int isi_camera_poll(struct file *file, poll_table *pt)

--
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
diff mbox

Patch

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 1208818..eb179e7 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -386,6 +386,10 @@  static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct atmel_isi *isi = ici->priv;
 	int ret;
 
+	ret = clk_prepare_enable(isi->pclk);
+	if (ret)
+		return ret;
+
 	/* Reset ISI */
 	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
 	if (ret < 0) {
@@ -445,6 +449,8 @@  static void stop_streaming(struct vb2_queue *vq)
 	ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
 	if (ret < 0)
 		dev_err(icd->parent, "Disable ISI timed out\n");
+
+	clk_disable_unprepare(isi->pclk);
 }
 
 static struct vb2_ops isi_video_qops = {
@@ -723,14 +729,9 @@  static int isi_camera_clock_start(struct soc_camera_host *ici)
 	struct atmel_isi *isi = ici->priv;
 	int ret;
 
-	ret = clk_prepare_enable(isi->pclk);
-	if (ret)
-		return ret;
-
 	if (!IS_ERR(isi->mck)) {
 		ret = clk_prepare_enable(isi->mck);
 		if (ret) {
-			clk_disable_unprepare(isi->pclk);
 			return ret;
 		}
 	}
@@ -745,7 +746,6 @@  static void isi_camera_clock_stop(struct soc_camera_host *ici)
 
 	if (!IS_ERR(isi->mck))
 		clk_disable_unprepare(isi->mck);
-	clk_disable_unprepare(isi->pclk);
 }
 
 static unsigned int isi_camera_poll(struct file *file, poll_table *pt)