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

Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)

On Sun, Oct 16, 2011 at 4:57 AM, Stefan Richter
<stefanr@xxxxxxxxxxxxxxxxx> wrote:
> Hi Piotr,
> thanks for getting this going again.  - I have not yet looked through the
> source but have some small remarks on the patch format.
>  - In your changelogs and in the diffs, somehow the space between real
>    name and e-mail address got lost.
>  - The repetition of the Subject: line as first line in the changelog is
>    unnecessary (and would cause an undesired duplication e.g. when git-am
>    is used, last time I checked).
>  - AFAICT, author of patch 1/7 is not Devin but you.  Hence the From: line
>    right above the changelog is wrong.
>  - The reference to the source hg tree is very helpful.  However, since
>    the as102 related history in there is very well laid out, it may be
>    beneficial to quote some of this prior history.  I suggest, include
>    the changelog of "as102: import initial as102 driver",
>    http://kernellabs.com/hg/~dheitmueller/v4l-dvb-as102-2/rev/a78bda1e1a0b
>    (but mark it clearly as a quote from the out-of-tree repo), and include
>    a shortlog from this commit inclusive until the head commit inclusive.
>    (Note, the hg author field appears to be wrong; some of the changes
>    apparently need to be attributed to Pierrick Hascoet as author.)
>    This would IMO improve the picture of who contributed what when this
>    goes into mainline git history, even though the hg history needed to
>    be discarded.
>  - A diffstat is always very nice to have in a patch posting.  Most tools
>    for patch generation make it easy to add, and it helps the recipients
>    of the patch posting.
>    (Also, a diffstat of all patches combined would be good to have in the
>    introductory PATCH 0/n posting, but not many developers take the time
>    to do so.)
> Again, thanks for the effort and also thanks to Devin for making it
> possible.

I think collapsing my entire patch series in to a single patch would
not be acceptable, as it loses the entire history of what code was
originally delivered by Abilis as well as what changes I made.  The
fact that it's from multiple authors (including a commercial entity
contributing the code) makes this worse.

I think the tree does need to be rebased, but I don't think the entire
patch series would need to be reworked to be on staging from the
beginning.  You could probably import the first patch (as102: import
initial as102 driver), fix the usb_alloc_coherent() so that it
compiles (and put a note in it saying you did), apply the rest of the
patch series, and then add a final patch that says something like
"moving to staging as code is not production ready".  This would allow
the history to be preserved without having to rebase every patch to
deal with the files being moved to the staging tree.

An alternative would be make a minor tweak to my first patch which
removes the driver from the makefile.  Then every patch in the patch
series wouldn't actually have to compile successfully until the very
end when you add it back into the Makefile.  This is a luxury you can
do when it's a brand new driver.

When it's a brand new driver there is a bit more flexibility as long
as you don't break "git bisect".  Both of the approaches described
above accomplish that.

Mauro, what do you think?


Devin J. Heitmueller - Kernel Labs
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

[Linux Input]     [Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Yosemite Backpacking]

Add to Google Powered by Linux