Patchwork [v3,2/2,media] : mx2_camera: Fix regression caused by clock conversion

login
register
mail settings
Submitter Fabio Estevam
Date Oct. 9, 2012, 2:02 p.m.
Message ID <1349791352-9829-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/190312/
State New
Headers show

Comments

Fabio Estevam - Oct. 9, 2012, 2:02 p.m.
Since mx27 transitioned to the commmon clock framework in 3.5, the correct way
to acquire the csi clock is to get csi_ahb and csi_per clocks separately.

By not doing so the camera sensor does not probe correctly:

soc-camera-pdrv soc-camera-pdrv.0: Probing soc-camera-pdrv.0
mx2-camera mx2-camera.0: Camera driver attached to camera 0
ov2640 0-0030: Product ID error fb:fb
mx2-camera mx2-camera.0: Camera driver detached from camera 0
mx2-camera mx2-camera.0: MX2 Camera (CSI) driver probed, clock frequency: 66500000

Adapt the mx2_camera driver to the new clock framework and make it functional
again.

Tested-by: Gaëtan Carlier <gcembed@gmail.com>
Tested-by: Javier Martin <javier.martin@vista-silicon.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- Fix clock error handling code as pointed out by Russell King
Changes since v1:
- Rebased against linux-next 20121008.
 drivers/media/platform/soc_camera/mx2_camera.c |   50 ++++++++++++++++++------
 1 file changed, 38 insertions(+), 12 deletions(-)
Guennadi Liakhovetski - Oct. 22, 2012, 9:07 p.m.
Hi Fabio

On Tue, 9 Oct 2012, Fabio Estevam wrote:

> Since mx27 transitioned to the commmon clock framework in 3.5, the correct way
> to acquire the csi clock is to get csi_ahb and csi_per clocks separately.
> 
> By not doing so the camera sensor does not probe correctly:
> 
> soc-camera-pdrv soc-camera-pdrv.0: Probing soc-camera-pdrv.0
> mx2-camera mx2-camera.0: Camera driver attached to camera 0
> ov2640 0-0030: Product ID error fb:fb
> mx2-camera mx2-camera.0: Camera driver detached from camera 0
> mx2-camera mx2-camera.0: MX2 Camera (CSI) driver probed, clock frequency: 66500000
> 
> Adapt the mx2_camera driver to the new clock framework and make it functional
> again.
> 
> Tested-by: Gaëtan Carlier <gcembed@gmail.com>
> Tested-by: Javier Martin <javier.martin@vista-silicon.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

I've got a question to this your patch: could you explain to me, which 
clock is obtained by

> +	pcdev->clk_csi_per = devm_clk_get(&pdev->dev, "per");

? I don't find a clock named "per" and associated with "mx2-camera.0" in 
arch/arm/mach-imx/clk-imx27.c. I only find it in clk-imx25.c. Does this 
mean, that this your patch is for i.MX25? But you're saying it's for 
i.MX27. Confused...

Thanks
Guennadi


