[1/7] marvell-ccic: add MIPI support for marvell-ccic driver
Commit Message
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(-)
Comments
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
@@ -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)
@@ -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);
@@ -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 */
@@ -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;
@@ -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;
};