On Sat, 15 Dec 2012 17:57:51 +0800
Albert Wang <twang13@xxxxxxxxxxx> wrote:
> From: Libin Yang <lbyang@xxxxxxxxxxx>
>
> This patch adds the MIPI support for marvell-ccic.
> Board driver should determine whether using MIPI or not.
There are limits to how deeply I can review this, since I know little about
the MIPI mode and don't have any hardware that uses it. So I'm assuming
that it all works :) My comments are on a different level.
> +static int mcam_config_mipi(struct mcam_camera *mcam, int enable)
> +{
> + if (mcam->bus_type == V4L2_MBUS_CSI2 && 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_DPHY6, mcam->dphy[2]);
> + mcam_reg_write(mcam, REG_CSI2_DPHY5, mcam->dphy[1]);
Is there a reason you're writing them in something other than direct
increasing order? If so, a comment saying why might help somebody in ghe future.
> + if (mcam->mipi_enabled == 0) {
> + /*
> + * 0x41 actives 1 lane
> + * 0x43 actives 2 lanes
> + * 0x47 actives 4 lanes
> + * There is no 3 lanes case
> + */
> + switch (mcam->lane) {
> + case 1:
> + mcam_reg_write(mcam, REG_CSI2_CTRL0, 0x41);
> + break;
> + case 2:
> + mcam_reg_write(mcam, REG_CSI2_CTRL0, 0x43);
> + break;
> + case 4:
> + mcam_reg_write(mcam, REG_CSI2_CTRL0, 0x47);
> + break;
Can we have defined symbols rather than magic constants here?
> @@ -656,6 +701,15 @@ 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);
> + mcam_reg_write(cam, REG_CTRL3, 0x4);
Again, how about a symbol, or at least an explanation of what 0x4 means?
> + }
> }
>
[...]
> @@ -1551,6 +1615,11 @@ static int mcam_v4l_open(struct file *filp)
> mcam_set_config_needed(cam, 1);
> }
> (cam->users)++;
> + cam->pll1 = devm_clk_get(cam->dev, "pll1");
> + if (IS_ERR(cam->pll1)) {
> + cam_err(cam, "Could not get pll1 clock\n");
> + ret = PTR_ERR(cam->pll1);
> + }
This looks like it gets the clock in all cases? Is that right?
> #define REG_IMGPITCH 0x24 /* Image pitch register */
> @@ -292,7 +311,9 @@ int mccic_resume(struct mcam_camera *cam);
> #define C0_DOWNSCALE 0x08000000 /* Enable downscaler */
> #define C0_SIFM_MASK 0xc0000000 /* SIF mode bits */
> #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_EOF_VSYNC 0x00400000 /* Generate EOF by VSYNC */
> +#define C0_VEDGE_CTRL 0x00800000 /* Detect falling edge of VSYNC */
Being the retentive sort of guy I am, I try to keep definitions like these
in numerical order. Any chance you could humor me and maintain that?
> /* Bits below C1_444ALPHA are not present in Cafe */
> #define REG_CTRL1 0x40 /* Control 1 */
> @@ -308,6 +329,7 @@ int mccic_resume(struct mcam_camera *cam);
> #define C1_TWOBUFS 0x08000000 /* Use only two DMA buffers */
> #define C1_PWRDWN 0x10000000 /* Power down */
>
> +#define REG_CTRL3 0x1ec /* CCIC parallel mode */
> #define REG_CLKCTRL 0x88 /* Clock control */
> #define CLK_DIV_MASK 0x0000ffff /* Upper bits RW "reserved" */
Here, too, I'd rather keep them in order if possible.
> +/*
> + * calc the dphy register values
> + * There are three dphy registers being used.
> + * dphy[0] 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 dphy[0] is calculated dynamically,
> + * pdata->lane_clk should be already set
> + * either in the board driver statically
> + * or in the sensor driver dynamically.
> + */
> + switch (pdata->dphy3_algo) {
> + case 1:
> + /*
> + * dphy3_algo == 1
> + * Calculate CSI2_DPHY3 algo for PXA910
> + */
> + pdata->dphy[0] = ((1 + pdata->lane_clk * 80 / 1000) & 0xff) << 8
> + | (1 + pdata->lane_clk * 35 / 1000);
> + break;
What are the chances of getting a comment or some other reference so that a
naive reader like me has a chance of understanding what this calculation
does? Where do all those constants come from?
> + case 2:
> + /*
> + * dphy3_algo == 2
> + * 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:
> + /*
> + * dphy3_algo == 0
> + * Use default CSI2_DPHY3 value for PXA688/PXA988
> + */
> + dev_dbg(dev, "camera: use the default CSI2_DPHY3 value\n");
> + }
> +
> + /*
> + * pll1 will never be changed, it is a fixed value
> + */
> +
> + if (IS_ERR(mcam->pll1))
> + return;
> +
> + tx_clk_esc = clk_get_rate(mcam->pll1) / 1000000 / 12;
Being who I am (see "retentive" above) I'll always parenthesize an
expression like this to make the intended order of operations explicit.
Mostly low-level comments, it's looking pretty good.
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
