Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marek

On Sat, 22 Aug 2009, Marek Vasut wrote:

> From 479aafc9a6540efec8a691a84aff166eb0218a72 Mon Sep 17 00:00:00 2001
> From: Marek Vasut <marek.vasut@xxxxxxxxx>
> Date: Sat, 22 Aug 2009 05:14:23 +0200
> Subject: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
> 
> Signed-off-by: Marek Vasut <marek.vasut@xxxxxxxxx>

Ok, I see, you rebased your patches on my soc-camera patch-stack, this is 
good, thanks. But you missed a couple of my comments - you still have a 
few static functions in the ov9640.c marked inline: ov9640_set_crop() is 
not used at all, ov9640_set_bus_param() gets assigned to a struct member, 
so, cannot be inline. ov9640_alter_regs() is indeed only called at one 
location, but see Chapter 15 in Documentation/CodingStyle. You left at 
least one multi-line comment wrongly formatted. You left some broken 
printk format lines, etc. You moved your header under drivers/... - that's 
good. But, please, address all my comments that I provided in a private 
review, or explain why you do not agree. Otherwise I feel like I wasted my 
time. Besides, your mailer has wrapped lines. Please, read 
Documentation/email-clients.txt on how to configure your email client to 
send patches properly. In the worst case, you can inline your patches 
while reviewing, and then attach them for a final submission. This is, 
however, discouraged, because it makes review and test of your 
intermediate patches with wrapped lines more difficult. Also, please check 
your patches with scripts/checkpatch.pl.

[patch skipped]

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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

[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux