RE: [PATCH V3 12/15] [media] marvell-ccic: add soc_camera support in mmp driver

2012-12-16 Thread Albert Wang 
Hi, Jonathan



>-Original Message-
>From: Jonathan Corbet [mailto:cor...@lwn.net]
>Sent: Monday, 17 December, 2012 00:46
>To: Albert Wang
>Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH V3 12/15] [media] marvell-ccic: add soc_camera support in 
>mmp
>driver
>
>On Sat, 15 Dec 2012 17:58:01 +0800
>Albert Wang  wrote:
>
>> This patch adds the soc_camera support in the platform driver: mmp-driver.c.
>> Specified board driver also should be modified to support soc_camera by 
>> passing
>> some platform datas to platform driver.
>>
>> Currently the soc_camera mode in mmp driver only supports B_DMA_contig mode.
>
>You do intend to add the other modes (or SG, at least) in the future?
>
[Albert Wang] Yes, if need we can add the other modes in the future.

>> --- 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
>> +   bool
>
>If Linus sees this you'll get an unpleasant reminder that vowels are not
>actually in short supply; I'd suggest spelling out "MARVELL".
>
[Albert Wang] Sorry, we will change it.

>> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
>b/drivers/media/platform/marvell-ccic/mmp-driver.c
>> index 40c243e..cd850f4 100755
>> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
>> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
>> @@ -28,6 +28,10 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>>
>>  #include "mcam-core.h"
>>
>> @@ -40,6 +44,8 @@ struct mmp_camera {
>>  struct platform_device *pdev;
>>  struct mcam_camera mcam;
>>  struct list_head devlist;
>> +/* will change here */
>> +struct clk *clk[3]; /* CCIC_GATE, CCIC_RST, CCIC_DBG clocks */
>
>What does that comment mean?
>
[Albert Wang] It means there are 3 clk setting, gate_clk, rst_clk, dbg_clk.
Forgive me, the name of CCIC_DBG register is not good, but it's our actual 
register name. :(

>>  int irq;
>>  };
>>
>> @@ -144,15 +150,17 @@ static void mmpcam_power_up(struct mcam_camera
>*mcam)
>>   * Provide power to the sensor.
>>   */
>>  mcam_reg_write(mcam, REG_CLKCTRL, 0x6002);
>> -pdata = cam->pdev->dev.platform_data;
>> -gpio_set_value(pdata->sensor_power_gpio, 1);
>> -mdelay(5);
>> +if (mcam->chip_id == V4L2_IDENT_ARMADA610) {
>
>I'm seeing a lot of these tests being added to the code.  I can imagine
>more in the future as new chipsets are supported in the driver.  Maybe it's
>time to add a structure to hide chipset-specific low-level operations?  It
>would make the code a lot cleaner.
>
[Albert Wang] OK, we will do it.

>Actually, things like mmpcam_power_up() were meant to be exactly that.  Can
>we just define a different version of this function for different chipsets?
>
[Albert Wang] Sure.

>> +pdata = cam->pdev->dev.platform_data;
>> +gpio_set_value(pdata->sensor_power_gpio, 1);
>> +mdelay(5);
>> +/* reset is active low */
>> +gpio_set_value(pdata->sensor_reset_gpio, 0);
>> +mdelay(5);
>> +gpio_set_value(pdata->sensor_reset_gpio, 1);
>> +mdelay(5);
>> +}
>>  mcam_reg_clear_bit(mcam, REG_CTRL1, 0x1000);
>> -gpio_set_value(pdata->sensor_reset_gpio, 0); /* reset is active low */
>> -mdelay(5);
>> -gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
>> -mdelay(5);
>> -
>>  mcam_clk_set(mcam, 1);
>>  }
>>
>> @@ -165,13 +173,14 @@ static void mmpcam_power_down(struct mcam_camera
>*mcam)
>>   */
>>  iowrite32(0, cam->power_regs + REG_CCIC_DCGCR);
>>  iowrite32(0, cam->power_regs + REG_CCIC_CRCR);
>> -/*
>> - * Shut down the sensor.
>> - */
>> -pdata = cam->pdev->dev.platform_data;
>> -gpio_set_value(pdata->sensor_power_gpio, 0);
>> -gpio_set_value(pdata->sensor_reset_gpio, 0);
>> -
>> +if (mcam->chip_id == V4L2_IDENT_ARMADA610) {
>
>Same comment applies here.
>
>> +/*
>> + * Shut down the sensor.
>> + */
>> +pdata = cam->pdev->dev.platform_data;
>> +gpio_set_value(pdata->sensor_power_gpio, 0);
>> +gpio_set_value(pdata->sensor_reset_gpio, 0);
>> +}
>>  mcam_clk_set(mcam, 0);
>
>jon


