RE: [PATCH 3/4] [media] marvell-ccic: mmp: add soc camera support on marvell-ccic mmp-driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>-----Original Message-----
>From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx]
>Sent: Monday, 01 October, 2012 18:09
>To: Albert Wang
>Cc: corbet@xxxxxxx; linux-media@xxxxxxxxxxxxxxx; Libin Yang
>Subject: Re: [PATCH 3/4] [media] marvell-ccic: mmp: add soc camera support on
>marvell-ccic mmp-driver
>
>Hi Albert
>
>On Fri, 28 Sep 2012, Albert Wang wrote:
>
>> From: Libin Yang <lbyang@xxxxxxxxxxx>
>>
>> This patch adds the support of Soc Camera on marvell-ccic mmp-driver.
>> The Soc Camera mode does not compatible with current mode.
>> Only one mode can be used at one time.
>
>Again, once these patches are split into smaller ones reviewing them should become
>easier and new issues will likely arise, but please take a look at these comments so far.
>
Yes, I understand your concern.
We will continue to discuss how to split them.

>>
>> To enable Soc Camera on mmp:
>> In Device Drivers --> Multimedia support:
>>   select Cameras/video grabbers support Then in Video capture adapters
>> --> V4L platform devices --> Camera support on Marvell MMP:
>>   select Marvell MMP camera driver based on SOC_CAMERA Also in Video
>> capture adapters --> V4L platform devices:
>>   select SoC camera support
>>   select the relevant sensor in target platform
>>
>> Also add MIPI interface and dual CCICs support in Soc Camera mode.
>>
>> Signed-off-by: Albert Wang <twang13@xxxxxxxxxxx>
>> Signed-off-by: Libin Yang <lbyang@xxxxxxxxxxx>
>> ---
>>  drivers/media/platform/Makefile                  |    4 +-
>>  drivers/media/platform/marvell-ccic/Kconfig      |   22 +++
>>  drivers/media/platform/marvell-ccic/Makefile     |    1 +
>>  drivers/media/platform/marvell-ccic/mmp-driver.c |  253 +++++++++++++++++++------
>>  include/media/mmp-camera.h                    |   13 ++
>>  5 files changed, 233 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/media/platform/Makefile
>> b/drivers/media/platform/Makefile index b7da9fa..ca60607 100755
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -146,9 +146,6 @@ obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
>>
>>  obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
>>
>> -obj-$(CONFIG_VIDEO_CAFE_CCIC) += marvell-ccic/
>> -obj-$(CONFIG_VIDEO_MMP_CAMERA) += marvell-ccic/
>> -
>>  obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
>>
>>  obj-$(CONFIG_VIDEO_OMAP3)   += omap3isp/
>> @@ -182,6 +179,7 @@ obj-$(CONFIG_VIDEO_MX1)                  +=
>mx1_camera.o
>>  obj-$(CONFIG_VIDEO_MX2)                     += mx2_camera.o
>>  obj-$(CONFIG_VIDEO_MX3)                     += mx3_camera.o
>>  obj-$(CONFIG_VIDEO_PXA27x)          += pxa_camera.o
>> +obj-$(CONFIG_VIDEO_MARVELL_CCIC)    += marvell-ccic/
>>  obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2)  += sh_mobile_csi2.o
>>  obj-$(CONFIG_VIDEO_SH_MOBILE_CEU)   += sh_mobile_ceu_camera.o
>>  obj-$(CONFIG_VIDEO_OMAP1)           += omap1_camera.o
>> diff --git a/drivers/media/platform/marvell-ccic/Kconfig
>> b/drivers/media/platform/marvell-ccic/Kconfig
>> index bf739e3..6e3eaa0 100755
>> --- a/drivers/media/platform/marvell-ccic/Kconfig
>> +++ b/drivers/media/platform/marvell-ccic/Kconfig
>> @@ -1,23 +1,45 @@
>> +config VIDEO_MARVELL_CCIC
>> +       tristate
>> +config VIDEO_MRVL_SOC_CAMERA
>> +       tristate
>> +
>>  config VIDEO_CAFE_CCIC
>>      tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
>>      depends on PCI && I2C && VIDEO_V4L2
>>      select VIDEO_OV7670
>>      select VIDEOBUF2_VMALLOC
>>      select VIDEOBUF2_DMA_CONTIG
>> +    select VIDEO_MARVELL_CCIC
>>      ---help---
>>        This is a video4linux2 driver for the Marvell 88ALP01 integrated
>>        CMOS camera controller.  This is the controller found on first-
>>        generation OLPC systems.
>>
>> +choice
>> +    prompt "Camera support on Marvell MMP"
>> +    depends on ARCH_MMP && VIDEO_V4L2
>> +    optional
>>  config VIDEO_MMP_CAMERA
>>      tristate "Marvell Armada 610 integrated camera controller support"
>>      depends on ARCH_MMP && I2C && VIDEO_V4L2
>>      select VIDEO_OV7670
>>      select I2C_GPIO
>>      select VIDEOBUF2_DMA_SG
>> +    select VIDEO_MARVELL_CCIC
>>      ---help---
>>        This is a Video4Linux2 driver for the integrated camera
>>        controller found on Marvell Armada 610 application
>>        processors (and likely beyond).  This is the controller found
>>        in OLPC XO 1.75 systems.
>>
>> +config VIDEO_MMP_SOC_CAMERA
>> +    bool "Marvell MMP camera driver based on SOC_CAMERA"
>> +    depends on VIDEO_DEV && SOC_CAMERA && ARCH_MMP && VIDEO_V4L2
>> +    select VIDEOBUF2_DMA_CONTIG
>> +    select VIDEO_MARVELL_CCIC
>> +    select VIDEO_MRVL_SOC_CAMERA
>> +    ---help---
>> +      This is a Video4Linux2 driver for the Marvell Mobile Soc
>> +      PXA910/PXA688/PXA2128/PXA988 CCIC
>> +      (CMOS Camera Interface Controller) endchoice
>> diff --git a/drivers/media/platform/marvell-ccic/Makefile
>> b/drivers/media/platform/marvell-ccic/Makefile
>> index 05a792c..d6ffd16 100755
>> --- a/drivers/media/platform/marvell-ccic/Makefile
>> +++ b/drivers/media/platform/marvell-ccic/Makefile
>> @@ -2,5 +2,6 @@ obj-$(CONFIG_VIDEO_CAFE_CCIC) += cafe_ccic.o
>> cafe_ccic-y := cafe-driver.o mcam-core.o
>>
>>  obj-$(CONFIG_VIDEO_MMP_CAMERA) += mmp_camera.o
>> +obj-$(CONFIG_VIDEO_MMP_SOC_CAMERA) += mmp_camera.o
>>  mmp_camera-y := mmp-driver.o mcam-core.o
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
>> b/drivers/media/platform/marvell-ccic/mmp-driver.c
>> index c4c17fe..afdd702 100755
>> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
>> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
>> @@ -4,6 +4,12 @@
>>   *
>>   * Copyright 2011 Jonathan Corbet <corbet@xxxxxxx>
>>   *
>> + * History:
>> + * Support Soc Camera
>> + * Support MIPI interface and Dual CCICs in Soc Camera mode
>> + * Sep-2012: Libin Yang <lbyang@xxxxxxxxxxx>
>> + *           Albert Wang <twang13@xxxxxxxxxxx>
>> + *
>>   * This file may be distributed under the terms of the GNU General
>>   * Public License, version 2.
>>   */
>> @@ -17,9 +23,6 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/slab.h>
>>  #include <linux/videodev2.h>
>> -#include <media/v4l2-device.h>
>> -#include <media/v4l2-chip-ident.h>
>> -#include <media/mmp-camera.h>
>>  #include <linux/device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/gpio.h>
>> @@ -27,6 +30,19 @@
>>  #include <linux/delay.h>
>>  #include <linux/list.h>
>>  #include <linux/pm.h>
>> +#include <linux/clk.h>
>> +#include <linux/regulator/consumer.h> #include <media/v4l2-device.h>
>> +#include <media/v4l2-chip-ident.h> #include
>> +<media/videobuf2-dma-contig.h> #include <media/mmp-camera.h> #ifdef
>> +CONFIG_VIDEO_MMP_SOC_CAMERA
>
>Like in #2/4: no need for an ifdef.
>
Yes.

