[v3,2/2,media] : mx2_camera: Fix regression caused by clock conversion
Commit Message
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(-)
Comments
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/
--
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
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
--
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
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.
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/
--
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
@@ -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;