> ---
> Changes since v2:
> - Fix clock error handling code as pointed out by Russell King
> Changes since v1:
> - Rebased against linux-next 20121008.
>  drivers/media/platform/soc_camera/mx2_camera.c |   50 ++++++++++++++++++------
>  1 file changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
> index 403d7f1..382b305 100644
> --- a/drivers/media/platform/soc_camera/mx2_camera.c
> +++ b/drivers/media/platform/soc_camera/mx2_camera.c
> @@ -272,7 +272,8 @@ struct mx2_camera_dev {
>  	struct device		*dev;
>  	struct soc_camera_host	soc_host;
>  	struct soc_camera_device *icd;
> -	struct clk		*clk_csi, *clk_emma_ahb, *clk_emma_ipg;
> +	struct clk		*clk_emma_ahb, *clk_emma_ipg;
> +	struct clk		*clk_csi_ahb, *clk_csi_per;
>  
>  	void __iomem		*base_csi, *base_emma;
>  
> @@ -432,7 +433,8 @@ static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
>  {
>  	unsigned long flags;
>  
> -	clk_disable_unprepare(pcdev->clk_csi);
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
> +	clk_disable_unprepare(pcdev->clk_csi_per);
>  	writel(0, pcdev->base_csi + CSICR1);
>  	if (cpu_is_mx27()) {
>  		writel(0, pcdev->base_emma + PRP_CNTL);
> @@ -460,10 +462,14 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>  	if (pcdev->icd)
>  		return -EBUSY;
>  
> -	ret = clk_prepare_enable(pcdev->clk_csi);
> +	ret = clk_prepare_enable(pcdev->clk_csi_ahb);
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = clk_prepare_enable(pcdev->clk_csi_per);
> +	if (ret < 0)
> +		goto exit_csi_ahb;
> +
>  	csicr1 = CSICR1_MCLKEN;
>  
>  	if (cpu_is_mx27())
> @@ -480,6 +486,11 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>  		 icd->devnum);
>  
>  	return 0;
> +
> +exit_csi_ahb:
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
> +
> +	return ret;
>  }
>  
>  static void mx2_camera_remove_device(struct soc_camera_device *icd)
> @@ -1725,27 +1736,35 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>  		goto exit;
>  	}
>  
> -	pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> -	if (IS_ERR(pcdev->clk_csi)) {
> -		dev_err(&pdev->dev, "Could not get csi clock\n");
> -		err = PTR_ERR(pcdev->clk_csi);
> +	pcdev->clk_csi_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(pcdev->clk_csi_ahb)) {
> +		dev_err(&pdev->dev, "Could not get csi ahb clock\n");
> +		err = PTR_ERR(pcdev->clk_csi_ahb);
>  		goto exit;
>  	}
>  
> +	pcdev->clk_csi_per = devm_clk_get(&pdev->dev, "per");
> +	if (IS_ERR(pcdev->clk_csi_per)) {
> +		dev_err(&pdev->dev, "Could not get csi per clock\n");
> +		err = PTR_ERR(pcdev->clk_csi_per);
> +		goto exit_csi_ahb;
> +	}
> +
>  	pcdev->pdata = pdev->dev.platform_data;
>  	if (pcdev->pdata) {
>  		long rate;
>  
>  		pcdev->platform_flags = pcdev->pdata->flags;
>  
> -		rate = clk_round_rate(pcdev->clk_csi, pcdev->pdata->clk * 2);
> +		rate = clk_round_rate(pcdev->clk_csi_per,
> +						pcdev->pdata->clk * 2);
>  		if (rate <= 0) {
>  			err = -ENODEV;
> -			goto exit;
> +			goto exit_csi_per;
>  		}
> -		err = clk_set_rate(pcdev->clk_csi, rate);
> +		err = clk_set_rate(pcdev->clk_csi_per, rate);
>  		if (err < 0)
> -			goto exit;
> +			goto exit_csi_per;
>  	}
>  
>  	INIT_LIST_HEAD(&pcdev->capture);
> @@ -1801,7 +1820,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>  		goto exit_free_emma;
>  
>  	dev_info(&pdev->dev, "MX2 Camera (CSI) driver probed, clock frequency: %ld\n",
> -			clk_get_rate(pcdev->clk_csi));
> +			clk_get_rate(pcdev->clk_csi_per));
>  
>  	return 0;
>  
> @@ -1812,6 +1831,10 @@ eallocctx:
>  		clk_disable_unprepare(pcdev->clk_emma_ipg);
>  		clk_disable_unprepare(pcdev->clk_emma_ahb);
>  	}
> +exit_csi_per:
> +	clk_disable_unprepare(pcdev->clk_csi_per);
> +exit_csi_ahb:
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
>  exit:
>  	return err;
>  }
> @@ -1831,6 +1854,9 @@ static int __devexit mx2_camera_remove(struct platform_device *pdev)
>  		clk_disable_unprepare(pcdev->clk_emma_ahb);
>  	}
>  
> +	clk_disable_unprepare(pcdev->clk_csi_per);
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
> +
>  	dev_info(&pdev->dev, "MX2 Camera driver unloaded\n");
>  
>  	return 0;
> -- 
> 1.7.9.5
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Fabio Estevam - Oct. 22, 2012, 10:17 p.m.
Hi Guennadi

On Mon, Oct 22, 2012 at 7:07 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> ? I don't find a clock named "per" and associated with "mx2-camera.0" in
> arch/arm/mach-imx/clk-imx27.c. I only find it in clk-imx25.c. Does this
> mean, that this your patch is for i.MX25? But you're saying it's for
> i.MX27. Confused...