>> +#include <media/soc_camera.h>
>> +#include <media/soc_mediabus.h>
>> +#endif
>> +
>> +#include <mach/regs-apmu.h>
>> +#include <plat/mfp.h>
>>
>>  #include "mcam-core.h"
>>
>> @@ -39,6 +55,7 @@ struct mmp_camera {
>>      struct platform_device *pdev;
>>      struct mcam_camera mcam;
>>      struct list_head devlist;
>> +    struct clk *clk[3];     /* CCIC_GATE, CCIC_RST, CCIC_DBG clocks */
>
>I don't see clk[1] amd clk[2] used anywhere in your patches. If they are indeed needed
>and unless you really want to address them as an array, it might be better to use a
>descriptive name for each clock instead of the array.
>
Actually, we used all clocks in our soc platform files.
For this patch, maybe we need find a better method to handle it.

>>      int irq;
>>  };
>>
>> @@ -90,15 +107,7 @@ static struct mmp_camera *mmpcam_find_device(struct
>platform_device *pdev)
>>      return NULL;
>>  }
>>
>> -
>> -
>> -
>> -/*
>> - * Power-related registers; this almost certainly belongs
>> - * somewhere else.
>> - *
>> - * ARMADA 610 register manual, sec 7.2.1, p1842.
>> - */
>
>Why are you removing this comment?
>
Actually, these regs address are same in both ARMADA 610 and other MMP SOC family.
We think it is out-dated.
But of course, we can keep these comments.

