[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