On Tue, Sep 13, 2011 at 14:34, Scott Jiang wrote:
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
>
> +obj-$(CONFIG_VIDEO_VS6624) += vs6624.o
> obj-$(CONFIG_VIDEO_VPX3220) += vpx3220.o
should be after vpx, not before ?
> --- /dev/null
> +++ b/drivers/media/video/vs6624.c
>
> +#include <asm/gpio.h>
run these patches through checkpatch.pl ? this should be linux/gpio.h ...
> +static const u16 vs6624_p1[] = {
> +static const u16 vs6624_p2[] = {
add comments as to what these are for ?
> +static inline int vs6624_read(struct v4l2_subdev *sd, u16 index)
> +static inline int vs6624_write(struct v4l2_subdev *sd, u16 index,
> + u8 value)
should these be inline ? they're a little "fat" ... better to let the
compiler choose
> +static int vs6624_writeregs(struct v4l2_subdev *sd, const u16 *regs)
> +{
> + u16 reg, data;
> +
> + while (*regs != 0x00) {
> + reg = *regs++;
> + data = *regs++;
> +
> + vs6624_write(sd, reg, (u8)data);
what's the point of declaring data as u16 if the top 8 bits are never used ?
> +static int vs6624_g_chip_ident(struct v4l2_subdev *sd,
> + struct v4l2_dbg_chip_ident *chip)
> +{
> + int rev;
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + rev = vs6624_read(sd, VS6624_FW_VSN_MAJOR) << 8
> + | vs6624_read(sd, VS6624_FW_VSN_MINOR);
i'm a little surprised the compiler didnt warn about this. usually
bit shifts + bitwise operators want paren to keep things clear.
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
just use DEBUG ?
> + v4l_info(client, "chip found @ 0x%02x (%s)\n",
> + client->addr << 1, client->adapter->name);
is that "<< 1" correct ? i dont think so ...
-mike
--
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
