On Monday 28 April 2008 15:48:19 Mauro Carvalho Chehab wrote:
> 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.
No, this is right. Only request the module if it is not compiled into
the kernel.
>
> + /* 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.
Done. I also put this whole block of code between #if 0 since GOP end
detection is currently broken in the cx18 firmware.
>
> Please, change it to use video_ioctl2 for 2.6.27.
When I convert ivtv, I'll also convert cx18. Note that cx18 closely
resembles ivtv, but there are just too many differences all over that
prevented me from merging the two (although cx18 also uses the cx2341x
module).
>
> 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.
Done.
> 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)
Done.
> 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.
I've shortened lines where I thought it wouldn't impact the readability
too much. It has about 100 fewer of these warnings now.
> 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.
Removed the include of this header altogether. Doesn't seem to be
needed.
>
> 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.
Removed volatile here.
>
> 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;
Done.
>
> 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++);
I'll post a new PULL request.
Thanks for the review.
Regards,
Hans
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
