marvell-ccic: add MIPI support for marvell-ccic driver
PATCH 1/7 marvell-ccic: add MIPI support for marvell-ccic driver
From: Libin Yang <[email protected]>
This patch adds the MIPI support for marvell-ccic. Board driver should determine whether using MIPI or not.
--- drivers/media/platform/marvell-ccic/cafe-driver.c | 4 +- drivers/media/platform/marvell-ccic/mcam-core.c | 75 +++++++++++- drivers/media/platform/marvell-ccic/mcam-core.h | 32 +++++- drivers/media/platform/marvell-ccic/mmp-driver.c | 126 ++++++++++++++++++++- include/media/mmp-camera.h | 19 ++++ 5 files changed, 245 insertions(+), 11 deletions(-)
drivers/media/platform/marvell-ccic/cafe-driver.c +3 -1
@@ -400,7 +400,7 @@ static void cafe_ctlr_init(struct mcam_camera *mcam) } -static void cafe_ctlr_power_up (struct mcam_camera *mcam) +static int cafe_ctlr_power_up (struct mcam_camera *mcam) {/* * Part one of the sensor dance: turn the global
@@ -415,6 +415,8 @@ static void cafe_ctlr_power_up(struct mcam_camera *mcam)*/ mcam_reg_write (mcam,REG_GPR ,GPR_C1EN |GPR_C0EN );/* pwr up, reset */ mcam_reg_write (mcam,REG_GPR ,GPR_C1EN |GPR_C0EN |GPR_C0 ); + +return 0 ; }static void cafe_ctlr_power_down (struct mcam_camera *mcam)
drivers/media/platform/marvell-ccic/mcam-core.c +71 -4
134 lines not on this page · raw message
drivers/media/platform/marvell-ccic/mcam-core.h +29 -3
@@ -102,11 +102,23 @@ struct mcam_camera {short int clock_speed;/* Sensor clock speed, default 30 */ short int use_smbus;/* SMBUS or straight I2c? */ enum mcam_buffer_mode buffer_mode; + +int mclk_min; +int mclk_src; +int mclk_div; + +enum v4l2_mbus_type bus_type; +/* MIPI support */ +int *dphy; +bool mipi_enabled; +int lane;/* lane number */ +/* * Callbacks from the core to the platform code. */ -void (*plat_power_up) (struct mcam_camera *cam); +int (*plat_power_up) (struct mcam_camera *cam);void (*plat_power_down) (struct mcam_camera *cam); +void (*calc_dphy) (struct mcam_camera *cam);/* * Everything below here is private to the mcam core and
@@ -220,6 +232,17 @@ int mccic_resume(struct mcam_camera *cam);#define REG_Y0BAR 0x00#define REG_Y1BAR 0x04#define REG_Y2BAR 0x08 + +/* +* register definitions for MIPI support +*/ +#define REG_CSI2_CTRL0 0x100 +#define CSI2_C0_MIPI_EN (0x1 << 0) +#define CSI2_C0_ACT_LANE (n) ((n-1) << 1) +#define REG_CSI2_DPHY3 0x12c +#define REG_CSI2_DPHY5 0x134 +#define REG_CSI2_DPHY6 0x138 +/* ... */ #define REG_IMGPITCH 0x24/* Image pitch register */
@@ -288,13 +311,16 @@ int mccic_resume(struct mcam_camera *cam);#define C0_YUVE_XUVY 0x00020000/* 420: .UVY */ #define C0_YUVE_XVUY 0x00030000/* 420: .VUY */ /* Bayer bits 18,19 if needed */ +#define C0_EOF_VSYNC 0x00400000/* Generate EOF by VSYNC */ +#define C0_VEDGE_CTRL 0x00800000/* Detect falling edge of VSYNC */ #define C0_HPOL_LOW 0x01000000/* HSYNC polarity active low */ #define C0_VPOL_LOW 0x02000000/* VSYNC polarity active low */ #define C0_VCLK_LOW 0x04000000/* VCLK on falling edge */ #define C0_DOWNSCALE 0x08000000/* Enable downscaler */ -#define C0_SIFM_MASK 0xc0000000/* SIF mode bits */ +/* SIFMODE */ #define C0_SIF_HVSYNC 0x00000000/* Use H/VSYNC */ -#define CO_SOF_NOSYNC 0x40000000/* Use inband active signaling */ +#define C0_SOF_NOSYNC 0x40000000/* Use inband active signaling */ +#define C0_SIFM_MASK 0xc0000000/* SIF mode bits */ /* Bits below C1_444ALPHA are not present in Cafe */ #define REG_CTRL1 0x40/* Control 1 */
drivers/media/platform/marvell-ccic/mmp-driver.c +123 -3
187 lines not on this page · raw message
include/media/mmp-camera.h +19 -0
@@ -1,9 +1,28 @@ +#include <media/v4l2-mediabus.h> /* * Information for the Marvell Armada MMP camera */ +enum dphy3_algo { +DPHY3_ALGO_DEFAULT =0 , +DPHY3_ALGO_PXA910 , +DPHY3_ALGO_PXA2128 +}; +struct mmp_camera_platform_data {struct platform_device *i2c_device;int sensor_power_gpio;int sensor_reset_gpio; +enum v4l2_mbus_type bus_type; +int mclk_min; +int mclk_src; +int mclk_div; +/* +* MIPI support +*/ +int dphy[3 ];/* DPHY: CSI2_DPHY3, CSI2_DPHY5, CSI2_DPHY6 */ +enum dphy3_algo dphy3_algo;/* algos for calculate CSI2_DPHY3 */ +int mipi_enabled;/* MIPI enabled flag */ +int lane;/* ccic used lane number; 0 means DVP mode */ +int lane_clk; };
Signature · 1 line
1.7.9.5
Better late than never, I hope...in response to Hans's poke, I'm going to try to do a quick review. I am allegedly in vacation, so this may not be as thorough as we might like...
Quoted 29 lines · On Tue, 4 Jun 2013 13:39:40 +0800 lbyang <[email protected]> wrote:
From: Libin Yang <[email protected]>
This patch adds the MIPI support for marvell-ccic. Board driver should determine whether using MIPI or not.
Signed-off-by Albert Wang <[email protected]>Signed-off-by Libin Yang <[email protected]>--- drivers/media/platform/marvell-ccic/cafe-driver.c | 4 +- drivers/media/platform/marvell-ccic/mcam-core.c | 75 +++++++++++- drivers/media/platform/marvell-ccic/mcam-core.h | 32 +++++- drivers/media/platform/marvell-ccic/mmp-driver.c | 126 ++++++++++++++++++++- include/media/mmp-camera.h | 19 ++++ 5 files changed, 245 insertions(+), 11 deletions(-)drivers/media/platform/marvell-ccic/cafe-driver.c +1
-1@@ -400,7 +400,7 @@ static void cafe_ctlr_init(struct mcam_camera *mcam) } -static void cafe_ctlr_power_up (struct mcam_camera *mcam) +static int cafe_ctlr_power_up (struct mcam_camera *mcam) {/* * Part one of the sensor dance: turn the global
drivers/media/platform/marvell-ccic/cafe-driver.c static void cafe_ctlr_power_up(struct mcam_camera *mcam) · line 420
Quoted 6 lines
drivers/media/platform/marvell-ccic/cafe-driver.c +2
-0@@ -415,6 +415,8 @@ static void cafe_ctlr_power_up(struct mcam_camera *mcam)*/ mcam_reg_write (mcam,REG_GPR ,GPR_C1EN |GPR_C0EN );/* pwr up, reset */ mcam_reg_write (mcam,REG_GPR ,GPR_C1EN |GPR_C0EN |GPR_C0 ); + +return 0 ; }
Curious: why add the return value when it never changes? Do I assume that some future patch adds some complexity here? Not opposed to this, but it seems like the wrong time.
Quoted 45 lines
drivers/media/platform/marvell-ccic/cafe-driver.c +0
-0static void cafe_ctlr_power_down (struct mcam_camera *mcam)drivers/media/platform/marvell-ccic/mcam-core.c +29
-0@@ -19,6 +19,7 @@#include <linux/delay.h> #include <linux/vmalloc.h> #include <linux/io.h> +#include <linux/clk.h> #include <linux/videodev2.h> #include <media/v4l2-device.h> #include <media/v4l2-ioctl.h> @@ -254,6 +255,45 @@ static void mcam_ctlr_stop(struct mcam_camera *cam)mcam_reg_clear_bit (cam,REG_CTRL0 ,C0_ENABLE ); } +static int mcam_config_mipi (struct mcam_camera *mcam,bool enable) +{ +if (enable) { +/* Using MIPI mode and enable MIPI */ +cam_dbg (mcam,"camera: DPHY3=0x%x, DPHY5=0x%x, DPHY6=0x%x\n" , + mcam->dphy[0 ], mcam->dphy[1 ], mcam->dphy[2 ]); +mcam_reg_write (mcam,REG_CSI2_DPHY3 , mcam->dphy[0 ]); +mcam_reg_write (mcam,REG_CSI2_DPHY5 , mcam->dphy[1 ]); +mcam_reg_write (mcam,REG_CSI2_DPHY6 , mcam->dphy[2 ]); + +if (!mcam->mipi_enabled) { +if (mcam->lane >4 || mcam->lane <=0 ) { +cam_warn (mcam,"lane number error\n" ); + mcam->lane =1 ;/* set the default value */ + } +/* +* 0x41 actives 1 lane +* 0x43 actives 2 lanes +* 0x45 actives 3 lanes (never happen) +* 0x47 actives 4 lanes +*/ +mcam_reg_write (mcam,REG_CSI2_CTRL0 , +CSI2_C0_MIPI_EN |CSI2_C0_ACT_LANE (mcam->lane)); +mcam_reg_write (mcam,REG_CLKCTRL , + (mcam->mclk_src <<29 ) | mcam->mclk_div); + + mcam->mipi_enabled = true; + }
drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct mcam_camera *cam) · line 295
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.c +10
-0+ }else { +/* Using Parallel mode or disable MIPI */ +mcam_reg_write (mcam,REG_CSI2_CTRL0 ,0x0 ); +mcam_reg_write (mcam,REG_CSI2_DPHY3 ,0x0 ); +mcam_reg_write (mcam,REG_CSI2_DPHY5 ,0x0 ); +mcam_reg_write (mcam,REG_CSI2_DPHY6 ,0x0 ); + mcam->mipi_enabled = false; + } +return 0 ; +}
I think I said this before...having separate enable/disable functions seems better to me than a multiplexed function with an overall flag. Is there a reason it's done this way?
[...]
Quoted 4 lines
drivers/media/platform/marvell-ccic/mcam-core.c +0
-1/* * Power up and down. */ -static void mcam_ctlr_power_up (struct mcam_camera *cam)
drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop_dma(struct mcam_camera *cam) · line 812
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.c +5
-1+static int mcam_ctlr_power_up (struct mcam_camera *cam) {unsigned long flags; +int ret;spin_lock_irqsave (&cam->dev_lock, flags); - cam->plat_power_up (cam); + ret = cam->plat_power_up (cam); +if (ret) +return ret;
You just returned with the lock held - that's a big mistake.
Quoted 11 lines
drivers/media/platform/marvell-ccic/mcam-core.c +1
-0mcam_reg_clear_bit (cam,REG_CTRL1 ,C1_PWRDWN );spin_unlock_irqrestore (&cam->dev_lock, flags);msleep (5 );/* Just to be sure */ +return 0 ; }static void mcam_ctlr_power_down (struct mcam_camera *cam)@@ -887,6 +938,16 @@ static int mcam_read_setup(struct mcam_camera *cam)spin_lock_irqsave (&cam->dev_lock, flags);clear_bit (CF_DMA_ACTIVE , &cam->flags);mcam_reset_buffers (cam);
drivers/media/platform/marvell-ccic/mcam-core.c static int mcam_read_setup(struct mcam_camera *cam) · line 950
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.c +10
-0+/* +* Update CSI2_DPHY value +*/ +if (cam->calc_dphy) + cam->calc_dphy (cam); +cam_dbg (cam,"camera: DPHY sets: dphy3=0x%x, dphy5=0x%x, dphy6=0x%x\n" , + cam->dphy[0 ], cam->dphy[1 ], cam->dphy[2 ]); + ret =mcam_config_mipi (cam, cam->bus_type ==V4L2_MBUS_CSI2 ); +if (ret <0 ) +return ret;
Once again - you're holding a spinlock here.
[...]
drivers/media/platform/marvell-ccic/mcam-core.c int mccic_resume(struct mcam_camera *cam) · line 1886
Quoted 8 lines
drivers/media/platform/marvell-ccic/mcam-core.c +3
-1@@ -1816,7 +1881,9 @@ int mccic_resume(struct mcam_camera *cam)mutex_lock (&cam->s_mutex);if (cam->users >0 ) { -mcam_ctlr_power_up (cam); + ret =mcam_ctlr_power_up (cam); +if (ret) +return ret;
...and here you're holding a mutex. There's a reason so much kernel code uses the "goto out" pattern; you really have to be careful about returning from the middle of a function.
Quoted 11 lines
drivers/media/platform/marvell-ccic/mcam-core.c +0
-0__mcam_cam_reset (cam); }else {mcam_ctlr_power_down (cam);drivers/media/platform/marvell-ccic/mcam-core.h +0
-0@@ -102,11 +102,23 @@ struct mcam_camera {short int clock_speed;/* Sensor clock speed, default 30 */ short int use_smbus;/* SMBUS or straight I2c? */ enum mcam_buffer_mode buffer_mode;
drivers/media/platform/marvell-ccic/mcam-core.h struct mcam_camera { · line 114
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.h +10
-0+ +int mclk_min; +int mclk_src; +int mclk_div; + +enum v4l2_mbus_type bus_type; +/* MIPI support */ +int *dphy; +bool mipi_enabled; +int lane;/* lane number */
Can we document these new fields a bit better?
Quoted 30 lines
drivers/media/platform/marvell-ccic/mcam-core.h +13
-1/* * Callbacks from the core to the platform code. */ -void (*plat_power_up) (struct mcam_camera *cam); +int (*plat_power_up) (struct mcam_camera *cam);void (*plat_power_down) (struct mcam_camera *cam); +void (*calc_dphy) (struct mcam_camera *cam);/* * Everything below here is private to the mcam core and @@ -220,6 +232,17 @@ int mccic_resume(struct mcam_camera *cam);#define REG_Y0BAR 0x00#define REG_Y1BAR 0x04#define REG_Y2BAR 0x08 + +/* +* register definitions for MIPI support +*/ +#define REG_CSI2_CTRL0 0x100 +#define CSI2_C0_MIPI_EN (0x1 << 0) +#define CSI2_C0_ACT_LANE (n) ((n-1) << 1) +#define REG_CSI2_DPHY3 0x12c +#define REG_CSI2_DPHY5 0x134 +#define REG_CSI2_DPHY6 0x138 +/* ... */ #define REG_IMGPITCH 0x24/* Image pitch register */ @@ -288,13 +311,16 @@ int mccic_resume(struct mcam_camera *cam);#define C0_YUVE_XUVY 0x00020000/* 420: .UVY */
drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 320
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.h +3
-1#define C0_YUVE_XVUY 0x00030000/* 420: .VUY */ /* Bayer bits 18,19 if needed */ +#define C0_EOF_VSYNC 0x00400000/* Generate EOF by VSYNC */ +#define C0_VEDGE_CTRL 0x00800000/* Detect falling edge of VSYNC */ #define C0_HPOL_LOW 0x01000000/* HSYNC polarity active low */ #define C0_VPOL_LOW 0x02000000/* VSYNC polarity active low */ #define C0_VCLK_LOW 0x04000000/* VCLK on falling edge */ #define C0_DOWNSCALE 0x08000000/* Enable downscaler */ -#define C0_SIFM_MASK 0xc0000000/* SIF mode bits */ +/* SIFMODE */
What's this change for?
Quoted 4 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +0
-0
[...]
Quoted 32 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +32
-0+void mmpcam_calc_dphy (struct mcam_camera *mcam) +{ +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; + +/* +* If CSI2_DPHY3 is calculated dynamically, +* pdata->lane_clk should be already set +* either in the board driver statically +* or in the sensor driver dynamically. +*/ +/* +* dphy[0] - CSI2_DPHY3: +* bit 0 ~ bit 7: HS Term Enable. +* defines the time that the DPHY +* wait before enabling the data +* lane termination after detecting +* that the sensor has driven the data +* lanes to the LP00 bridge state. +* The value is calculated by: +* (Max T(D_TERM_EN)/Period(DDR)) - 1 +* bit 8 ~ bit 15: HS_SETTLE +* Time interval during which the HS +* receiver shall ignore any Data Lane +* HS transistions. +* The vaule has been calibrated on +* different boards. It seems to work well. +* +* More detail please refer +* MIPI Alliance Spectification for D-PHY
drivers/media/platform/marvell-ccic/mmp-driver.c static void mmpcam_power_down(struct mcam_camera *mcam) · line 218
Quoted 10 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +10
-0+* document for explanation of HS-SETTLE +* and D-TERM-EN. +*/ +switch (pdata->dphy3_algo) { +case DPHY3_ALGO_PXA910 : +/* +* Calculate CSI2_DPHY3 algo for PXA910 +*/ + pdata->dphy[0 ] = ((1 + pdata->lane_clk *80 /1000 ) &0xff ) <<8 + | (1 + pdata->lane_clk *35 /1000 );
There's enough operators here that some parentheses would really help to make the code more readable. Lots of places in this file.
[...]
Quoted 5 lines
static
irqreturn_tmmpcam_irq(int irq, void *data){drivers/media/platform/marvell-ccic/mmp-driver.c +0
-0@@ -174,17 +280,30 @@ static int mmpcam_probe(struct platform_device *pdev)struct mmp_camera_platform_data *pdata;int ret;
drivers/media/platform/marvell-ccic/mmp-driver.c static int mmpcam_probe(struct platform_device *pdev) · line 291
Quoted 10 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +5
-0+ pdata = pdev->dev.platform_data; +if (!pdata) +return -ENODEV ; + cam =kzalloc (sizeof (*cam),GFP_KERNEL );if (cam ==NULL )return -ENOMEM ; cam->pdev = pdev; + cam->mipi_clk =ERR_PTR (-EINVAL );
The use of ERR_PTR here (and in related places) seems a bit weird; is there a reason you can't just use NULL?
In summary, I'm not really familiar with the MIPI interface, and I don't have any hardware with it, so I'll have to take your word that the code works. I've pointed out a bunch of nits that are worth fixing. The locking mistakes are fatal, though, and need attention. They should be quick to fix, though; this code should be ready to merge in short order.
Thanks,
jon
Hi Jonathan,
Sorry for delay reply. Please see the below comments.
Regards,
Libin
Quoted 52 lines
-----Original Message-----
From: Jonathan Corbet [mailto:[email protected]]
Sent: Saturday, June 22, 2013 1:00 AM
To: Libin Yang
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 1/7] marvell-ccic: add MIPI support for marvell-ccic driverBetter late than never, I hope...in response to Hans's poke, I'm going to try to do a quick review. I am allegedly in vacation, so this may not be as thorough as we might like...
Quoted 10 lines · On Tue, 4 Jun 2013 13:39:40 +0800 lbyang <[email protected]> wrote:
From: Libin Yang <[email protected]>
This patch adds the MIPI support for marvell-ccic. Board driver should determine whether using MIPI or not.
Signed-off-by Albert Wang <[email protected]>Signed-off-by Libin Yang <[email protected]>--- drivers/media/platform/marvell-ccic/cafe-driver.c | 4 +- drivers/media/platform/marvell-ccic/mcam-core.c | 75+++++++++++-
Quoted 2 lines
drivers/media/platform/marvell-ccic/mcam-core.h | 32 +++++- drivers/media/platform/marvell-ccic/mmp-driver.c | 126++++++++++++++++++++-
Quoted 9 lines
include/media/mmp-camera.h | 19 ++++ 5 files changed, 245 insertions(+), 11 deletions(-)drivers/media/platform/marvell-ccic/cafe-driver.c +0
-0drivers/media/platform/marvell-ccic/cafe-driver.c static void cafe_ctlr_init(struct mcam_camera · line 406
Quoted 8 lines
drivers/media/platform/marvell-ccic/cafe-driver.c +1
-1@@ -400,7 +400,7 @@ static void cafe_ctlr_init(struct mcam_camera *mcam) } -static void cafe_ctlr_power_up (struct mcam_camera *mcam) +static int cafe_ctlr_power_up (struct mcam_camera *mcam) {/* * Part one of the sensor dance: turn the global @@ -415,6 +415,8 @@
drivers/media/platform/marvell-ccic/cafe-driver.c
Quoted 3 lines
static void
cafe_ctlr_power_up(struct mcam_camera *mcam)drivers/media/platform/marvell-ccic/cafe-driver.c +0
-0*/ mcam_reg_write (mcam,REG_GPR ,GPR_C1EN |GPR_C0EN );/* pwr up, reset */
Quoted 8 lines
drivers/media/platform/marvell-ccic/cafe-driver.c static void cafe_ctlr_power_up(struct mcam_camera *mcam) · line 420
Quoted 4 lines
drivers/media/platform/marvell-ccic/cafe-driver.c +2
-0mcam_reg_write (mcam,REG_GPR ,GPR_C1EN |GPR_C0EN |GPR_C0 ); + +return 0 ; }Curious: why add the return value when it never changes? Do I assume that some future patch adds some complexity here? Not opposed to this, but it seems like the wrong time.
[Libin] This function will be set to mcam->plat_power_up eventually. The callback plat_power_up is changed to return int type because of mmp-driver.c
Quoted 57 lines
Quoted 2 lines
static void
cafe_ctlr_power_down(struct mcam_camera *mcam)(quoted diff) +0
-0drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct
Quoted 14 lines
a/drivers/media/platform/marvell-ccic/mcam-core.cb/drivers/media/platform/marvell-ccic/mcam-core.c
index 64ab91e..bb3de1f 100644drivers/media/platform/marvell-ccic/mcam-core.c +1
-0@@ -19,6 +19,7 @@#include <linux/delay.h> #include <linux/vmalloc.h> #include <linux/io.h> +#include <linux/clk.h> #include <linux/videodev2.h> #include <media/v4l2-device.h> #include <media/v4l2-ioctl.h> @@ -254,6 +255,45 @@ static void mcam_ctlr_stop(struct
mcam_camera*cam)drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct mcam_camera *cam)
Quoted 6 lines
drivers/media/platform/marvell-ccic/mcam-core.c +4
-0mcam_reg_clear_bit (cam,REG_CTRL0 ,C0_ENABLE ); } +static int mcam_config_mipi (struct mcam_camera *mcam,bool enable) { +if (enable) { +/* Using MIPI mode and enable MIPI */ + cam_dbg(mcam, "camera: DPHY3=0x%x, DPHY5=0x%x,DPHY6=0x%x\n",
drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct mcam_camera *cam)
Quoted 9 lines
drivers/media/platform/marvell-ccic/mcam-core.c +9
-0+ mcam->dphy[0], mcam->dphy[1], mcam->dphy[2]); +mcam_reg_write (mcam,REG_CSI2_DPHY3 , mcam->dphy[0 ]); +mcam_reg_write (mcam,REG_CSI2_DPHY5 , mcam->dphy[1 ]); +mcam_reg_write (mcam,REG_CSI2_DPHY6 , mcam->dphy[2 ]); + +if (!mcam->mipi_enabled) { +if (mcam->lane >4 || mcam->lane <=0 ) { +cam_warn (mcam,"lane number error\n" ); + mcam->lane =1 ;/* set the default value */
drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct mcam_camera *cam)
Quoted 9 lines
drivers/media/platform/marvell-ccic/mcam-core.c +9
-0+ } +/* +* 0x41 actives 1 lane +* 0x43 actives 2 lanes +* 0x45 actives 3 lanes (never happen) +* 0x47 actives 4 lanes +*/ + mcam_reg_write(mcam, REG_CSI2_CTRL0, + CSI2_C0_MIPI_EN |
CSI2_C0_ACT_LANE(mcam->lane));drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct · line 280
Quoted 2 lines
drivers/media/platform/marvell-ccic/mcam-core.c +2
-0+ mcam_reg_write(mcam, REG_CLKCTRL, + (mcam->mclk_src << 29) |
mcam->mclk_div);Quoted 9 lines
drivers/media/platform/marvell-ccic/mcam-core.c +9
-0+ + mcam->mipi_enabled = true; + } + }else { +/* Using Parallel mode or disable MIPI */ +mcam_reg_write (mcam,REG_CSI2_CTRL0 ,0x0 ); +mcam_reg_write (mcam,REG_CSI2_DPHY3 ,0x0 ); +mcam_reg_write (mcam,REG_CSI2_DPHY5 ,0x0 ); +mcam_reg_write (mcam,REG_CSI2_DPHY6 ,0x0 );
Quoted 8 lines
drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop(struct mcam_camera *cam) · line 295
Quoted 4 lines
drivers/media/platform/marvell-ccic/mcam-core.c +4
-0+ mcam->mipi_enabled = false; + } +return 0 ; +}I think I said this before...having separate enable/disable functions seems better to me than a multiplexed function with an overall flag. Is there a reason it's done this way?
[LIbin] OK, I will split it into 2 functions.
Quoted 11 lines
[...]
Quoted 9 lines
/*
* Power up and down.
*/drivers/media/platform/marvell-ccic/mcam-core.c +2
-1-static void mcam_ctlr_power_up (struct mcam_camera *cam) +static int mcam_ctlr_power_up (struct mcam_camera *cam) {unsigned long flags; +int ret;
Quoted 7 lines
drivers/media/platform/marvell-ccic/mcam-core.c static void mcam_ctlr_stop_dma(struct mcam_camera *cam) · line 812
Quoted 5 lines
drivers/media/platform/marvell-ccic/mcam-core.c +3
-1spin_lock_irqsave (&cam->dev_lock, flags); - cam->plat_power_up (cam); + ret = cam->plat_power_up (cam); +if (ret) +return ret;You just returned with the lock held - that's a big mistake.
[LIbin] Yes. I will correct it. Thanks for point it out.
Quoted 19 lines
Quoted 8 lines
drivers/media/platform/marvell-ccic/mcam-core.c +1
-0mcam_reg_clear_bit (cam,REG_CTRL1 ,C1_PWRDWN );spin_unlock_irqrestore (&cam->dev_lock, flags);msleep (5 );/* Just to be sure */ +return 0 ; } static void mcam_ctlr_power_down(struct mcam_camera *cam) @@ -887,6drivers/media/platform/marvell-ccic/mcam-core.c
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.c +7
-0+938,16 @@ static int mcam_read_setup(struct mcam_camera *cam) spin_lock_irqsave(&cam->dev_lock, flags);clear_bit (CF_DMA_ACTIVE , &cam->flags);mcam_reset_buffers (cam); +/* +* Update CSI2_DPHY value +*/ +if (cam->calc_dphy) + cam->calc_dphy (cam); + cam_dbg(cam, "camera: DPHY sets: dphy3=0x%x,dphy5=0x%x, dphy6=0x%x\n",
Quoted 6 lines
drivers/media/platform/marvell-ccic/mcam-core.c static int mcam_read_setup(struct mcam_camera *cam) · line 950
Quoted 4 lines
drivers/media/platform/marvell-ccic/mcam-core.c +4
-0+ cam->dphy[0], cam->dphy[1], cam->dphy[2]); + ret =mcam_config_mipi (cam, cam->bus_type ==V4L2_MBUS_CSI2 ); +if (ret <0 ) +return ret;Once again - you're holding a spinlock here.
[Libin] Yes.
Quoted 5 lines
[...]
Quoted 2 lines
drivers/media/platform/marvell-ccic/mcam-core.c +0
-0@@ -1816,7 +1881,9 @@ int mccic_resume(struct mcam_camera *cam)
Quoted 10 lines
drivers/media/platform/marvell-ccic/mcam-core.c int mccic_resume(struct mcam_camera *cam) · line 1886
Quoted 6 lines
drivers/media/platform/marvell-ccic/mcam-core.c +3
-1mutex_lock (&cam->s_mutex);if (cam->users >0 ) { -mcam_ctlr_power_up (cam); + ret =mcam_ctlr_power_up (cam); +if (ret) +return ret;...and here you're holding a mutex. There's a reason so much kernel code uses the "goto out" pattern; you really have to be careful about returning from the middle of a function.
[Libin] Yes. I will correct it.
Quoted 17 lines
Quoted 17 lines
drivers/media/platform/marvell-ccic/mcam-core.c +0
-0__mcam_cam_reset (cam); }else {mcam_ctlr_power_down (cam);drivers/media/platform/marvell-ccic/mcam-core.h +4
-0@@ -102,11 +102,23 @@ struct mcam_camera {short int clock_speed;/* Sensor clock speed, default 30 */ short int use_smbus;/* SMBUS or straight I2c? */ enum mcam_buffer_mode buffer_mode; + +int mclk_min; +int mclk_src; +int mclk_div;
Quoted 8 lines
drivers/media/platform/marvell-ccic/mcam-core.h struct mcam_camera { · line 114
Quoted 6 lines
drivers/media/platform/marvell-ccic/mcam-core.h +6
-0+ +enum v4l2_mbus_type bus_type; +/* MIPI support */ +int *dphy; +bool mipi_enabled; +int lane;/* lane number */ Can we document these new fields a bit better?
[Libin] OK.
Quoted 42 lines
drivers/media/platform/marvell-ccic/mcam-core.h struct mcam_camera {
Quoted 11 lines
drivers/media/platform/marvell-ccic/mcam-core.h +2
-1/* * Callbacks from the core to the platform code. */ -void (*plat_power_up) (struct mcam_camera *cam); +int (*plat_power_up) (struct mcam_camera *cam);void (*plat_power_down) (struct mcam_camera *cam); +void (*calc_dphy) (struct mcam_camera *cam);/* * Everything below here is private to the mcam core and @@ -220,6
Quoted 18 lines
drivers/media/platform/marvell-ccic/mcam-core.h +1
-0+232,17 @@ int mccic_resume(struct mcam_camera *cam);#define
REG_Y0BAR0x00
#defineREG_Y1BAR0x04
#defineREG_Y2BAR0x08drivers/media/platform/marvell-ccic/mcam-core.h +10
-0+ +/* +* register definitions for MIPI support */ +#define REG_CSI2_CTRL0 0x100 +#define CSI2_C0_MIPI_EN (0x1 << 0) +#define CSI2_C0_ACT_LANE (n) ((n-1) << 1) +#define REG_CSI2_DPHY3 0x12c +#define REG_CSI2_DPHY5 0x134 +#define REG_CSI2_DPHY6 0x138 +/* ... */ #define REG_IMGPITCH 0x24/* Image pitch register */ drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 311
Quoted 1 line
drivers/media/platform/marvell-ccic/mcam-core.h +0
-0@@ -288,13 +311,16 @@ int mccic_resume(struct mcam_camera *cam);#define C0_YUVE_XUVY 0x00020000/* 420: .UVY */drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 312
Quoted 1 line
drivers/media/platform/marvell-ccic/mcam-core.h +0
-0#define C0_YUVE_XVUY 0x00030000/* 420: .VUY */drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 314
Quoted 2 lines
drivers/media/platform/marvell-ccic/mcam-core.h +1
-0/* Bayer bits 18,19 if needed */ +#define C0_EOF_VSYNC 0x00400000/* Generate EOF by VSYNC */
drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 315
Quoted 1 line
drivers/media/platform/marvell-ccic/mcam-core.h +1
-0+#define C0_VEDGE_CTRL 0x00800000/* Detect falling edge of VSYNC */
drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 316
Quoted 1 line
drivers/media/platform/marvell-ccic/mcam-core.h +0
-0#define C0_HPOL_LOW 0x01000000/* HSYNC polarity active low */
drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 317
Quoted 1 line
drivers/media/platform/marvell-ccic/mcam-core.h +0
-0#define C0_VPOL_LOW 0x02000000/* VSYNC
Quoted 9 lines
polarity active low */
drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 318
Quoted 1 line
drivers/media/platform/marvell-ccic/mcam-core.h +0
-0#define C0_VCLK_LOW 0x04000000/* VCLK on falling edge */
drivers/media/platform/marvell-ccic/mcam-core.h int mccic_resume(struct mcam_camera *cam); · line 320
Quoted 3 lines
drivers/media/platform/marvell-ccic/mcam-core.h +1
-1#define C0_DOWNSCALE 0x08000000/* Enable downscaler */ -#define C0_SIFM_MASK 0xc0000000/* SIF mode bits */ +/* SIFMODE */ What's this change for?
[LIbin] Mostly, the code is to fix the typo: CO_SOF_NOSYNC -> C0_SOF_NOSYNC. And it also re-sorts the code by the value to improve the readability.
Quoted 48 lines
Quoted 5 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +0
-0[...]
drivers/media/platform/marvell-ccic/mmp-driver.c
Quoted 3 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +3
-0+void mmpcam_calc_dphy (struct mcam_camera *mcam) { +struct mmp_camera *cam =mcam_to_cam (mcam); + struct mmp_camera_platform_data *pdata =
cam->pdev->dev.platform_data;Quoted 27 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +27
-0+struct device *dev = &cam->pdev->dev; +unsigned long tx_clk_esc; + +/* +* If CSI2_DPHY3 is calculated dynamically, +* pdata->lane_clk should be already set +* either in the board driver statically +* or in the sensor driver dynamically. +*/ +/* +* dphy[0] - CSI2_DPHY3: +* bit 0 ~ bit 7: HS Term Enable. +* defines the time that the DPHY +* wait before enabling the data +* lane termination after detecting +* that the sensor has driven the data +* lanes to the LP00 bridge state. +* The value is calculated by: +* (Max T(D_TERM_EN)/Period(DDR)) - 1 +* bit 8 ~ bit 15: HS_SETTLE +* Time interval during which the HS +* receiver shall ignore any Data Lane +* HS transistions. +* The vaule has been calibrated on +* different boards. It seems to work well. +* +* More detail please refer drivers/media/platform/marvell-ccic/mmp-driver.c static void mmpcam_power_down(struct mcam_camera *mcam)
Quoted 10 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +10
-0+* MIPI Alliance Spectification for D-PHY +* document for explanation of HS-SETTLE +* and D-TERM-EN. +*/ + switch (pdata->dphy3_algo) { + case DPHY3_ALGO_PXA910: + /* + * Calculate CSI2_DPHY3 algo for PXA910 + */ + pdata->dphy[0] = ((1 + pdata->lane_clk * 80 /1000) &
0xff) << 8
Quoted 5 lines
drivers/media/platform/marvell-ccic/mmp-driver.c
Quoted 1 line
drivers/media/platform/marvell-ccic/mmp-driver.c +1
-0+ | (1 + pdata->lane_clk * 35 / 1000);There's enough operators here that some parentheses would really help to make the code more readable. Lots of places in this file.
[Libin] OK. I will add some parentheses to help it readable.
Quoted 7 lines
[...]
Quoted 5 lines
static
irqreturn_tmmpcam_irq(int irq, void *data){ @@ -174,17
+280,30 @@ static intmmpcam_probe(struct platform_device *pdev)drivers/media/platform/marvell-ccic/mmp-driver.c +0
-0struct mmp_camera_platform_data *pdata;int ret;
Quoted 12 lines
drivers/media/platform/marvell-ccic/mmp-driver.c static int mmpcam_probe(struct platform_device *pdev) · line 291
Quoted 9 lines
drivers/media/platform/marvell-ccic/mmp-driver.c +5
-0+ pdata = pdev->dev.platform_data; +if (!pdata) +return -ENODEV ; + cam =kzalloc (sizeof (*cam),GFP_KERNEL );if (cam ==NULL )return -ENOMEM ; cam->pdev = pdev; + cam->mipi_clk =ERR_PTR (-EINVAL );The use of
ERR_PTRhere (and in related places) seems a bit weird; is there a reason you can't just use NULL?
[Libin] OK, I will use NULL in next version.
Quoted 12 lines
In summary, I'm not really familiar with the MIPI interface, and I don't have any hardware with it, so I'll have to take your word that the code works. I've pointed out a bunch of nits that are worth fixing. The locking mistakes are fatal, though, and need attention. They should be quick to fix, though; this code should be ready to merge in short order.
Thanks,
jon