Re: [uclinux-dist-devel] [PATCH 3/4] v4l2: add vs6624 sensor driver
On Wed, Sep 14, 2011 at 03:28, Scott Jiang wrote: +#ifdef CONFIG_VIDEO_ADV_DEBUG just use DEBUG ? no, v4l2 use CONFIG_VIDEO_ADV_DEBUG ok, i was thinking this was something we added (since we have ADVxxx parts) + v4l_info(client, chip found @ 0x%02x (%s)\n, + client-addr 1, client-adapter-name); is that 1 correct ? i dont think so ... every driver under media I see use this, so what's wrong? meh, they're all wrong imo then :p. they're shifting the address to accommodate datasheets that incorrectly specify the i2c address with the read/write as bit 0. but it's fine for this driver to do that if it's the standard that the rest of the v4l code has adopted. -mike -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [uclinux-dist-devel] [PATCH 3/4] v4l2: add vs6624 sensor driver
+#ifdef CONFIG_VIDEO_ADV_DEBUG just use DEBUG ? no, v4l2 use CONFIG_VIDEO_ADV_DEBUG + v4l_info(client, chip found @ 0x%02x (%s)\n, + client-addr 1, client-adapter-name); is that 1 correct ? i dont think so ... every driver under media I see use this, so what's wrong? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [uclinux-dist-devel] [PATCH 3/4] v4l2: add vs6624 sensor driver
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