Thanks
Albert Wang
86-21-61092656
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 12/15] [media] marvell-ccic: add soc_camera support in mmp driver

2012-12-16 Thread Jonathan Corbet 
On Sat, 15 Dec 2012 17:58:01 +0800
Albert Wang  wrote:

> This patch adds the soc_camera support in the platform driver: mmp-driver.c.
> Specified board driver also should be modified to support soc_camera by 
> passing
> some platform datas to platform driver.
> 
> Currently the soc_camera mode in mmp driver only supports B_DMA_contig mode.

You do intend to add the other modes (or SG, at least) in the future?

> --- 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
> +   bool

If Linus sees this you'll get an unpleasant reminder that vowels are not
actually in short supply; I'd suggest spelling out "MARVELL".

> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c 
> b/drivers/media/platform/marvell-ccic/mmp-driver.c
> index 40c243e..cd850f4 100755
> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
> @@ -28,6 +28,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  #include "mcam-core.h"
>  
> @@ -40,6 +44,8 @@ struct mmp_camera {
>   struct platform_device *pdev;
>   struct mcam_camera mcam;
>   struct list_head devlist;
> + /* will change here */
> + struct clk *clk[3]; /* CCIC_GATE, CCIC_RST, CCIC_DBG clocks */

What does that comment mean?

>   int irq;
>  };
>  
> @@ -144,15 +150,17 @@ static void mmpcam_power_up(struct mcam_camera *mcam)
>   * Provide power to the sensor.
>   */
>   mcam_reg_write(mcam, REG_CLKCTRL, 0x6002);
> - pdata = cam->pdev->dev.platform_data;
> - gpio_set_value(pdata->sensor_power_gpio, 1);
> - mdelay(5);
> + if (mcam->chip_id == V4L2_IDENT_ARMADA610) {

I'm seeing a lot of these tests being added to the code.  I can imagine
more in the future as new chipsets are supported in the driver.  Maybe it's
time to add a structure to hide chipset-specific low-level operations?  It
would make the code a lot cleaner.

Actually, things like mmpcam_power_up() were meant to be exactly that.  Can
we just define a different version of this function for different chipsets?

> + pdata = cam->pdev->dev.platform_data;
> + gpio_set_value(pdata->sensor_power_gpio, 1);
> + mdelay(5);
> + /* reset is active low */
> + gpio_set_value(pdata->sensor_reset_gpio, 0);
> + mdelay(5);
> + gpio_set_value(pdata->sensor_reset_gpio, 1);
> + mdelay(5);
> + }
>   mcam_reg_clear_bit(mcam, REG_CTRL1, 0x1000);
> - gpio_set_value(pdata->sensor_reset_gpio, 0); /* reset is active low */
> - mdelay(5);
> - gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
> - mdelay(5);
> -
>   mcam_clk_set(mcam, 1);
>  }
>  
> @@ -165,13 +173,14 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
>   */
>   iowrite32(0, cam->power_regs + REG_CCIC_DCGCR);
>   iowrite32(0, cam->power_regs + REG_CCIC_CRCR);
> -/*
> - * Shut down the sensor.
> - */
> - pdata = cam->pdev->dev.platform_data;
> - gpio_set_value(pdata->sensor_power_gpio, 0);
> - gpio_set_value(pdata->sensor_reset_gpio, 0);
> -
> + if (mcam->chip_id == V4L2_IDENT_ARMADA610) {

Same comment applies here.

> + /*
> +  * Shut down the sensor.
> +  */
> + pdata = cam->pdev->dev.platform_data;
> + gpio_set_value(pdata->sensor_power_gpio, 0);
> + gpio_set_value(pdata->sensor_reset_gpio, 0);
> + }
>   mcam_clk_set(mcam, 0);

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3 12/15] [media] marvell-ccic: add soc_camera support in mmp driver