I provide this mx27 clock in the first patch of the series:
http://patchwork.linuxtv.org/patch/14915/

Regards,

Fabio Estevam
Javier Martin - Oct. 24, 2012, 7:47 a.m.
On 23 October 2012 00:17, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Guennadi
>
> On Mon, Oct 22, 2012 at 7:07 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
>> ? I don't find a clock named "per" and associated with "mx2-camera.0" in
>> arch/arm/mach-imx/clk-imx27.c. I only find it in clk-imx25.c. Does this
>> mean, that this your patch is for i.MX25? But you're saying it's for
>> i.MX27. Confused...
>
> I provide this mx27 clock in the first patch of the series:
> http://patchwork.linuxtv.org/patch/14915/

Yes, I made the same mistake.
Guennadi Liakhovetski - Oct. 30, 2012, 7:44 a.m.
Hi Fabio

Sorry for a late review, but

On Tue, 9 Oct 2012, Fabio Estevam wrote:

> Since mx27 transitioned to the commmon clock framework in 3.5, the correct way
> to acquire the csi clock is to get csi_ahb and csi_per clocks separately.
> 
> By not doing so the camera sensor does not probe correctly:
> 
> soc-camera-pdrv soc-camera-pdrv.0: Probing soc-camera-pdrv.0
> mx2-camera mx2-camera.0: Camera driver attached to camera 0
> ov2640 0-0030: Product ID error fb:fb
> mx2-camera mx2-camera.0: Camera driver detached from camera 0
> mx2-camera mx2-camera.0: MX2 Camera (CSI) driver probed, clock frequency: 66500000
> 
> Adapt the mx2_camera driver to the new clock framework and make it functional
> again.
> 
> Tested-by: Gaëtan Carlier <gcembed@gmail.com>
> Tested-by: Javier Martin <javier.martin@vista-silicon.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v2:
> - Fix clock error handling code as pointed out by Russell King
> Changes since v1:
> - Rebased against linux-next 20121008.
>  drivers/media/platform/soc_camera/mx2_camera.c |   50 ++++++++++++++++++------
>  1 file changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
> index 403d7f1..382b305 100644
> --- a/drivers/media/platform/soc_camera/mx2_camera.c
> +++ b/drivers/media/platform/soc_camera/mx2_camera.c
> @@ -272,7 +272,8 @@ struct mx2_camera_dev {
>  	struct device		*dev;
>  	struct soc_camera_host	soc_host;
>  	struct soc_camera_device *icd;
> -	struct clk		*clk_csi, *clk_emma_ahb, *clk_emma_ipg;
> +	struct clk		*clk_emma_ahb, *clk_emma_ipg;
> +	struct clk		*clk_csi_ahb, *clk_csi_per;
>  
>  	void __iomem		*base_csi, *base_emma;
>  
> @@ -432,7 +433,8 @@ static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
>  {
>  	unsigned long flags;
>  
> -	clk_disable_unprepare(pcdev->clk_csi);
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
> +	clk_disable_unprepare(pcdev->clk_csi_per);
>  	writel(0, pcdev->base_csi + CSICR1);
>  	if (cpu_is_mx27()) {
>  		writel(0, pcdev->base_emma + PRP_CNTL);
> @@ -460,10 +462,14 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>  	if (pcdev->icd)
>  		return -EBUSY;
>  
> -	ret = clk_prepare_enable(pcdev->clk_csi);
> +	ret = clk_prepare_enable(pcdev->clk_csi_ahb);
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = clk_prepare_enable(pcdev->clk_csi_per);
> +	if (ret < 0)
> +		goto exit_csi_ahb;
> +
>  	csicr1 = CSICR1_MCLKEN;
>  
>  	if (cpu_is_mx27())
> @@ -480,6 +486,11 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>  		 icd->devnum);
>  
>  	return 0;
> +
> +exit_csi_ahb:
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
> +
> +	return ret;
>  }
>  
>  static void mx2_camera_remove_device(struct soc_camera_device *icd)
> @@ -1725,27 +1736,35 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>  		goto exit;
>  	}
>  
> -	pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> -	if (IS_ERR(pcdev->clk_csi)) {
> -		dev_err(&pdev->dev, "Could not get csi clock\n");
> -		err = PTR_ERR(pcdev->clk_csi);
> +	pcdev->clk_csi_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(pcdev->clk_csi_ahb)) {
> +		dev_err(&pdev->dev, "Could not get csi ahb clock\n");
> +		err = PTR_ERR(pcdev->clk_csi_ahb);
>  		goto exit;
>  	}
>  
> +	pcdev->clk_csi_per = devm_clk_get(&pdev->dev, "per");
> +	if (IS_ERR(pcdev->clk_csi_per)) {
> +		dev_err(&pdev->dev, "Could not get csi per clock\n");
> +		err = PTR_ERR(pcdev->clk_csi_per);
> +		goto exit_csi_ahb;
> +	}
> +
>  	pcdev->pdata = pdev->dev.platform_data;
>  	if (pcdev->pdata) {
>  		long rate;
>  
>  		pcdev->platform_flags = pcdev->pdata->flags;
>  
> -		rate = clk_round_rate(pcdev->clk_csi, pcdev->pdata->clk * 2);
> +		rate = clk_round_rate(pcdev->clk_csi_per,
> +						pcdev->pdata->clk * 2);
>  		if (rate <= 0) {
>  			err = -ENODEV;
> -			goto exit;
> +			goto exit_csi_per;
>  		}
> -		err = clk_set_rate(pcdev->clk_csi, rate);
> +		err = clk_set_rate(pcdev->clk_csi_per, rate);
>  		if (err < 0)
> -			goto exit;
> +			goto exit_csi_per;
>  	}
>  
>  	INIT_LIST_HEAD(&pcdev->capture);
> @@ -1801,7 +1820,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>  		goto exit_free_emma;
>  
>  	dev_info(&pdev->dev, "MX2 Camera (CSI) driver probed, clock frequency: %ld\n",
> -			clk_get_rate(pcdev->clk_csi));
> +			clk_get_rate(pcdev->clk_csi_per));
>  
>  	return 0;
>  
> @@ -1812,6 +1831,10 @@ eallocctx:
>  		clk_disable_unprepare(pcdev->clk_emma_ipg);
>  		clk_disable_unprepare(pcdev->clk_emma_ahb);
>  	}
> +exit_csi_per:
> +	clk_disable_unprepare(pcdev->clk_csi_per);
> +exit_csi_ahb:
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);

