On Wed, 11 Mar 2009 03:10:38 -0700 (PDT)
Uri Shkolnik <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
