Hi Janusz,
This patch provides a V4L2 SoC Camera driver for OV6650 camera sensor, found on OMAP1 SoC based Amstrad Delta videophone.
Nice work. I don't understand the subject matter so I've just scanned for cut and paste errors, etc. I thought there was the odd place, e.g.
+#define REG_COML 0x16
where the use of spaces and tabs didn't match over lines. Don't know if that matters to the kernel folks. expand(1) and unexpand(1) may help.
+ dev_err(&client->dev, "Failed reading register 0x%02x!\n", reg); + dev_err(&client->dev, "Failed writing register 0x%02x!\n", reg); + dev_err(&client->dev, + "Failed reading back register 0x%02x!\n", reg); + dev_err(&client->dev, + "[Read]-Modify-Write of register %02x failed!\n", reg); + dev_err(&client->dev, + "Read-Modify-[Write] of register %02x failed!\n", reg); + dev_err(&client->dev, "Pixel format not handled : %x\n", code); + dev_err(&client->dev, "Product ID error %x:%x\n", pidh, pidl); + dev_info(&client->dev, "ov6650 Product ID %0x:%0x Manufacturer ID %x:%x" + "\n", pidh, pidl, midh, midl);
There's a variety of styles used. Sometimes `0x' prefixes hex output, sometimes not. It may be easier to use the printf(3)'s `#' modifier which the kernel printf() also supports, e.g. dev_err(&client->dev, "Failed reading register %#x!\n", reg); This prints the `0x' for you, and even saves a byte in the string. :-) Also, there's no point doing `%0x', the `0' is having no effect there.
+/* Set status of additional camera capabilities */ +static int ov6650_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl) ... + switch (ctrl->id) { + case V4L2_CID_AUTOGAIN: + if (ctrl->value) { + ret = ov6650_reg_write(client, REG_GAIN, DEF_GAIN); + if (ret) + break; + priv->gain = DEF_GAIN; + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AGC, 0); + } else { + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AGC); + } + if (ret) + break; + priv->agc = ctrl->value; + break;
Would if (!ret) priv->agc = ctrl->value; break; be preferable at the end? There's a whole load the same and it reads to me like a bit of a double-negative. Perhaps you'd prefer to keep the "if error then bail" pattern going.
+ case V4L2_CID_HUE_AUTO: + if (ctrl->value) { + ret = ov6650_reg_rmw(client, REG_HUE, + SET_HUE(DEF_HUE), SET_HUE(HUE_MASK)); + if (ret) + break; + priv->hue = DEF_HUE; + } else { + ret = ov6650_reg_rmw(client, REG_HUE, HUE_EN, 0); + } + if (ret) + break; + priv->hue_auto = ctrl->value; + break;
The second test of ret can be inside the else.
+ if (automatic) + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AEC, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AEC);
There's quite a few calls to ov6650_reg_rmw() like this. Is ret = ov6650_reg_rmw(client, REG_COMB, automatic ? COMB_AEC : 0, COMB_AEC); better? The mask can always be passed and causes no extra work in ov6650_reg_rmw(), and not having to decide whether to pass a mask of 0 or COMB_AEC saves work for the caller.
+/* select nearest higher resolution for capture */ +static void ov6650_res_roundup(u32 *width, u32 *height) +{ + int i; + enum { QCIF, CIF }; + int res_x[] = { 176, 352 }; + int res_y[] = { 144, 288 };
Making res_x and res_y static should reduce the size of the text produced. It does here with a quick -O3 x86 test. A way that avoids the duplicating the assignment is static void ov6650_res_roundup(u32 *width, u32 *height) { int i, lasti; static int res_x[] = { W_QCIF, W_CIF }; static int res_y[] = { H_QCIF, H_CIF }; for (i = 0; i < ARRAY_SIZE(res_x); i++) { lasti = i; if (res_x[i] >= *width && res_y[i] >= *height) break; } *width = res_x[lasti]; *height = res_y[lasti]; } This avoids the need for the enum and I've also used W_QCIF, etc., instead.
+static int ov6650_enum_fmt(struct v4l2_subdev *sd, unsigned int index, + enum v4l2_mbus_pixelcode *code) +{ + if ((unsigned int)index >= ARRAY_SIZE(ov6650_codes)) + return -EINVAL; + + *code = ov6650_codes[index]; + return 0; +}
Is the cast of index required?
+ memset(cp, 0, sizeof(struct v4l2_captureparm));
Using `sizeof *cp' would mean I don't have to check against cp's declaration. Sorry it's just lots of nit-picky stuff. Cheers, Ralph.