I don't understand why you need the above two clk_disable_unprepare() - 
you don't seem to clk_enable_prepare() your csi clocks here. Similarly

>  exit:
>  	return err;
>  }
> @@ -1831,6 +1854,9 @@ static int __devexit mx2_camera_remove(struct platform_device *pdev)
>  		clk_disable_unprepare(pcdev->clk_emma_ahb);
>  	}
>  
> +	clk_disable_unprepare(pcdev->clk_csi_per);
> +	clk_disable_unprepare(pcdev->clk_csi_ahb);
> +

I don't think the above two are needed

>  	dev_info(&pdev->dev, "MX2 Camera driver unloaded\n");
>  
>  	return 0;
> -- 
> 1.7.9.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Patch

diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
index 403d7f1..382b305 100644
--- a/drivers/media/platform/soc_camera/mx2_camera.c
+++ b/drivers/media/platform/soc_camera/mx2_camera.c
@@ -272,7 +272,8 @@  struct mx2_camera_dev {
 	struct device		*dev;
 	struct soc_camera_host	soc_host;
 	struct soc_camera_device *icd;
-	struct clk		*clk_csi, *clk_emma_ahb, *clk_emma_ipg;
+	struct clk		*clk_emma_ahb, *clk_emma_ipg;
+	struct clk		*clk_csi_ahb, *clk_csi_per;
 
 	void __iomem		*base_csi, *base_emma;
 
@@ -432,7 +433,8 @@  static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
 {
 	unsigned long flags;
 
-	clk_disable_unprepare(pcdev->clk_csi);
+	clk_disable_unprepare(pcdev->clk_csi_ahb);
+	clk_disable_unprepare(pcdev->clk_csi_per);
 	writel(0, pcdev->base_csi + CSICR1);
 	if (cpu_is_mx27()) {
 		writel(0, pcdev->base_emma + PRP_CNTL);
@@ -460,10 +462,14 @@  static int mx2_camera_add_device(struct soc_camera_device *icd)
 	if (pcdev->icd)
 		return -EBUSY;
 
-	ret = clk_prepare_enable(pcdev->clk_csi);
+	ret = clk_prepare_enable(pcdev->clk_csi_ahb);
 	if (ret < 0)
 		return ret;
 
+	ret = clk_prepare_enable(pcdev->clk_csi_per);
+	if (ret < 0)
+		goto exit_csi_ahb;
+
 	csicr1 = CSICR1_MCLKEN;
 
 	if (cpu_is_mx27())
@@ -480,6 +486,11 @@  static int mx2_camera_add_device(struct soc_camera_device *icd)
 		 icd->devnum);
 
 	return 0;
+
+exit_csi_ahb:
+	clk_disable_unprepare(pcdev->clk_csi_ahb);
+
+	return ret;
 }
 
 static void mx2_camera_remove_device(struct soc_camera_device *icd)
@@ -1725,27 +1736,35 @@  static int __devinit mx2_camera_probe(struct platform_device *pdev)
 		goto exit;
 	}
 
