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

2015-03-06 Thread Josh Wu 

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...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-05 Thread Laurent Pinchart 
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...@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)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-04 Thread Josh Wu 
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...@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)
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html