2012-12-15 Thread Albert Wang 
This patch adds the soc_camera support in the platform driver: mmp-driver.c.
Specified board driver also should be modified to support soc_camera by passing
some platform datas to platform driver.

Currently the soc_camera mode in mmp driver only supports B_DMA_contig mode.

Signed-off-by: Libin Yang 
Signed-off-by: Albert Wang 
---
 drivers/media/platform/Makefile  |4 +-
 drivers/media/platform/marvell-ccic/Kconfig  |   22 
 drivers/media/platform/marvell-ccic/mmp-driver.c |  147 ++
 include/media/mmp-camera.h   |2 +
 4 files changed, 120 insertions(+), 55 deletions(-)

diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index baaa550..95c1ce5 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -11,8 +11,6 @@ obj-$(CONFIG_VIDEO_TIMBERDALE)+= timblogiw.o
 obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
 
 obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
-obj-$(CONFIG_VIDEO_CAFE_CCIC) += marvell-ccic/
-obj-$(CONFIG_VIDEO_MMP_CAMERA) += marvell-ccic/
 
 obj-$(CONFIG_VIDEO_OMAP2)  += omap2cam.o
 obj-$(CONFIG_VIDEO_OMAP3)  += omap3isp/
@@ -43,6 +41,8 @@ obj-$(CONFIG_VIDEO_SH_VOU)+= sh_vou.o
 
 obj-$(CONFIG_SOC_CAMERA)   += soc_camera/
 
+obj-$(CONFIG_VIDEO_MARVELL_CCIC)   += marvell-ccic/
+
 obj-y  += davinci/
 
 obj-$(CONFIG_ARCH_OMAP)+= omap/
diff --git a/drivers/media/platform/marvell-ccic/Kconfig 
b/drivers/media/platform/marvell-ccic/Kconfig
index bf739e3..910c068 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
+   bool
+
 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/mmp-driver.c 
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 40c243e..cd850f4 100755
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -28,6 +28,10 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 
 #include "mcam-core.h"
 
@@ -40,6 +44,8 @@ struct mmp_camera {
struct platform_device *pdev;
struct mcam_camera mcam;
struct list_head devlist;
+   /* will change here */
+   struct clk *clk[3]; /* CCIC_GATE, CCIC_RST, CCIC_DBG clocks */
int irq;
 };
 
@@ -144,15 +150,17 @@ static void mmpcam_power_up(struct mcam_camera *mcam)
  * Provide power to the sensor.
  */
mcam_reg_write(mcam, REG_CLKCTRL, 0x6002);
-   pdata = cam->pdev->dev.platform_data;
-   gpio_set_value(pdata->sensor_power_gpio, 1);
-   mdelay(5);
+   if (mcam->chip_id == V4L2_IDENT_ARMADA610) {
+   pdata = cam->pdev->dev.platform_data;
+   gpio_set_value(pdata->sensor_power_gpio, 1);
+   mdelay(5);
+   /* reset is active low */
+   gpio_set_value(pdata->sensor_reset_gpio, 0);
+   mdelay(5);
+   gpio_set_value(pdata->sensor_reset_gpio, 1);
+   mdelay(5);
+   }
mcam_reg_clear_bit(mcam, REG_CTRL1, 0x1000);
-   gpio_set_value(pdata->sensor_reset_gpio, 0); /* reset is active low */
-   mdelay(5);
-   gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
-   mdelay(5);
-
mcam_clk_set(mcam, 1);