-	pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
-	if (IS_ERR(pcdev->clk_csi)) {
-		dev_err(&pdev->dev, "Could not get csi clock\n");
-		err = PTR_ERR(pcdev->clk_csi);
+	pcdev->clk_csi_ahb = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(pcdev->clk_csi_ahb)) {
+		dev_err(&pdev->dev, "Could not get csi ahb clock\n");
+		err = PTR_ERR(pcdev->clk_csi_ahb);
 		goto exit;
 	}
 
+	pcdev->clk_csi_per = devm_clk_get(&pdev->dev, "per");
+	if (IS_ERR(pcdev->clk_csi_per)) {
+		dev_err(&pdev->dev, "Could not get csi per clock\n");
+		err = PTR_ERR(pcdev->clk_csi_per);
+		goto exit_csi_ahb;
+	}
+
 	pcdev->pdata = pdev->dev.platform_data;
 	if (pcdev->pdata) {
 		long rate;
 
 		pcdev->platform_flags = pcdev->pdata->flags;
 
-		rate = clk_round_rate(pcdev->clk_csi, pcdev->pdata->clk * 2);
+		rate = clk_round_rate(pcdev->clk_csi_per,
+						pcdev->pdata->clk * 2);
 		if (rate <= 0) {
 			err = -ENODEV;
-			goto exit;
+			goto exit_csi_per;
 		}
-		err = clk_set_rate(pcdev->clk_csi, rate);
+		err = clk_set_rate(pcdev->clk_csi_per, rate);
 		if (err < 0)
-			goto exit;
+			goto exit_csi_per;
 	}
 
 	INIT_LIST_HEAD(&pcdev->capture);
@@ -1801,7 +1820,7 @@  static int __devinit mx2_camera_probe(struct platform_device *pdev)
 		goto exit_free_emma;
 
 	dev_info(&pdev->dev, "MX2 Camera (CSI) driver probed, clock frequency: %ld\n",
-			clk_get_rate(pcdev->clk_csi));
+			clk_get_rate(pcdev->clk_csi_per));
 
 	return 0;
 
@@ -1812,6 +1831,10 @@  eallocctx:
 		clk_disable_unprepare(pcdev->clk_emma_ipg);
 		clk_disable_unprepare(pcdev->clk_emma_ahb);
 	}
+exit_csi_per:
+	clk_disable_unprepare(pcdev->clk_csi_per);
+exit_csi_ahb:
+	clk_disable_unprepare(pcdev->clk_csi_ahb);
 exit:
 	return err;
 }
@@ -1831,6 +1854,9 @@  static int __devexit mx2_camera_remove(struct platform_device *pdev)
 		clk_disable_unprepare(pcdev->clk_emma_ahb);
 	}
 
+	clk_disable_unprepare(pcdev->clk_csi_per);
+	clk_disable_unprepare(pcdev->clk_csi_ahb);
+
 	dev_info(&pdev->dev, "MX2 Camera driver unloaded\n");
 
 	return 0;