[PATCH v2 0/3] media: atmel-isi: rework on the clock part and add runtime pm support

[PATCH v2 0/3] media: atmel-isi: rework on the clock part and add runtime pm support

[Josh Wu]

This patch series fix the peripheral clock code and enable runtime pm
support.
Also it clean up the code which is for the compatiblity of mck.

Changes in v2:
- merged v1 two patch into one.
- use runtime_pm_put() instead of runtime_pm_put_sync()
- enable peripheral clock before access ISI registers.
- totally remove clock_start()/clock_stop() as they are optional.

Josh Wu (3):
  media: atmel-isi: remove the useless code which disable isi
  media: atmel-isi: add runtime pm support
  media: atmel-isi: remove mck back compatiable code as it's not need

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

-- 
1.9.1

[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi

[Josh Wu]

To program ISI control register, the pixel clock should be enabled.
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.

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

Changes in v2:
- this file is new added.

 drivers/media/platform/soc_camera/atmel-isi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index c125b1d..31254b4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
 		return -EINVAL;
 	}
 
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
 	cfg2 = isi_readl(isi, ISI_CFG2);
 	/* Set YCC swap mode */
 	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
 
 	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
 
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
 	isi_writel(isi, ISI_CFG1, cfg1);
 
 	return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10)
 		isi->width_flags |= 1 << 9;
 
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
 	irq = platform_get_irq(pdev, 0);
 	if (IS_ERR_VALUE(irq)) {
 		ret = irq;
-- 
1.9.1

[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Thursday 09 April 2015 17:01:46 Josh Wu wrote:
> To program ISI control register, the pixel clock should be enabled.

That's an awful hardware design :-(

> So without pixel clock (from sensor) enabled, disable ISI controller is
> not make sense. So this patch remove those code.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
> Changes in v2:
> - this file is new added.
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index c125b1d..31254b4
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
> width, return -EINVAL;
>  	}
> 
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
>  	cfg2 = isi_readl(isi, ISI_CFG2);

Can the configuration registers be accessed when the pixel clock is disabled ?

>  	/* Set YCC swap mode */
>  	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
> @@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
> soc_camera_device *icd)
> 
>  	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
> 
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>  	isi_writel(isi, ISI_CFG1, cfg1);
> 
>  	return 0;
> @@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device
> *pdev) if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10)
>  		isi->width_flags |= 1 << 9;
> 
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
>  	irq = platform_get_irq(pdev, 0);
>  	if (IS_ERR_VALUE(irq)) {
>  		ret = irq;

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi

[Josh Wu]

Hi, Laurent

On 4/12/2015 9:10 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Thursday 09 April 2015 17:01:46 Josh Wu wrote:
>> To program ISI control register, the pixel clock should be enabled.
> That's an awful hardware design :-(

yes, But I need to live with this.

>
>> So without pixel clock (from sensor) enabled, disable ISI controller is
>> not make sense. So this patch remove those code.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>> Changes in v2:
>> - this file is new added.
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index c125b1d..31254b4
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
>> width, return -EINVAL;
>>   	}
>>
>> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> -
>>   	cfg2 = isi_readl(isi, ISI_CFG2);
> Can the configuration registers be accessed when the pixel clock is disabled ?

yes, it can be accessed. The CFG1, CFG2 are not impacted.

So far as I know only the ISI_CR is impacted, we can 
disable/enable/reset ISI by set ISI_CR.
Since ISI_CR register will work with pixel clock, so you need to poll 
the ISI_SR to verify your control of ISI is effective.

Best Regards,
Josh Wu

