On Sun, 27 Apr 2008 12:55:11 +0200 Hans Verkuil <[EMAIL PROTECTED]> wrote:
> Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-cx18 for > the following: How! a very big changeset. Probably, I'll miss some things. I have just a few comments about the first patch. Thanks for your good work! > > - cx18: new driver for the Conexant CX23418 MPEG encoder chip Hmm... this seems to be wrong. IMO, it should be just the opposite. + /* load modules */ +#ifndef CONFIG_VIDEO_TUNER + hw = cx18_request_module(cx, hw, "tuner", CX18_HW_TUNER); +#endif +#ifndef CONFIG_VIDEO_CS5345 + hw = cx18_request_module(cx, hw, "cs5345", CX18_HW_CS5345); +#endif + /* To convert jiffies to ms, we must multiply by 1000 + * and divide by HZ. To avoid runtime division, we + * convert this to multiplication by 1000/HZ. + * Since integer division truncates, we get the best + * accuracy if we do a rounding calculation of the constant. + * Think of the case where HZ is 1024. + */ + duration = ((1000 + HZ / 2) / HZ) * (jiffies - then); Please, use jiffies_to_ms() instead. Please, change it to use video_ioctl2 for 2.6.27. The Makefile seems broken for in-kernel compilation. Ok, a later changeset fixes it, but this will mean that bisect will be broken. If Mkrufky acks, the better is to merge the "add DVB dependency" on this changeset. There are a few checkpatch.pl errors that I think you should address: ERROR: do not use assignment in if condition #3278: FILE: linux/drivers/media/video/cx18/cx18-driver.c:186: + if (intr && (ret = signal_pending(current))) better to do: ret = signal_pending(current); if (intr && ret) There are a large number of compiants about 80 columns violation. Ok, this is just a warning for a minor codingstyle violation. Yet, it is a violation. Due to the large number of warnings (288), probably, you've missed those: WARNING: Use #include <linux/uaccess.h> instead of <asm/uaccess.h> #4114: FILE: linux/drivers/media/video/cx18/cx18-driver.h:47: +#include <asm/uaccess.h> The usage of asm should be avoided, since this may break on some architectures. Always prefer to use linux/foo, if available. WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #5782: FILE: linux/drivers/media/video/cx18/cx18-firmware.c:106: +static int load_cpu_fw_direct(const char *fn, volatile u8 __iomem *mem, struct cx18 *cx, long size) WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #5790: FILE: linux/drivers/media/video/cx18/cx18-firmware.c:114: + volatile u32 __iomem *dst = (volatile u32 __iomem *)mem; WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #5827: FILE: linux/drivers/media/video/cx18/cx18-firmware.c:151: +static int load_apu_fw_direct(const char *fn, volatile u8 __iomem *dst, struct cx18 *cx, long size) I didn't checked if this is valid on your case, but probably you don't need volatile there. Please check the referred doc. WARNING: braces {} are not necessary for single statement blocks #6289: FILE: linux/drivers/media/video/cx18/cx18-i2c.c:100: + for (i = 0; cx->i2c_clients[i] && i < I2C_CLIENTS_MAX; i++) {} I would, instead, use this: for (i = 0; i < I2C_CLIENTS_MAX; i++) if (cx->i2c_clients[i]) break; This also seems to be valid, but, IMO, not as clear as the previous one: for (i = 0; cx->i2c_clients[i] && i < I2C_CLIENTS_MAX; i++); Cheers, Mauro _______________________________________________ ivtv-devel mailing list [email protected] http://ivtvdriver.org/mailman/listinfo/ivtv-devel
