On Wed, 16 Mar 2011 21:06:13 +0100
Patrice Chotard <patrice.chotard@xxxxxx> wrote:
> This patch add a new jeilin dual mode camera support and some
> specific controls settings.
Hi Patrice and Theodore,
Here are somme comments about Patrice's patch.
> #include <linux/workqueue.h>
> +#include <linux/delay.h>
> #include <linux/slab.h>
It is not a good idea to use mdelay(): it is a loop. Better use
msleep().
> - u8 quality; /* image quality */
> - u8 jpegqual; /* webcam quality */
> + u8 camquality; /* webcam quality */
> + u8 jpegquality; /* jpeg quality */
The webcam (encoding) quality and the jpeg (decoding) quality must be
the same. Then, looking carefully, jpegquality is not used!
> + u8 freq;
> + u8 type;
> + /* below variables are only used for SPORTSCAM_DV15 */
> + u8 autogain;
> + u8 cyan;
> + u8 magenta;
> + u8 yellow;
You should use the new control mechanism (see stk014, sonixj, zc3xx...).
> +#define V4L2_CID_CAMQUALITY (V4L2_CID_USER_BASE + 1)
> + .id = V4L2_CID_CAMQUALITY,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "Image quality",
The JPEG quality must be get/set by the VIDIOC_G_JPEGCOMP /
VIDIOC_S_JPEGCOMP ioctl's.
> +#define V4L2_CID_CYAN_BALANCE (V4L2_CID_USER_BASE + 2)
[snip]
> +#define V4L2_CID_MAGENTA_BALANCE (V4L2_CID_USER_BASE + 3)
[snip]
> +#define V4L2_CID_YELLOW_BALANCE (V4L2_CID_USER_BASE + 4)
These values redefine V4L2_CID_SATURATION and V4L2_CID_HUE (user_base +
4 is no more defined). You should use V4L2_CID_RED_BALANCE,
V4L2_CID_BLUE_BALANCE and V4L2_CID_GAIN to set these controls.
> + if (sd->type == SPORTSCAM_DV15)
> + start_commands_size = 9;
> + else
> + start_commands_size = ARRAY_SIZE(start_commands);
Don't use magic values ('9').
> + mdelay(start_commands[i].delay);
See above.
BTW, Theodore, as there is no USB command in the loop, there is no need
to have a work queue (look at the SENSOR_OV772x in ov534).
Best regards.
--
Ken ar c'hentaà | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
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
