[PATCH 0/14] staging/media/as102: new driver submission (was Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
Piotr Chmura
chmooreck at poczta.onet.pl
Tue Oct 18 09:10:44 UTC 2011
On Sun, 16 Oct 2011 11:44:14 -0200
Mauro Carvalho Chehab <mchehab at redhat.com> wrote:
> Em 16-10-2011 09:45, Devin Heitmueller escreveu:
> > On Sun, Oct 16, 2011 at 4:57 AM, Stefan Richter
> > <stefanr at s5r6.in-berlin.de> 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.
>
> Rebasing a changeset to move it to staging is not as complex as it seems.
> I did it with tm6000 at the time I merged it. A simple bash script could
> do that. Something like (untested):
>
> $ git export some_base_reference
> $ for i in 00*.patch; do sed s,/drivers/media/video,/drivers/staging,g <$i >a && mv a $i; done
> $ mkdir patches/
> $ mv 00*.patches patches/
> $ (cd patches; ls *.patch > series)
> $ git quiltimport
>
> Of course, the Makefike/Kconfig patch will need changes, but this is as easy
> as just dropping the hunks that are touching at /drivers/media/video/Makefile
> and /drivers/media/video/Kconfig, and then adding a final patch adding it to
> the /drivers/staging/*.
>
> >
> > 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?
>
> Didn't actually reviewed the changeset, but let me comment about the
> submission procedure.
>
> Folding patches from different authors generally is not a good idea.
> As Devin said, things like replacing "foo" by "bar" because "foo" core
> function were replaced by "bar" upstream is the kind of thing that you
> could fold, if you properly document it with:
>
> [john at john_email.com: replaced "foo" by "bar" to fix compilation]
>
> So, you should rebase the patchsets, instead of just folding everything.
> It is up to you to change the patches on each patch, or to do it only at
> a final patch. Both ways work for me.
>
> I suggest you to remove the Kconfig/Makefile hunks that enables the driver
> compilation from the original series, and apply it as a final patch at the
> end. This makes your rebasing work easier, as you won't need to test patch
> by patch if they are not breaking compilation.
>
> In any case, when analyzing a driver submission like that, I generally just
> apply everything from a quilt series, see the final result and review it,
> as:
> 1) the history doesn't matter for me;
> 2) it is easier to review a new driver as a hole, as the history
> may be full of things that will later be changed by something else;
> 3) checkpatch will be happier, especially if you've added some
> patches to fix checkpatch complains at the end of the series.
>
> Btw, I've agreed with Greg that I'll be moving the media staging stuff into
> a separate directory. I'll prepare the patches later, probably during the next
> merge window, in order to avoid merge conflicts between Greg's tree and mine,
> especially since I intend to move some drivers out of staging, as they seem
> to be ready to be at drivers/media.
>
> So, if you're submitting a new driver for staging, I suggest you to put it already
> into drivers/staging/media. This mean one less driver for me to move on my
> upcoming changeset ;)
>
> I hope that helps.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for comments for all of you.
[PATCH 1-12/14] Following your guidelines i exported all changes from hg one by one. This way we will have all history in kernel tree.
I moved driver to staging/media and removed Kconfig/Makefile changes in parent directory in first patch.
[PATCH 13/14] Fixes compilation and introduces Kconfig/Makefile changes in staging (which probably you'll need to change anyway in your tree).
[PATCH 14/14] Adds nBox tuner I have, so i can test the driver (works fine).
I think we can take care of making checkpatch happy when the driver will be in place. I'll resend my previous patches (esp. "[PATCH 4/7] as102: cleanup - formatting code" needs some more work).
Stats for package:
drivers/staging/Kconfig | 2
drivers/staging/Makefile | 1
drivers/staging/media/as102/Kconfig | 7
drivers/staging/media/as102/Makefile | 5
drivers/staging/media/as102/as102_drv.c | 454 ++++++++--
drivers/staging/media/as102/as102_drv.h | 147 +++
drivers/staging/media/as102/as102_fe.c | 1131 +++++++++++++++++++------
drivers/staging/media/as102/as102_fw.c | 337 ++++++-
drivers/staging/media/as102/as102_fw.h | 42
drivers/staging/media/as102/as102_usb_drv.c | 584 +++++++++++-
drivers/staging/media/as102/as102_usb_drv.h | 72 +
drivers/staging/media/as102/as10x_cmd.c | 983 ++++++++++++++++-----
drivers/staging/media/as102/as10x_cmd.h | 540 +++++++++++
drivers/staging/media/as102/as10x_cmd_cfg.c | 492 ++++++++--
drivers/staging/media/as102/as10x_cmd_stream.c | 555 ++++++++----
drivers/staging/media/as102/as10x_handle.h | 58 +
drivers/staging/media/as102/as10x_types.h | 198 ++++
17 files changed, 4705 insertions(+), 903 deletions(-)
More information about the devel
mailing list