| Message ID | 1370324380.26072.19.camel@younglee-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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... On Tue, 4 Jun 2013 13:39:40 +0800 lbyang <lbyang@marvell.com> wrote: > From: Libin Yang <lbyang@marvell.com> > > This patch adds the MIPI support for marvell-ccic. > Board driver should determine whether using MIPI or not. > > Signed-off-by: Albert Wang <twang13@marvell.com> > Signed-off-by: Libin Yang <lbyang@marvell.com> > --- > 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(-) > > diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c b/drivers/media/platform/marvell-ccic/cafe-driver.c > index d030f9b..68e82fb 100644 > --- a/drivers/media/platform/marvell-ccic/cafe-driver.c > +++ b/drivers/media/platform/marvell-ccic/cafe-driver.c > @@ -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; > } 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. > static void cafe_ctlr_power_down(struct mcam_camera *mcam) > diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c > index 64ab91e..bb3de1f 100644 > --- a/drivers/media/platform/marvell-ccic/mcam-core.c > +++ b/drivers/media/platform/marvell-ccic/mcam-core.c > @@ -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; > + } > + } 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? [...] > /* > * Power up and down. > */ > -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; > > 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. > mcam_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); > + /* > + * 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. [...] > @@ -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. > __mcam_cam_reset(cam); > } else { > mcam_ctlr_power_down(cam); > diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h > index 01dec9e..be271b3 100644 > --- a/drivers/media/platform/marvell-ccic/mcam-core.h > +++ b/drivers/media/platform/marvell-ccic/mcam-core.h > @@ -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 */ Can we document these new fields a bit better? > /* > * 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 */ What's this change for? > diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c > index c4c17fe..3dad182 100644 > --- a/drivers/media/platform/marvell-ccic/mmp-driver.c > +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c [...] > +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 > + * 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. [...] > static irqreturn_t mmpcam_irq(int irq, void *data) > { > @@ -174,17 +280,30 @@ static int mmpcam_probe(struct platform_device *pdev) > struct mmp_camera_platform_data *pdata; > int ret; > > + 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 -- 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 Jonathan, Sorry for delay reply. Please see the below comments. Regards, Libin >-----Original Message----- >From: Jonathan Corbet [mailto:corbet@lwn.net] >Sent: Saturday, June 22, 2013 1:00 AM >To: Libin Yang >Cc: g.liakhovetski@gmx.de; mchehab@redhat.com; >linux-media@vger.kernel.org; albert.v.wang@gmail.com >Subject: Re: [PATCH 1/7] marvell-ccic: add MIPI support for >marvell-ccic driver > >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... > >On Tue, 4 Jun 2013 13:39:40 +0800 >lbyang <lbyang@marvell.com> wrote: > >> From: Libin Yang <lbyang@marvell.com> >> >> This patch adds the MIPI support for marvell-ccic. >> Board driver should determine whether using MIPI or not. >> >> Signed-off-by: Albert Wang <twang13@marvell.com> >> Signed-off-by: Libin Yang <lbyang@marvell.com> >> --- >> 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(-) >> >> diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c >> b/drivers/media/platform/marvell-ccic/cafe-driver.c >> index d030f9b..68e82fb 100644 >> --- a/drivers/media/platform/marvell-ccic/cafe-driver.c >> +++ b/drivers/media/platform/marvell-ccic/cafe-driver.c >> @@ -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; >> } > >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 > >> static void cafe_ctlr_power_down(struct mcam_camera *mcam) >diff --git >> a/drivers/media/platform/marvell-ccic/mcam-core.c >> b/drivers/media/platform/marvell-ccic/mcam-core.c >> index 64ab91e..bb3de1f 100644 >> --- a/drivers/media/platform/marvell-ccic/mcam-core.c >> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c >> @@ -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; >> + } >> + } 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? [LIbin] OK, I will split it into 2 functions. > >[...] >> /* >> * Power up and down. >> */ >> -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; >> >> 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. [LIbin] Yes. I will correct it. Thanks for point it out. > >> mcam_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); >> + /* >> + * 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. [Libin] Yes. > >[...] > >> @@ -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. [Libin] Yes. I will correct it. > >> __mcam_cam_reset(cam); >> } else { >> mcam_ctlr_power_down(cam); >> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h >> b/drivers/media/platform/marvell-ccic/mcam-core.h >> index 01dec9e..be271b3 100644 >> --- a/drivers/media/platform/marvell-ccic/mcam-core.h >> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h >> @@ -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 */ > >Can we document these new fields a bit better? [Libin] OK. > >> /* >> * 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 */ > >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. >> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c >> b/drivers/media/platform/marvell-ccic/mmp-driver.c >> index c4c17fe..3dad182 100644 >> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c >> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c >[...] >> +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 >> + * 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. [Libin] OK. I will add some parentheses to help it readable. > >[...] >> static irqreturn_t mmpcam_irq(int irq, void *data) { @@ -174,17 >> +280,30 @@ static int mmpcam_probe(struct platform_device *pdev) >> struct mmp_camera_platform_data *pdata; >> int ret; >> >> + 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? [Libin] OK, I will use NULL in next version. > >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 >-- 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
diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c b/drivers/media/platform/marvell-ccic/cafe-driver.c index d030f9b..68e82fb 100644 --- a/drivers/media/platform/marvell-ccic/cafe-driver.c +++ b/drivers/media/platform/marvell-ccic/cafe-driver.c @@ -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) diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c index 64ab91e..bb3de1f 100644 --- a/drivers/media/platform/marvell-ccic/mcam-core.c +++ b/drivers/media/platform/marvell-ccic/mcam-core.c @@ -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; + } + } 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; +} + /* ------------------------------------------------------------------- */ #ifdef MCAM_MODE_VMALLOC @@ -657,6 +697,13 @@ static void mcam_ctlr_image(struct mcam_camera *cam) */ mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC, C0_SIFM_MASK); + + /* + * This field controls the generation of EOF(DVP only) + */ + if (cam->bus_type != V4L2_MBUS_CSI2) + mcam_reg_set_bit(cam, REG_CTRL0, + C0_EOF_VSYNC | C0_VEDGE_CTRL); } @@ -754,15 +801,19 @@ static void mcam_ctlr_stop_dma(struct mcam_camera *cam) /* * Power up and down. */ -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; spin_lock_irqsave(&cam->dev_lock, flags); - cam->plat_power_up(cam); + ret = cam->plat_power_up(cam); + if (ret) + return ret; mcam_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); + /* + * 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; mcam_ctlr_irq_enable(cam); cam->state = S_STREAMING; if (!test_bit(CF_SG_RESTART, &cam->flags)) @@ -1503,7 +1564,9 @@ static int mcam_v4l_open(struct file *filp) ret = mcam_setup_vb2(cam); if (ret) goto out; - mcam_ctlr_power_up(cam); + ret = mcam_ctlr_power_up(cam); + if (ret) + return ret; __mcam_cam_reset(cam); mcam_set_config_needed(cam, 1); } @@ -1526,10 +1589,12 @@ static int mcam_v4l_release(struct file *filp) if (cam->users == 0) { mcam_ctlr_stop_dma(cam); mcam_cleanup_vb2(cam); + mcam_config_mipi(cam, false); mcam_ctlr_power_down(cam); if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read) mcam_free_dma_bufs(cam); } + mutex_unlock(&cam->s_mutex); return 0; } @@ -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; __mcam_cam_reset(cam); } else { mcam_ctlr_power_down(cam); diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h index 01dec9e..be271b3 100644 --- a/drivers/media/platform/marvell-ccic/mcam-core.h +++ b/drivers/media/platform/marvell-ccic/mcam-core.h @@ -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 */ diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c index c4c17fe..3dad182 100644 --- a/drivers/media/platform/marvell-ccic/mmp-driver.c +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c @@ -27,6 +27,7 @@ #include <linux/delay.h> #include <linux/list.h> #include <linux/pm.h> +#include <linux/clk.h> #include "mcam-core.h" @@ -39,6 +40,7 @@ struct mmp_camera { struct platform_device *pdev; struct mcam_camera mcam; struct list_head devlist; + struct clk *mipi_clk; int irq; }; @@ -113,7 +115,7 @@ static void mmpcam_power_up_ctlr(struct mmp_camera *cam) mdelay(1); } -static void mmpcam_power_up(struct mcam_camera *mcam) +static int mmpcam_power_up(struct mcam_camera *mcam) { struct mmp_camera *cam = mcam_to_cam(mcam); struct mmp_camera_platform_data *pdata; @@ -133,6 +135,12 @@ static void mmpcam_power_up(struct mcam_camera *mcam) mdelay(5); gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */ mdelay(5); + if (mcam->bus_type == V4L2_MBUS_CSI2 && IS_ERR(cam->mipi_clk)) { + cam->mipi_clk = devm_clk_get(mcam->dev, "mipi"); + if ((IS_ERR(cam->mipi_clk) && mcam->dphy[2] == 0)) + return PTR_ERR(cam->mipi_clk); + } + return 0; } static void mmpcam_power_down(struct mcam_camera *mcam) @@ -150,8 +158,106 @@ static void mmpcam_power_down(struct mcam_camera *mcam) pdata = cam->pdev->dev.platform_data; gpio_set_value(pdata->sensor_power_gpio, 0); gpio_set_value(pdata->sensor_reset_gpio, 0); + + if (mcam->bus_type == V4L2_MBUS_CSI2 && !IS_ERR(cam->mipi_clk)) { + devm_clk_put(mcam->dev, cam->mipi_clk); + cam->mipi_clk = ERR_PTR(-EINVAL); + } } +/* + * calc the dphy register values + * There are three dphy registers being used. + * dphy[0] - CSI2_DPHY3 + * dphy[1] - CSI2_DPHY5 + * dphy[2] - CSI2_DPHY6 + * CSI2_DPHY3 and CSI2_DPHY6 can be set with a default value + * or be calculated dynamically + */ +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 + * 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); + break; + case DPHY3_ALGO_PXA2128: + /* + * Calculate CSI2_DPHY3 algo for PXA2128 + */ + pdata->dphy[0] = + ((2 + pdata->lane_clk * 110 / 1000) & 0xff) << 8 + | (1 + pdata->lane_clk * 35 / 1000); + break; + default: + /* + * Use default CSI2_DPHY3 value for PXA688/PXA988 + */ + dev_dbg(dev, "camera: use the default CSI2_DPHY3 value\n"); + } + + /* + * mipi_clk will never be changed, it is a fixed value on MMP + */ + if (IS_ERR(cam->mipi_clk)) + return; + + /* get the escape clk, this is hard coded */ + tx_clk_esc = (clk_get_rate(cam->mipi_clk) / 1000000) / 12; + + /* + * dphy[2] - CSI2_DPHY6: + * bit 0 ~ bit 7: CK Term Enable + * Time for the Clock Lane receiver to enable the HS line + * termination. The value is calculated similarly with + * HS Term Enable + * bit 8 ~ bit 15: CK Settle + * Time interval during which the HS receiver shall ignore + * any Clock Lane HS transitions. + * The value is calibrated on the boards. + */ + pdata->dphy[2] = ((534 * tx_clk_esc / 2000 - 1) & 0xff) << 8 + | ((38 * tx_clk_esc / 1000 - 1) & 0xff); + + dev_dbg(dev, "camera: DPHY sets: dphy3=0x%x, dphy5=0x%x, dphy6=0x%x\n", + pdata->dphy[0], pdata->dphy[1], pdata->dphy[2]); +} static irqreturn_t mmpcam_irq(int irq, void *data) { @@ -174,17 +280,30 @@ static int mmpcam_probe(struct platform_device *pdev) struct mmp_camera_platform_data *pdata; int ret; + 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); INIT_LIST_HEAD(&cam->devlist); 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->mclk_min = pdata->mclk_min; + mcam->mclk_src = pdata->mclk_src; + mcam->mclk_div = pdata->mclk_div; + mcam->bus_type = pdata->bus_type; + mcam->dphy = pdata->dphy; + mcam->mipi_enabled = false; + mcam->lane = pdata->lane; mcam->chip_id = V4L2_IDENT_ARMADA610; mcam->buffer_mode = B_DMA_sg; spin_lock_init(&mcam->dev_lock); @@ -223,7 +342,6 @@ static int mmpcam_probe(struct platform_device *pdev) * Find the i2c adapter. This assumes, of course, that the * i2c bus is already up and functioning. */ - pdata = pdev->dev.platform_data; mcam->i2c_adapter = platform_get_drvdata(pdata->i2c_device); if (mcam->i2c_adapter == NULL) { ret = -ENODEV; @@ -250,7 +368,9 @@ static int mmpcam_probe(struct platform_device *pdev) /* * Power the device up and hand it off to the core. */ - mmpcam_power_up(mcam); + ret = mmpcam_power_up(mcam); + if (ret) + goto out_gpio2; ret = mccic_register(mcam); if (ret) goto out_gpio2; diff --git a/include/media/mmp-camera.h b/include/media/mmp-camera.h index 7611963..b099d4b 100644 --- a/include/media/mmp-camera.h +++ b/include/media/mmp-camera.h @@ -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; };