>
>>   	/* Set YCC swap mode */
>>   	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
>> @@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
>> soc_camera_device *icd)
>>
>>   	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>>
>> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>   	isi_writel(isi, ISI_CFG1, cfg1);
>>
>>   	return 0;
>> @@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device
>> *pdev) if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10)
>>   		isi->width_flags |= 1 << 9;
>>
>> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> -
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (IS_ERR_VALUE(irq)) {
>>   		ret = irq;

[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi

[Guennadi Liakhovetski]

Hi Josh,

Thanks for the patch. I'm afraid I don't quite understand why it is 
needed, could you maybe explain a bit more? Is it because

(1) during ISI configuration the pixel clock from the sensor is (usually) 
anyway disabled, so, we don't have to additionally disable the ISI, 
without the pixel clock the ISI is anyway already disabled.

or

(2) disabling the ISI at those locations below breaks something, because 
when the ISI is disabled, the functionality, that's later used isn't 
available.

I assume it's (1), but if that's the case, then this patch is just 
cosmetic, right? The ISI is anyway disabled, so, that operation simply has 
no effect, but also doesn't hurt. OTOH, if some sensor keeps its master 
clock running all the time, then switching the ISI off as in the current 
version helps save some power, unless it breaks anything?

Thanks
Guennadi

On Thu, 9 Apr 2015, Josh Wu wrote:

> To program ISI control register, the pixel clock should be enabled.
> So without pixel clock (from sensor) enabled, disable ISI controller is
> not make sense. So this patch remove those code.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
> Changes in v2:
> - this file is new added.
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index c125b1d..31254b4 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
>  		return -EINVAL;
>  	}
>  
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
>  	cfg2 = isi_readl(isi, ISI_CFG2);
>  	/* Set YCC swap mode */
>  	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
> @@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
>  
>  	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>  
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>  	isi_writel(isi, ISI_CFG1, cfg1);
>  
>  	return 0;
> @@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
>  	if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10)
>  		isi->width_flags |= 1 << 9;
>  
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
>  	irq = platform_get_irq(pdev, 0);
>  	if (IS_ERR_VALUE(irq)) {
>  		ret = irq;
> -- 
> 1.9.1
> 

[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi

[Josh Wu]

Hi, Guennadi

On 5/12/2015 4:16 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> Thanks for the patch. I'm afraid I don't quite understand why it is
> needed, could you maybe explain a bit more? Is it because
>
> (1) during ISI configuration the pixel clock from the sensor is (usually)
> anyway disabled, so, we don't have to additionally disable the ISI,
> without the pixel clock the ISI is anyway already disabled.
>
> or
>
> (2) disabling the ISI at those locations below breaks something, because
> when the ISI is disabled, the functionality, that's later used isn't
> available.
>
> I assume it's (1), but if that's the case, then this patch is just
> cosmetic, right?
Right, it is (1). This patch doesn't impact the function of isi.

The point is when we disable the ISI, we need make sure ISI peripheral 
clock and pixel clock is enabled. And also need to check the status 
register or irq to know whether the disable operation is successful. So 
best way to disable ISI is to call atmel_isi_wait_status(isi, 
WAIT_ISI_DISABLE).

 From this point, the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in 
atmel_isi_probe() is not useful as ISI peripheral clock is not enable yet.
but the other two in configure_geometry(), isi_camera_set_bus_param() 
can work and may protect the case the ISI is not in off state.

> The ISI is anyway disabled, so, that operation simply has
> no effect, but also doesn't hurt. OTOH, if some sensor keeps its master
> clock running all the time, then switching the ISI off as in the current
> version helps save some power, unless it breaks anything?
So what about to keep the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in 
configure_geometry() and isi_camera_set_bus_param().
Only remove it from atmel_isi_probe()?

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
> On Thu, 9 Apr 2015, Josh Wu wrote:
>
>> To program ISI control register, the pixel clock should be enabled.
>> So without pixel clock (from sensor) enabled, disable ISI controller is
>> not make sense. So this patch remove those code.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>> Changes in v2:
>> - this file is new added.
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index c125b1d..31254b4 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
>>   		return -EINVAL;
>>   	}
>>   
>> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> -
>>   	cfg2 = isi_readl(isi, ISI_CFG2);
>>   	/* Set YCC swap mode */
>>   	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
>> @@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
>>   
>>   	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>>   
>> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>   	isi_writel(isi, ISI_CFG1, cfg1);
>>   
>>   	return 0;
>> @@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
>>   	if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10)
>>   		isi->width_flags |= 1 << 9;
>>   
>> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> -
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (IS_ERR_VALUE(irq)) {
>>   		ret = irq;
>> -- 
>> 1.9.1
>>

[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi

[Guennadi Liakhovetski]

Hi Josh,

On Tue, 12 May 2015, Josh Wu wrote:

> Hi, Guennadi
> 
> On 5/12/2015 4:16 AM, Guennadi Liakhovetski wrote:
> > Hi Josh,
> > 
> > Thanks for the patch. I'm afraid I don't quite understand why it is
> > needed, could you maybe explain a bit more? Is it because
> > 
> > (1) during ISI configuration the pixel clock from the sensor is (usually)
> > anyway disabled, so, we don't have to additionally disable the ISI,
> > without the pixel clock the ISI is anyway already disabled.
> > 
> > or
> > 
> > (2) disabling the ISI at those locations below breaks something, because
> > when the ISI is disabled, the functionality, that's later used isn't
> > available.
> > 
> > I assume it's (1), but if that's the case, then this patch is just
> > cosmetic, right?
> Right, it is (1). This patch doesn't impact the function of isi.
> 
> The point is when we disable the ISI, we need make sure ISI peripheral clock
> and pixel clock is enabled. And also need to check the status register or irq
> to know whether the disable operation is successful. So best way to disable
> ISI is to call atmel_isi_wait_status(isi, WAIT_ISI_DISABLE).
> 
> From this point, the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in
> atmel_isi_probe() is not useful as ISI peripheral clock is not enable yet.
> but the other two in configure_geometry(), isi_camera_set_bus_param() can work
> and may protect the case the ISI is not in off state.

Ok, thanks for the explanation.

> 
> > The ISI is anyway disabled, so, that operation simply has
> > no effect, but also doesn't hurt. OTOH, if some sensor keeps its master
> > clock running all the time, then switching the ISI off as in the current
> > version helps save some power, unless it breaks anything?
> So what about to keep the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in
> configure_geometry() and isi_camera_set_bus_param().
> Only remove it from atmel_isi_probe()?

Good, let's do that.

Thanks
Guennadi

> 
> Best Regards,
> Josh Wu
> 
> > 
> > Thanks
> > Guennadi
> > 
> > On Thu, 9 Apr 2015, Josh Wu wrote:
> > 
> > > To program ISI control register, the pixel clock should be enabled.
> > > So without pixel clock (from sensor) enabled, disable ISI controller is
> > > not make sense. So this patch remove those code.
> > > 
> > > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > > ---
> > > 
> > > Changes in v2:
> > > - this file is new added.
> > > 
> > >   drivers/media/platform/soc_camera/atmel-isi.c | 5 -----
> > >   1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> > > b/drivers/media/platform/soc_camera/atmel-isi.c
> > > index c125b1d..31254b4 100644
> > > --- a/drivers/media/platform/soc_camera/atmel-isi.c
> > > +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> > > @@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi,
> > > u32 width,
> > >   		return -EINVAL;
> > >   	}
> > >   -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> > > -
> > >   	cfg2 = isi_readl(isi, ISI_CFG2);
> > >   	/* Set YCC swap mode */
> > >   	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
> > > @@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
> > > soc_camera_device *icd)
> > >     	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
> > >   -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> > >   	isi_writel(isi, ISI_CFG1, cfg1);
> > >     	return 0;
> > > @@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device
> > > *pdev)
> > >   	if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10)
> > >   		isi->width_flags |= 1 << 9;
> > >   -	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> > > -
> > >   	irq = platform_get_irq(pdev, 0);
> > >   	if (IS_ERR_VALUE(irq)) {
> > >   		ret = irq;
> > > -- 
> > > 1.9.1
> > > 
> 

[PATCH v2 2/3] media: atmel-isi: add runtime pm support

[Josh Wu]

The runtime pm resume/suspend will enable/disable pclk (ISI peripheral
clock).
And we need to call runtime_pm_get_sync()/runtime_pm_put() when we need
access ISI registers.

In the meantime, as clock_start()/clock_stop() is used to control the
mclk not ISI peripheral clock. So move this to start[stop]_streaming()
function.

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

Changes in v2:
- merged v1 two patch into one.
- use runtime_pm_put() instead of runtime_pm_put_sync()
- enable peripheral clock before access ISI registers.

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

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 31254b4..2b05f89 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include <media/atmel-isi.h>
@@ -384,6 +385,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct atmel_isi *isi = ici->priv;
 	int ret;
 
+	pm_runtime_get_sync(ici->v4l2_dev.dev);
+
 	/* Reset ISI */
 	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
 	if (ret < 0) {
@@ -441,6 +444,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");
+
+	pm_runtime_put(ici->v4l2_dev.dev);
 }
 
 static struct vb2_ops isi_video_qops = {
@@ -509,7 +514,13 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	if (mf.code != xlate->code)
 		return -EINVAL;
 
+	/* Enable PM and peripheral clock before operate isi registers */
+	pm_runtime_get_sync(ici->v4l2_dev.dev);
+
 	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
+
+	pm_runtime_put(ici->v4l2_dev.dev);
+
 	if (ret < 0)
 		return ret;
 
@@ -722,14 +733,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;
 		}
 	}
