[E3-hacking] [RFC] [PATCH 3/6] SoC Camera: add driver for OV6650 sensor
Ralph Corderoy
ralph at inputplus.co.uk
Sun Jul 18 12:35:47 BST 2010
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.
More information about the e3-hacking
mailing list