Hi Javier
Thanks for the patch. Let me try to understand what it is doing, please,
correct any my mistakes.
On Thu, 26 Jul 2012, Javier Martin wrote:
> This driver wasn't converted to the new clock framework
> (e038ed50a4a767add205094c035b6943e7b30140).
>
> Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> ---
> drivers/media/video/mx2_camera.c | 67 ++++++++++++++++++++++----------------
> 1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 3c38b5f..88dcae6 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -273,7 +273,7 @@ struct mx2_camera_dev {
> struct device *dev;
> struct soc_camera_host soc_host;
> struct soc_camera_device *icd;
> - struct clk *clk_csi, *clk_emma;
> + struct clk *clk_csi, *clk_emma_ahb, *clk_emma_ipg;
>
> unsigned int irq_csi, irq_emma;
> void __iomem *base_csi, *base_emma;
> @@ -436,7 +436,8 @@ static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
> {
> unsigned long flags;
>
> - clk_disable(pcdev->clk_csi);
> + if (cpu_is_mx27())
> + clk_disable_unprepare(pcdev->clk_csi);
This tells me, there are already 2 things going on here:
1. add clock-(un)prepare operations to enable / disable. Is this a problem
atm? is the driver non-functional without this change or is it just an API
correctness change? I thought you replied to this already, but
unfortunately I couldn't find that your reply again, sorry.
2. use clocks only on i.MX27
> writel(0, pcdev->base_csi + CSICR1);
> if (cpu_is_mx27()) {
> writel(0, pcdev->base_emma + PRP_CNTL);
> @@ -464,9 +465,11 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
> if (pcdev->icd)
> return -EBUSY;
>
> - ret = clk_enable(pcdev->clk_csi);
> - if (ret < 0)
> - return ret;
> + if (cpu_is_mx27()) {
> + ret = clk_prepare_enable(pcdev->clk_csi);
> + if (ret < 0)
> + return ret;
> + }
Same two as above.
>
> csicr1 = CSICR1_MCLKEN;
>
> @@ -1675,23 +1678,12 @@ static int __devinit mx27_camera_emma_init(struct mx2_camera_dev *pcdev)
> goto exit_iounmap;
> }
>
> - pcdev->clk_emma = clk_get(NULL, "emma");
> - if (IS_ERR(pcdev->clk_emma)) {
> - err = PTR_ERR(pcdev->clk_emma);
> - goto exit_free_irq;
> - }
> -
> - clk_enable(pcdev->clk_emma);
> -
> err = mx27_camera_emma_prp_reset(pcdev);
> if (err)
> - goto exit_clk_emma_put;
> + goto exit_free_irq;
>
> return err;
>
> -exit_clk_emma_put:
> - clk_disable(pcdev->clk_emma);
> - clk_put(pcdev->clk_emma);
> exit_free_irq:
> free_irq(pcdev->irq_emma, pcdev);
> exit_iounmap:
> @@ -1714,6 +1706,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>
> res_csi = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> irq_csi = platform_get_irq(pdev, 0);
> +
> if (res_csi == NULL || irq_csi < 0) {
> dev_err(&pdev->dev, "Missing platform resources data\n");
> err = -ENODEV;
> @@ -1727,11 +1720,26 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
> goto exit;
> }
>
> - pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> - if (IS_ERR(pcdev->clk_csi)) {
> - dev_err(&pdev->dev, "Could not get csi clock\n");
> - err = PTR_ERR(pcdev->clk_csi);
> - goto exit_kfree;
> + if (cpu_is_mx27()) {
> + 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);
> + goto exit_kfree;
> + }
> + pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "emma-ipg");
> + if (IS_ERR(pcdev->clk_emma_ipg)) {
> + err = PTR_ERR(pcdev->clk_emma_ipg);
> + goto exit_kfree;
> + }
> + pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "emma-ahb");
> + if (IS_ERR(pcdev->clk_emma_ahb)) {
> + err = PTR_ERR(pcdev->clk_emma_ahb);
> + goto exit_kfree;
> + }
> + clk_prepare_enable(pcdev->clk_csi);
> + clk_prepare_enable(pcdev->clk_emma_ipg);
> + clk_prepare_enable(pcdev->clk_emma_ahb);
> }
>
> pcdev->res_csi = res_csi;
This does:
1. add clock-(un)prepare operations to enable / disable.
2. use clocks only on i.MX27
3. switch to managed clock requesting
4. groups clk_get(_devm)() and clk_prepare(_enable)() calls together
> @@ -1827,8 +1835,8 @@ exit_free_emma:
> eallocctx:
> if (cpu_is_mx27()) {
> free_irq(pcdev->irq_emma, pcdev);
> - clk_disable(pcdev->clk_emma);
> - clk_put(pcdev->clk_emma);
> + clk_disable_unprepare(pcdev->clk_emma_ipg);
> + clk_disable_unprepare(pcdev->clk_emma_ahb);
> iounmap(pcdev->base_emma);
> release_mem_region(pcdev->res_emma->start, resource_size(pcdev->res_emma));
> }
> @@ -1840,7 +1848,11 @@ exit_iounmap:
> exit_release:
> release_mem_region(res_csi->start, resource_size(res_csi));
> exit_dma_free:
> - clk_put(pcdev->clk_csi);
> + if (cpu_is_mx27()) {
> + clk_disable_unprepare(pcdev->clk_emma_ipg);
> + clk_disable_unprepare(pcdev->clk_emma_ahb);
> + clk_disable_unprepare(pcdev->clk_csi);
> + }
> exit_kfree:
> kfree(pcdev);
> exit:
> @@ -1854,7 +1866,6 @@ static int __devexit mx2_camera_remove(struct platform_device *pdev)
> struct mx2_camera_dev, soc_host);
> struct resource *res;
>
> - clk_put(pcdev->clk_csi);
> if (cpu_is_mx25())
> free_irq(pcdev->irq_csi, pcdev);
> if (cpu_is_mx27())
> @@ -1867,8 +1878,8 @@ static int __devexit mx2_camera_remove(struct platform_device *pdev)
> iounmap(pcdev->base_csi);
>
> if (cpu_is_mx27()) {
> - clk_disable(pcdev->clk_emma);
> - clk_put(pcdev->clk_emma);
> + clk_disable_unprepare(pcdev->clk_emma_ipg);
> + clk_disable_unprepare(pcdev->clk_emma_ahb);
> iounmap(pcdev->base_emma);
> res = pcdev->res_emma;
> release_mem_region(res->start, resource_size(res));
> --
> 1.7.9.5
The rest adds to the implementation of the above 4 points.
So, if this is correct, my suggestion would be:
1. Don't make the CSI clock i.MX27-specific. You can do
- pcdev->clk_csi = clk_get(&pdev->dev, NULL);
+ pcdev->clk_csi = clk_get(&pdev->dev, "ahb");
If NULL connection ID worked for i.MX25 before, using any non-empty string
will work too.
2. I would rather not re-group clock requesting. It is not too unlogical,
that mx27_camera_emma_init() allocates EMMA clocks. And keeping the CSI
clock where it is would anyway make your grouping look less symmetric than
in this version.
3. identify the real bug-fix portion of this patch and separate it from
all other changes. Is the fix in using two separate "emma-ipg" and
"emma-ahb" clocks instead of just a single "emma?" Or is prepare /
unprepare also required? Please, make these to 1 or two _fix_ patches and
the rest you can put in a clean-up / feature / improvement patch.
Does this make sense?
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