@@ -744,7 +750,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)
@@ -841,8 +846,13 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
 
 	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
 
+	/* Enable PM and peripheral clock before operate isi registers */
+	pm_runtime_get_sync(ici->v4l2_dev.dev);
+
 	isi_writel(isi, ISI_CFG1, cfg1);
 
+	pm_runtime_put(ici->v4l2_dev.dev);
+
 	return 0;
 }
 
@@ -1039,6 +1049,9 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	soc_host->v4l2_dev.dev	= &pdev->dev;
 	soc_host->nr		= pdev->id;
 
+	pm_suspend_ignore_children(&pdev->dev, true);
+	pm_runtime_enable(&pdev->dev);
+
 	if (isi->pdata.asd_sizes) {
 		soc_host->asd = isi->pdata.asd;
 		soc_host->asd_sizes = isi->pdata.asd_sizes;
@@ -1052,6 +1065,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	return 0;
 
 err_register_soc_camera_host:
+	pm_runtime_disable(&pdev->dev);
 err_req_irq:
 err_ioremap:
 	vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
@@ -1064,6 +1078,30 @@ err_alloc_ctx:
 	return ret;
 }
 
+static int atmel_isi_runtime_suspend(struct device *dev)
+{
+	struct soc_camera_host *soc_host = to_soc_camera_host(dev);
+	struct atmel_isi *isi = container_of(soc_host,
+					struct atmel_isi, soc_host);
+
+	clk_disable_unprepare(isi->pclk);
+
+	return 0;
+}
+static int atmel_isi_runtime_resume(struct device *dev)
+{
+	struct soc_camera_host *soc_host = to_soc_camera_host(dev);
+	struct atmel_isi *isi = container_of(soc_host,
+					struct atmel_isi, soc_host);
+
+	return clk_prepare_enable(isi->pclk);
+}
+
+static const struct dev_pm_ops atmel_isi_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend,
+				atmel_isi_runtime_resume, NULL)
+};
+
 static const struct of_device_id atmel_isi_of_match[] = {
 	{ .compatible = "atmel,at91sam9g45-isi" },
 	{ }
@@ -1075,6 +1113,7 @@ static struct platform_driver atmel_isi_driver = {
 	.driver		= {
 		.name = "atmel_isi",
 		.of_match_table = of_match_ptr(atmel_isi_of_match),
+		.pm	= &atmel_isi_dev_pm_ops,
 	},
 };
 
-- 
1.9.1

[PATCH v2 3/3] media: atmel-isi: remove mck back compatiable code as it's not need

[Josh Wu]

The master clock should handled by sensor itself.

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

Changes in v2:
- totally remove clock_start()/clock_stop() as they are optional.

 drivers/media/platform/soc_camera/atmel-isi.c | 45 ---------------------------
 1 file changed, 45 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 2b05f89..7bba7d9 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -83,8 +83,6 @@ struct atmel_isi {
 	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;
@@ -727,31 +725,6 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
 		 icd->devnum);
 }
 
