On Wed, 11 Mar 2009 03:10:38 -0700 (PDT) Uri Shkolnik <urishk@xxxxxxxxx> wrote: > My suggestion is that we’ll “reset” the process. > > We know for sure (tested and verified) that the final result, after merging all Siano’s patches from the vger is robust driver set (which has also been tested with Mike’s devices). We also know it passes “checkpatch.pl” script (from kernel version 2.6.28) so it’s quite good, maybe some coding style issues still exist, but not too many. > So, the code itself is in good condition. CodingStyle is one of the points on submitting a patch upstream, but for sure it isn't the most important one. There are other much more important rules, like [1]: - not cause regressions; - not break bisect; - one change per patch; - no new userspace API without prior discussions and proper documentation; - no hooks for proprietary driver extensions; - no usage of deprecated functions; - no backports sent upstream; ... [1] This is just an incomplete, unstructured set of common practices. You'll see a more formal set of rules at /README.patches (on v4l-dvb tree) and on Kernel's Documentation/. The informal rules will be found by reading and following LKML discussions (and reading some good Kernel development sites, like lwn.net). The objective of a patch review from the subsystem maintainer, from the driver maintainers for the affected drivers (or boards) and from the community is to identify problems and address they in advance, before breaking the drivers or generating bisect troubles. > So, my suggestion is as follow – > Let’s take, one after the other, the patches from the vger server, and “put them on the surgeon’s table”. > Per *single* patch, one at a time, we’ll ask for anyone's review. > If there will be no comment, it will be committed as is. If there will be, I’ll fix it, pass it through the needed QA tests, and than it will be submit for a second review. I'm trying to understand better you patch series. It seems that the patch names doesn't mean much (from Patchwork): Siano 10237 __le32 to le32 Siano 10236 SPI drv minor casting issue Siano 10234 big endian support Siano 10221 smssdio Siano 10231 add submenu to kconfig Siano 10230 Core coding style fix Siano 10229 Build configuration upgrade Siano 10228 Cards upgrade Siano 10227 core upgrade Siano 10226 Siano sub-system Siano 10225 DVBv3 Siano 10224 USB adapter Siano 10223 SPI driver It is hard to understand what each patch does, by just looking on their names. The message first line should summarize what's inside each patch. Also, please don't post patches like "core upgrade" (as described by you) as: <snip> This patch provides the following: A) Support for firmware download for kernel versions < 2.6.10 B) SPI and SDIO interface driver registration C) DVB-API v5 (S2) and Siano's subsystem registration D) Indentation Note: SMS adapter for DVBv5/S2 is experimental (alpha stage). <snip/> If the patch does 4 things, it should be sent as 4 different patches (except for CodingStyle fixes at the functions you're touching that you can eventually send on the same patch). Also, in general, big patches have more probability of causing a regression. A perfect patch is small, well described and touches on just one functionality. Every time I see a patch called "foo upgrade", I have the sensation that someone just did a diff between his tree and the kernel one, and don't have a very clear idea on what's being done (and where this is done) inside the patch. People should never do a diff from his tree and kernel, when submitting new stuff upstream. This procedure is known to cause regressions, especially when some changes were done by kernel janitors, or when some internal kernel API is being changed. Also, you shouldn't send a patch for supporting firmware downloading for versions <2.6.10, since: 1) Mercurial tree doesn't support 2.6.10 kernels, so such patch doesn't belong to mercurial tree; 2) No backport is allowed when sending patches upstream. So such patch also doesn't fit upstream. --- >From my POV, you should first solve the patches that causes breakages with the currently supported hardware. After having it solved, then you will need to break your patches into smaller ones and provide better descriptions for they. If you have patches that don't depend on that gpio changes, nor cause any regression on the currently supported devices, then you can start sending they again, with a good description, one change by patch. Patches that affects other drivers require an explicit ack or sob from the driver author. Ah, please avoid flooding me with a large amount of patches. The better is to submit me some patches, wait for my review (and for the community review), and then send more patches. The more simple is each individual patch, the more fast I'll analyse and apply, if ok. Before submitting the patches, I recommend you to read the /README.patches and the Documentation ones that are mentioned there, for you to be more familiar with the adopted procedures. Cheers, Mauro -- 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
