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);elseret = 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.