-/* Called with .host_lock held */
-static int isi_camera_clock_start(struct soc_camera_host *ici)
-{
-	struct atmel_isi *isi = ici->priv;
-	int ret;
-
-	if (!IS_ERR(isi->mck)) {
-		ret = clk_prepare_enable(isi->mck);
-		if (ret) {
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
-/* Called with .host_lock held */
-static void isi_camera_clock_stop(struct soc_camera_host *ici)
-{
-	struct atmel_isi *isi = ici->priv;
-
-	if (!IS_ERR(isi->mck))
-		clk_disable_unprepare(isi->mck);
-}
-
 static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
 {
 	struct soc_camera_device *icd = file->private_data;
@@ -865,8 +838,6 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
 	.owner		= THIS_MODULE,
 	.add		= isi_camera_add_device,
 	.remove		= isi_camera_remove_device,
-	.clock_start	= isi_camera_clock_start,
-	.clock_stop	= isi_camera_clock_stop,
 	.set_fmt	= isi_camera_set_fmt,
 	.try_fmt	= isi_camera_try_fmt,
 	.get_formats	= isi_camera_get_formats,
@@ -904,7 +875,6 @@ static int atmel_isi_probe_dt(struct atmel_isi *isi,
 
 	/* Default settings for ISI */
 	isi->pdata.full_mode = 1;
-	isi->pdata.mck_hz = ISI_DEFAULT_MCLK_FREQ;
 	isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL;
 
 	np = of_graph_get_next_endpoint(np, NULL);
@@ -980,21 +950,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&isi->video_buffer_list);
 	INIT_LIST_HEAD(&isi->dma_desc_head);
 
-	/* ISI_MCK is the sensor master clock. It should be handled by the
-	 * sensor driver directly, as the ISI has no use for that clock. Make
-	 * the clock optional here while platforms transition to the correct
-	 * model.
-	 */
-	isi->mck = devm_clk_get(dev, "isi_mck");
-	if (!IS_ERR(isi->mck)) {
-		/* Set ISI_MCK's frequency, it should be faster than pixel
-		 * clock.
-		 */
-		ret = clk_set_rate(isi->mck, isi->pdata.mck_hz);
-		if (ret < 0)
-			return ret;
-	}
-
 	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
 				sizeof(struct fbd) * MAX_BUFFER_NUM,
 				&isi->fb_descriptors_phys,
-- 
1.9.1

[PATCH v2 3/3] media: atmel-isi: remove mck back compatiable code as it's not need

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Thursday 09 April 2015 17:01:48 Josh Wu wrote:
> The master clock should handled by sensor itself.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
> Changes in v2:
> - totally remove clock_start()/clock_stop() as they are optional.
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 45 ------------------------
>  1 file changed, 45 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index 2b05f89..7bba7d9
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -83,8 +83,6 @@ struct atmel_isi {
>  	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;
> @@ -727,31 +725,6 @@ static void isi_camera_remove_device(struct
> soc_camera_device *icd) icd->devnum);
>  }
> 
> -/* Called with .host_lock held */
> -static int isi_camera_clock_start(struct soc_camera_host *ici)
> -{
> -	struct atmel_isi *isi = ici->priv;
> -	int ret;
> -
> -	if (!IS_ERR(isi->mck)) {
> -		ret = clk_prepare_enable(isi->mck);
> -		if (ret) {
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -/* Called with .host_lock held */
> -static void isi_camera_clock_stop(struct soc_camera_host *ici)
> -{
> -	struct atmel_isi *isi = ici->priv;
> -
> -	if (!IS_ERR(isi->mck))
> -		clk_disable_unprepare(isi->mck);
> -}
> -
>  static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
>  {
>  	struct soc_camera_device *icd = file->private_data;
> @@ -865,8 +838,6 @@ static struct soc_camera_host_ops
> isi_soc_camera_host_ops = { .owner		= THIS_MODULE,
>  	.add		= isi_camera_add_device,
>  	.remove		= isi_camera_remove_device,
> -	.clock_start	= isi_camera_clock_start,
> -	.clock_stop	= isi_camera_clock_stop,
>  	.set_fmt	= isi_camera_set_fmt,
>  	.try_fmt	= isi_camera_try_fmt,
>  	.get_formats	= isi_camera_get_formats,
> @@ -904,7 +875,6 @@ static int atmel_isi_probe_dt(struct atmel_isi *isi,
> 
>  	/* Default settings for ISI */
>  	isi->pdata.full_mode = 1;
> -	isi->pdata.mck_hz = ISI_DEFAULT_MCLK_FREQ;

You can also remove the #define ISI_DEFAULT_MCLK_FREQ at the beginning of this 
file.

With this fixed,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL;
> 
>  	np = of_graph_get_next_endpoint(np, NULL);
> @@ -980,21 +950,6 @@ static int atmel_isi_probe(struct platform_device
> *pdev) INIT_LIST_HEAD(&isi->video_buffer_list);
>  	INIT_LIST_HEAD(&isi->dma_desc_head);
> 
> -	/* ISI_MCK is the sensor master clock. It should be handled by the
> -	 * sensor driver directly, as the ISI has no use for that clock. Make
> -	 * the clock optional here while platforms transition to the correct
> -	 * model.
> -	 */
> -	isi->mck = devm_clk_get(dev, "isi_mck");
> -	if (!IS_ERR(isi->mck)) {
> -		/* Set ISI_MCK's frequency, it should be faster than pixel
> -		 * clock.
> -		 */
> -		ret = clk_set_rate(isi->mck, isi->pdata.mck_hz);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
>  	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>  				sizeof(struct fbd) * MAX_BUFFER_NUM,
>  				&isi->fb_descriptors_phys,

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/3] media: atmel-isi: remove mck back compatiable code as it's not need

[Josh Wu]

Hi, Laurent

On 4/12/2015 9:08 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Thursday 09 April 2015 17:01:48 Josh Wu wrote:
>> The master clock should handled by sensor itself.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>> Changes in v2:
>> - totally remove clock_start()/clock_stop() as they are optional.
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 45 ------------------------
>>   1 file changed, 45 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index 2b05f89..7bba7d9
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -83,8 +83,6 @@ struct atmel_isi {
>>   	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;
>> @@ -727,31 +725,6 @@ static void isi_camera_remove_device(struct
>> soc_camera_device *icd) icd->devnum);
>>   }
>>
>> -/* Called with .host_lock held */
>> -static int isi_camera_clock_start(struct soc_camera_host *ici)
>> -{
>> -	struct atmel_isi *isi = ici->priv;
>> -	int ret;
>> -
>> -	if (!IS_ERR(isi->mck)) {
>> -		ret = clk_prepare_enable(isi->mck);
>> -		if (ret) {
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -/* Called with .host_lock held */
>> -static void isi_camera_clock_stop(struct soc_camera_host *ici)
>> -{
>> -	struct atmel_isi *isi = ici->priv;
>> -
>> -	if (!IS_ERR(isi->mck))
>> -		clk_disable_unprepare(isi->mck);
>> -}
>> -
>>   static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
>>   {
>>   	struct soc_camera_device *icd = file->private_data;
>> @@ -865,8 +838,6 @@ static struct soc_camera_host_ops
>> isi_soc_camera_host_ops = { .owner		= THIS_MODULE,
>>   	.add		= isi_camera_add_device,
>>   	.remove		= isi_camera_remove_device,
>> -	.clock_start	= isi_camera_clock_start,
>> -	.clock_stop	= isi_camera_clock_stop,
>>   	.set_fmt	= isi_camera_set_fmt,
>>   	.try_fmt	= isi_camera_try_fmt,
>>   	.get_formats	= isi_camera_get_formats,
>> @@ -904,7 +875,6 @@ static int atmel_isi_probe_dt(struct atmel_isi *isi,
>>
>>   	/* Default settings for ISI */
>>   	isi->pdata.full_mode = 1;
>> -	isi->pdata.mck_hz = ISI_DEFAULT_MCLK_FREQ;
> You can also remove the #define ISI_DEFAULT_MCLK_FREQ at the beginning of this
> file.

I'll remove the useless definition.

>
> With this fixed,
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

Best Regards,
Josh Wu

>
>>   	isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL;
>>
>>   	np = of_graph_get_next_endpoint(np, NULL);
>> @@ -980,21 +950,6 @@ static int atmel_isi_probe(struct platform_device
>> *pdev) INIT_LIST_HEAD(&isi->video_buffer_list);
>>   	INIT_LIST_HEAD(&isi->dma_desc_head);
>>
>> -	/* ISI_MCK is the sensor master clock. It should be handled by the
>> -	 * sensor driver directly, as the ISI has no use for that clock. Make
>> -	 * the clock optional here while platforms transition to the correct
>> -	 * model.
>> -	 */
>> -	isi->mck = devm_clk_get(dev, "isi_mck");
>> -	if (!IS_ERR(isi->mck)) {
>> -		/* Set ISI_MCK's frequency, it should be faster than pixel
>> -		 * clock.
>> -		 */
>> -		ret = clk_set_rate(isi->mck, isi->pdata.mck_hz);
>> -		if (ret < 0)
>> -			return ret;
>> -	}
>> -
>>   	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>>   				sizeof(struct fbd) * MAX_BUFFER_NUM,
>>   				&isi->fb_descriptors_phys,