>> +#ifdef CONFIG_VIDEO_MMP_CAMERA
>>  #define CPU_SUBSYS_PMU_BASE 0xd4282800
>>  #define REG_CCIC_DCGCR              0x28    /* CCIC dyn clock gate ctrl reg */
>>  #define REG_CCIC_CRCR               0x50    /* CCIC clk reset ctrl reg      */
>> @@ -117,13 +126,13 @@ static void mmpcam_power_up(struct mcam_camera
>> *mcam)  {
>>      struct mmp_camera *cam = mcam_to_cam(mcam);
>>      struct mmp_camera_platform_data *pdata;
>> -/*
>> - * Turn on power and clocks to the controller.
>> - */
>> +    /*
>> +     * Turn on power and clocks to the controller.
>> +     */
>>      mmpcam_power_up_ctlr(cam);
>> -/*
>> - * Provide power to the sensor.
>> - */
>> +    /*
>> +     * Provide power to the sensor.
>> +     */
>>      mcam_reg_write(mcam, REG_CLKCTRL, 0x60000002);
>>      pdata = cam->pdev->dev.platform_data;
>>      gpio_set_value(pdata->sensor_power_gpio, 1); @@ -139,19 +148,91 @@
>> static void mmpcam_power_down(struct mcam_camera *mcam)  {
>>      struct mmp_camera *cam = mcam_to_cam(mcam);
>>      struct mmp_camera_platform_data *pdata;
>> -/*
>> - * Turn off clocks and set reset lines
>> - */
>> +    /*
>> +     * Turn off clocks and set reset lines
>> +     */
>>      iowrite32(0, cam->power_regs + REG_CCIC_DCGCR);
>>      iowrite32(0, cam->power_regs + REG_CCIC_CRCR);
>> -/*
>> - * Shut down the sensor.
>> - */
>> +    /*
>> +     * Shut down the sensor.
>> +     */
>
>These comment indentation changes do not belong in this patch.
>
OK.

>>      pdata = cam->pdev->dev.platform_data;
>>      gpio_set_value(pdata->sensor_power_gpio, 0);
>>      gpio_set_value(pdata->sensor_reset_gpio, 0);  }
>>
>> +void mmpcam_calc_dphy(struct mcam_camera *mcam,
>> +                    struct v4l2_subdev_frame_interval *inter) {
>> +    return;
>> +}
>> +#else
>> +/* setup the camera clk gate in APMU */ static void
>> +mmpcam_island_clk(struct mmp_camera *cam, int on) {
>> +    if (on)
>> +            clk_enable(cam->clk[0]);
>> +    else
>> +            clk_disable(cam->clk[0]);
>
>I would remove this function and inline clk_enable() / clk_disable() directly below.
>
OK, we can think about your suggestion after review code again.

>> +}
>> +
>> +static void mmpcam_power_up(struct mcam_camera *mcam) {
>> +    struct mmp_camera *cam = mcam_to_cam(mcam);
>> +    mmpcam_island_clk(cam, 1);
>> +}
>> +
>> +static void mmpcam_power_up_ctlr(struct mmp_camera *cam) {
>> +    mmpcam_power_up(&cam->mcam);
>> +}
>> +
>> +static void mmpcam_power_down(struct mcam_camera *mcam) {
>> +    struct mmp_camera *cam = mcam_to_cam(mcam);
>> +    mmpcam_island_clk(cam, 0);
>> +}
>> +
>> +void mmpcam_calc_dphy(struct mcam_camera *mcam,
>> +                    struct v4l2_subdev_frame_interval *inter) {
>> +    struct mmp_camera *cam = mcam_to_cam(mcam);
>> +    struct mmp_camera_platform_data *pdata = cam->pdev->dev.platform_data;
>> +    struct device *dev = &cam->pdev->dev;
>> +    unsigned long tx_clk_esc;
>> +    struct clk *pll1;
>> +
>> +    if (pdata->dphy3_algo == 1)
>> +            /*
>> +             * dphy3_algo == 1
>> +             * Calculate CSI2_DPHY3 algo for PXA910
>> +             */
>> +            pdata->dphy[0] = ((1 + inter->pad * 80 / 1000) & 0xff) << 8
>
>Ouch, this looks like an abuse to me. The pad API isn't yet supported by soc-camera,
>nor by the ccic driver, AFAICS. Your use of struct v4l2_subdev_frame_interval::pad here
>doesn't seem valid. I think using struct v4l2_fract directly for this function would be
>better. In patch 2/4 in mcam_camera_set_fmt() you assign
>
>+      inter.pad = mcam->mclk_min;
>
>and then call
>
>+      mcam->calc_dphy(mcam, &inter);
>
>where .mclk_min is anyway coming from mmpcam_probe() as
>
>+      mcam->mclk_min = pdata->mclk_min;
>
>so, looks like you don't need to pass .mclk_min to mmpcam_calc_dphy() at all. As for
>calling subdevice .g_frame_interval() method with that struct
>
>+      ret = v4l2_subdev_call(sd, video, g_frame_interval, &inter);
>
>that's an abuse too - the clock frequency has nothing to do with the pad number. The
>clock frequency should be passed to the sensor by different means. I think, this is an
>example of the problem, that has been discussed at the media mini-summit a month ago.
>The correct way to solve it would be to have your bridge driver export a clock for the
>sensor. If you cannot do that yet, we have to think about a different way to achieve this,
>please, let us know.
>
OK, we need an internal discussion for this usage.

>> +                    | (1 + inter->pad * 35 / 1000);
>> +    else if (pdata->dphy3_algo == 2)
>> +            /*
>> +             * dphy3_algo == 2
>> +             * Calculate CSI2_DPHY3 algo for PXA2128
>> +             */
>> +            pdata->dphy[0] = ((2 + inter->pad * 110 / 1000) & 0xff) << 8
>> +                    | (1 + inter->pad * 35 / 1000);
>> +
>> +    pll1 = clk_get(dev, "pll1");
>> +    if (IS_ERR(pll1)) {
>> +            dev_err(dev, "Could not get pll1 clock\n");
>> +            return;
>> +    }
>> +
>> +    tx_clk_esc = clk_get_rate(pll1) / 1000000 / 12;
>> +    clk_put(pll1);
>
>You are jeeping the clock frequency value around, so, you probably assume, that it will
>stay constant. However, as soon as you release the clock by calling clk_put() its
>frequency can be changed by another user. So, I think you should hold a reference to
>the clock for as long as you're using it.
>
Yes, maybe we need change it.

>> +
>> +    /*
>> +     * Update dphy6 according to current tx_clk_esc
>> +     */
>> +    pdata->dphy[2] = ((534 * tx_clk_esc / 2000 - 1) & 0xff) << 8
>> +                    | ((38 * tx_clk_esc / 1000 - 1) & 0xff);
>> +}
>> +#endif
>>
>>  static irqreturn_t mmpcam_irq(int irq, void *data)
>>  {
>> @@ -160,12 +241,15 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
>>
>>      spin_lock(&mcam->dev_lock);
>>      irqs = mcam_reg_read(mcam, REG_IRQSTAT);
>> +    if (!(irqs & FRAMEIRQS)) {
>> +            spin_unlock(&mcam->dev_lock);
>> +            return IRQ_NONE;
>> +    }
>
>This is a change in behaviour of the original driver and has to be
>submitted and tested on the original hardware separately.
>
Yes.

>>      handled = mccic_irq(mcam, irqs);
>>      spin_unlock(&mcam->dev_lock);
>>      return IRQ_RETVAL(handled);
>>  }
>>
>> -
>>  static int mmpcam_probe(struct platform_device *pdev)
>>  {
>>      struct mmp_camera *cam;
>> @@ -174,35 +258,76 @@ static int mmpcam_probe(struct platform_device *pdev)
>>      struct mmp_camera_platform_data *pdata;
>>      int ret;
>>
>> -    cam = kzalloc(sizeof(*cam), GFP_KERNEL);
>> +    pdata = pdev->dev.platform_data;
>> +
>> +    cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL);
>
>Switching to managed allocations is nice, but should also be a separate
>patch.
>
OK, we will prepare another patch to do that.

>>      if (cam == NULL)
>>              return -ENOMEM;
>> +
>>      cam->pdev = pdev;
>>      INIT_LIST_HEAD(&cam->devlist);
>> -
>> +    spin_lock_init(&mcam->dev_lock);
>> +    cam->clk[0] = clk_get(&pdev->dev, pdata->clk_name[0]);
>>      mcam = &cam->mcam;
>>      mcam->plat_power_up = mmpcam_power_up;
>>      mcam->plat_power_down = mmpcam_power_down;
>> +    mcam->calc_dphy = mmpcam_calc_dphy;
>>      mcam->dev = &pdev->dev;
>>      mcam->use_smbus = 0;
>> +    mcam->bus_type = pdata->bus_type;
>> +    mcam->ccic_id = pdev->id;
>> +    mcam->card_name = pdata->name;
>> +    mcam->mclk_min = pdata->mclk_min;
>> +    mcam->mclk_src = pdata->mclk_src;
>> +    mcam->mclk_div = pdata->mclk_div;
>> +    mcam->dphy = &(pdata->dphy);
>> +    mcam->mipi_enabled = 0;
>> +    mcam->lane = pdata->lane;
>> +#ifdef CONFIG_VIDEO_MMP_SOC_CAMERA
>> +    mcam->chip_id = pdata->chip_id;
>> +    mcam->buffer_mode = B_DMA_contig;
>> +#else
>>      mcam->chip_id = V4L2_IDENT_ARMADA610;
>>      mcam->buffer_mode = B_DMA_sg;
>> -    spin_lock_init(&mcam->dev_lock);
>> +#endif
>> +    switch (pdata->dma_burst) {
>> +    case 128:
>> +            mcam->burst = C1_DMAB128;
>> +            break;
>> +    case 256:
>> +            mcam->burst = C1_DMAB256;
>> +            break;
>> +    default:
>> +            mcam->burst = C1_DMAB64;
>> +            break;
>> +    }
>> +    INIT_LIST_HEAD(&mcam->buffers);
>>      /*
>>       * Get our I/O memory.
>>       */
>>      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>      if (res == NULL) {
>>              dev_err(&pdev->dev, "no iomem resource!\n");
>> -            ret = -ENODEV;
>> -            goto out_free;
>> +            return -ENODEV;
>>      }
>> -    mcam->regs = ioremap(res->start, resource_size(res));
>> +    mcam->regs = devm_request_and_ioremap(&pdev->dev, res);
>>      if (mcam->regs == NULL) {
>>              dev_err(&pdev->dev, "MMIO ioremap fail\n");
>> -            ret = -ENODEV;
>> -            goto out_free;
>> +            return -ENODEV;
>>      }
>> +
>> +#ifdef CONFIG_VIDEO_MMP_SOC_CAMERA
>> +    mcam->vb_alloc_ctx = (struct vb2_alloc_ctx *)
>> +            vb2_dma_contig_init_ctx(&pdev->dev);
>
>No need for a type-cast. In fact, cannot you use vb2_dma_contig support,
>already implemented in mcam-core.c::mcam_setup_vb2()?
>
It seems we need review code again.

>> +    if (IS_ERR(mcam->vb_alloc_ctx))
>> +            return PTR_ERR(mcam->vb_alloc_ctx);
>> +
>> +    ret = mcam_soc_camera_host_register(mcam);
>> +    if (ret)
>> +            goto out_free_ctx;
>> +#endif
>> +
>> +#ifdef CONFIG_VIDEO_MMP_CAMERA
>>      /*
>>       * Power/clock memory is elsewhere; get it too.  Perhaps this
>>       * should really be managed outside of this driver?
>> @@ -211,24 +336,25 @@ static int mmpcam_probe(struct platform_device *pdev)
>>      if (res == NULL) {
>>              dev_err(&pdev->dev, "no power resource!\n");
>>              ret = -ENODEV;
>> -            goto out_unmap1;
>> +            goto out_host_unregister;
>>      }
>> -    cam->power_regs = ioremap(res->start, resource_size(res));
>> +    cam->power_regs = devm_request_and_ioremap(&pdev->dev, res);
>>      if (cam->power_regs == NULL) {
>>              dev_err(&pdev->dev, "power MMIO ioremap fail\n");
>>              ret = -ENODEV;
>> -            goto out_unmap1;
>> +            goto out_host_unregister;
>>      }
>> +
>>      /*
>>       * Find the i2c adapter.  This assumes, of course, that the
>>       * i2c bus is already up and functioning.
>> +     * soc-camera manages i2c interface in sensor side
>>       */
>> -    pdata = pdev->dev.platform_data;
>>      mcam->i2c_adapter = platform_get_drvdata(pdata->i2c_device);
>>      if (mcam->i2c_adapter == NULL) {
>>              ret = -ENODEV;
>>              dev_err(&pdev->dev, "No i2c adapter\n");
>> -            goto out_unmap2;
>> +            goto out_host_unregister;
>>      }
>>      /*
>>       * Sensor GPIO pins.
>> @@ -237,23 +363,26 @@ static int mmpcam_probe(struct platform_device *pdev)
>>      if (ret) {
>>              dev_err(&pdev->dev, "Can't get sensor power gpio %d",
>>                              pdata->sensor_power_gpio);
>> -            goto out_unmap2;
>> +            goto out_host_unregister;
>>      }
>>      gpio_direction_output(pdata->sensor_power_gpio, 0);
>>      ret = gpio_request(pdata->sensor_reset_gpio, "cam-reset");
>>      if (ret) {
>>              dev_err(&pdev->dev, "Can't get sensor reset gpio %d",
>>                              pdata->sensor_reset_gpio);
>> -            goto out_gpio;
>> +            goto out_gpio1;
>>      }
>>      gpio_direction_output(pdata->sensor_reset_gpio, 0);
>> +
>>      /*
>>       * Power the device up and hand it off to the core.
>>       */
>>      mmpcam_power_up(mcam);
>> +#endif
>>      ret = mccic_register(mcam);
>>      if (ret)
>>              goto out_gpio2;
>> +
>>      /*
>>       * Finally, set up our IRQ now that the core is ready to
>>       * deal with it.
>> @@ -264,7 +393,7 @@ static int mmpcam_probe(struct platform_device *pdev)
>>              goto out_unregister;
>>      }
>>      cam->irq = res->start;
>> -    ret = request_irq(cam->irq, mmpcam_irq, IRQF_SHARED,
>> +    ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARED,
>>                      "mmp-camera", mcam);
>>      if (ret == 0) {
>>              mmpcam_add_device(cam);
>> @@ -274,35 +403,46 @@ static int mmpcam_probe(struct platform_device *pdev)
>>  out_unregister:
>>      mccic_shutdown(mcam);
>>  out_gpio2:
>> +#ifdef CONFIG_VIDEO_MMP_CAMERA
>>      mmpcam_power_down(mcam);
>>      gpio_free(pdata->sensor_reset_gpio);
>> -out_gpio:
>> +out_gpio1:
>>      gpio_free(pdata->sensor_power_gpio);
>> -out_unmap2:
>> -    iounmap(cam->power_regs);
>> -out_unmap1:
>> -    iounmap(mcam->regs);
>> -out_free:
>> -    kfree(cam);
>> +out_host_unregister:
>> +#endif
>> +#ifdef CONFIG_VIDEO_MMP_SOC_CAMERA
>> +    soc_camera_host_unregister(&mcam->soc_host);
>> +out_free_ctx:
>> +    vb2_dma_contig_cleanup_ctx(mcam->vb_alloc_ctx);
>> +    mcam->vb_alloc_ctx = NULL;
>> +#endif
>>      return ret;
>>  }
>>
>> -
>>  static int mmpcam_remove(struct mmp_camera *cam)
>>  {
>>      struct mcam_camera *mcam = &cam->mcam;
>> -    struct mmp_camera_platform_data *pdata;
>> +#ifdef CONFIG_VIDEO_MMP_SOC_CAMERA
>> +    struct soc_camera_host *soc_host = &mcam->soc_host;
>> +#else
>> +    struct mmp_camera_platform_data *pdata = cam->pdev->dev.platform_data;
>> +#endif
>>
>>      mmpcam_remove_device(cam);
>> -    free_irq(cam->irq, mcam);
>> +    devm_free_irq(&cam->pdev->dev, cam->irq, mcam);
>
>Why is this needed? This should happen automatically.
>
Yes, maybe it's our fault.

>>      mccic_shutdown(mcam);
>>      mmpcam_power_down(mcam);
>> -    pdata = cam->pdev->dev.platform_data;
>> +#ifdef CONFIG_VIDEO_MMP_SOC_CAMERA
>> +    soc_camera_host_unregister(soc_host);
>> +    vb2_dma_contig_cleanup_ctx(mcam->vb_alloc_ctx);
>> +    mcam->vb_alloc_ctx = NULL;
>> +#else
>>      gpio_free(pdata->sensor_reset_gpio);
>>      gpio_free(pdata->sensor_power_gpio);
>> -    iounmap(cam->power_regs);
>> -    iounmap(mcam->regs);
>> -    kfree(cam);
>> +    devm_iounmap(&cam->pdev->dev, cam->power_regs);
>> +#endif
>> +    devm_iounmap(&cam->pdev->dev, mcam->regs);
>> +    devm_kfree(&cam->pdev->dev, cam);
>
>Same holds for these - device managed resources are freed upon driver
>unbinding.
>
Yes.
devm_kfree do nothing, maybe it just pair for passing original mem leak check tool. :)
Anyway, we can remove the free operation.

>>      return 0;
>>  }
>>
>> @@ -312,6 +452,7 @@ static int mmpcam_platform_remove(struct platform_device
>*pdev)
>>
>>      if (cam == NULL)
>>              return -ENODEV;
>> +
>>      return mmpcam_remove(cam);
>>  }
>>
>> @@ -345,7 +486,6 @@ static int mmpcam_resume(struct platform_device *pdev)
>>
>>  #endif
>>
>> -
>>  static struct platform_driver mmpcam_driver = {
>>      .probe          = mmpcam_probe,
>>      .remove         = mmpcam_platform_remove,
>> @@ -359,7 +499,6 @@ static struct platform_driver mmpcam_driver = {
>>      }
>>  };
>>
>> -
>>  static int __init mmpcam_init_module(void)
>>  {
>>      mutex_init(&mmpcam_devices_lock);
>> diff --git a/include/media/mmp-camera.h b/include/media/mmp-camera.h
>> index 7611963..f90f8c4 100755
>> --- a/include/media/mmp-camera.h
>> +++ b/include/media/mmp-camera.h
>> @@ -6,4 +6,17 @@ struct mmp_camera_platform_data {
>>      struct platform_device *i2c_device;
>>      int sensor_power_gpio;
>>      int sensor_reset_gpio;
>> +    char *clk_name[3];
>> +    char name[16];
>> +    int clk_enabled;
>> +    int dphy[3];            /* DPHY: CSI2_DPHY3, CSI2_DPHY5, CSI2_DPHY6 */
>> +    int dphy3_algo;         /* Exist 2 algos for calculate CSI2_DPHY3 */
>> +    int bus_type;
>> +    int mipi_enabled;       /* MIPI enabled flag */
>> +    int lane;               /* ccic used lane number; 0 means DVP mode */
>> +    int dma_burst;
>> +    int mclk_min;
>> +    int mclk_src;
>> +    int mclk_div;
>> +    int chip_id;
>>  };
>> --
>> 1.7.0.4
>>
>
>Thanks
>Guennadi
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer
>http://www.open-technology.de/

Thanks
Albert Wang
86-21-61092656
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux