Re: [PATCH 0/14] staging/media/as102: new driver submission (was Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
On Tue, Oct 18, 2011 at 7:20 PM, Piotr Chmura chmoor...@poczta.onet.pl wrote: On Tue, 18 Oct 2011 11:52:17 -0400 Devin Heitmueller dheitmuel...@kernellabs.com wrote: On Tue, Oct 18, 2011 at 5:10 AM, Piotr Chmura chmoor...@poczta.onet.pl wrote: 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. Hello Piotr, Not that I want to create more work for you, but it would appear that your patches stripped off all the Signed-off-by lines for both myself and Pierrick Hascoet (the developer from the hardware vendor). You have replaced them with cc: lines, which breaks the chain of Developer's Certificate of Origin. When you take somebody else's patches, you need to preserve any existing Signed-off-by lines, adding your own at the bottom of the list. In other words, the first patch should be: Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl instead of: Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl Cc: Pierrick Hascoet pierrick.hasc...@abilis.com Cc: Devin Heitmueller dheitmuel...@kernellabs.com Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Ok, i'll resend them again. Should I replay to every patch with something like [RESEND PATCH nn/mm]..., right ? Peter Hi Peter, A common convention is to add the version of your patch in the subject like: [PATCH v2 0/14] staging/media/as102: new driver submission That way people can know that is actually a resend of a new patch-set and not a resend of the last one. Anything that is between your Signed-off and the diff get trimmed by Git when applying a patch (that is why Devin and Pierrick Signed-off where stripped in first place). So you can add information about the changes there. Something like: Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl v2: preserve any existing Signed-off-by lines so it doesn't get stripped when applied. diff -Nur linux.clean/drivers/staging/as102/as102_drv.c linux.as102_initial/drivers/staging/as102/as102_drv.c --- linux.clean/drivers/staging/as102/as102_drv.c 1970-01-01 01:00:00.0 +0100 +++ linux.as102_initial/drivers/staging/as102/as102_drv.c 2011-10-14 17:55:02.0 +0200 @@ -0,0 +1,360 @@ +/* + * Abilis Systems Single DVB-T Receiver + * Copyright (C) 2008 Pierrick Hascoetpierrick.hascoet@xx + * Copyright (C) 2010 Devin Heitmuellerdheitmueller@xx Hope it helps, -- Javier Martínez Canillas (+34) 682 39 81 69 Barcelona, Spain -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/14] staging/media/as102: new driver submission (was Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
Em 19-10-2011 09:41, Javier Martinez Canillas escreveu: On Tue, Oct 18, 2011 at 7:20 PM, Piotr Chmura chmoor...@poczta.onet.pl wrote: On Tue, 18 Oct 2011 11:52:17 -0400 Devin Heitmueller dheitmuel...@kernellabs.com wrote: On Tue, Oct 18, 2011 at 5:10 AM, Piotr Chmura chmoor...@poczta.onet.pl wrote: 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. Hello Piotr, Not that I want to create more work for you, but it would appear that your patches stripped off all the Signed-off-by lines for both myself and Pierrick Hascoet (the developer from the hardware vendor). You have replaced them with cc: lines, which breaks the chain of Developer's Certificate of Origin. When you take somebody else's patches, you need to preserve any existing Signed-off-by lines, adding your own at the bottom of the list. In other words, the first patch should be: Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl instead of: Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl Cc: Pierrick Hascoet pierrick.hasc...@abilis.com Cc: Devin Heitmueller dheitmuel...@kernellabs.com Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Ok, i'll resend them again. Should I replay to every patch with something like [RESEND PATCH nn/mm]..., right ? Peter Hi Peter, A common convention is to add the version of your patch in the subject like: [PATCH v2 0/14] staging/media/as102: new driver submission That way people can know that is actually a resend of a new patch-set and not a resend of the last one. Yes. Also, it seems that you've submitted only 12 patches of this 14 patch series. Where are the other two missing patches? Anything that is between your Signed-off and the diff get trimmed by Git when applying a patch (that is why Devin and Pierrick Signed-off where stripped in first place). So you can add information about the changes there. Something like: Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl v2: preserve any existing Signed-off-by lines so it doesn't get stripped when applied. diff -Nur linux.clean/drivers/staging/as102/as102_drv.c linux.as102_initial/drivers/staging/as102/as102_drv.c --- linux.clean/drivers/staging/as102/as102_drv.c 1970-01-01 01:00:00.0 +0100 +++ linux.as102_initial/drivers/staging/as102/as102_drv.c 2011-10-14 17:55:02.0 +0200 @@ -0,0 +1,360 @@ +/* + * Abilis Systems Single DVB-T Receiver + * Copyright (C) 2008 Pierrick Hascoetpierrick.hascoet@xx + * Copyright (C) 2010 Devin Heitmuellerdheitmueller@xx Hope it helps, -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/14] staging/media/as102: new driver submission (was Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
W dniu 19.10.2011 13:44, Mauro Carvalho Chehab pisze: Em 19-10-2011 09:41, Javier Martinez Canillas escreveu: On Tue, Oct 18, 2011 at 7:20 PM, Piotr Chmurachmoor...@poczta.onet.pl wrote: On Tue, 18 Oct 2011 11:52:17 -0400 Devin Heitmuellerdheitmuel...@kernellabs.com wrote: On Tue, Oct 18, 2011 at 5:10 AM, Piotr Chmurachmoor...@poczta.onet.pl wrote: 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. Hello Piotr, Not that I want to create more work for you, but it would appear that your patches stripped off all the Signed-off-by lines for both myself and Pierrick Hascoet (the developer from the hardware vendor). You have replaced them with cc: lines, which breaks the chain of Developer's Certificate of Origin. When you take somebody else's patches, you need to preserve any existing Signed-off-by lines, adding your own at the bottom of the list. In other words, the first patch should be: Signed-off-by: Pierrick Hascoetpierrick.hasc...@abilis.com Signed-off-by: Devin Heitmuellerdheitmuel...@kernellabs.com Signed-off-by: Piotr Chmurachmoor...@poczta.onet.pl instead of: Signed-off-by: Piotr Chmurachmoor...@poczta.onet.pl Cc: Pierrick Hascoetpierrick.hasc...@abilis.com Cc: Devin Heitmuellerdheitmuel...@kernellabs.com Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Ok, i'll resend them again. Should I replay to every patch with something like [RESEND PATCH nn/mm]..., right ? Peter Hi Peter, A common convention is to add the version of your patch in the subject like: [PATCH v2 0/14] staging/media/as102: new driver submission That way people can know that is actually a resend of a new patch-set and not a resend of the last one. Thanks, I'll do it this way next time. Yes. Also, it seems that you've submitted only 12 patches of this 14 patch series. Where are the other two missing patches? 13 and 14 were written by me, so they didn't suffer signed-off-by mistake. Do I need resend them too ? Peter -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/14] staging/media/as102: new driver submission (was Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
On Tue, Oct 18, 2011 at 5:10 AM, Piotr Chmura chmoor...@poczta.onet.pl wrote: 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. Hello Piotr, Not that I want to create more work for you, but it would appear that your patches stripped off all the Signed-off-by lines for both myself and Pierrick Hascoet (the developer from the hardware vendor). You have replaced them with cc: lines, which breaks the chain of Developer's Certificate of Origin. When you take somebody else's patches, you need to preserve any existing Signed-off-by lines, adding your own at the bottom of the list. In other words, the first patch should be: Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl instead of: Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl Cc: Pierrick Hascoet pierrick.hasc...@abilis.com Cc: Devin Heitmueller dheitmuel...@kernellabs.com Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/14] staging/media/as102: new driver submission (was Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
On Tue, 18 Oct 2011 11:52:17 -0400 Devin Heitmueller dheitmuel...@kernellabs.com wrote: On Tue, Oct 18, 2011 at 5:10 AM, Piotr Chmura chmoor...@poczta.onet.pl wrote: 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. Hello Piotr, Not that I want to create more work for you, but it would appear that your patches stripped off all the Signed-off-by lines for both myself and Pierrick Hascoet (the developer from the hardware vendor). You have replaced them with cc: lines, which breaks the chain of Developer's Certificate of Origin. When you take somebody else's patches, you need to preserve any existing Signed-off-by lines, adding your own at the bottom of the list. In other words, the first patch should be: Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl instead of: Signed-off-by: Piotr Chmura chmoor...@poczta.onet.pl Cc: Pierrick Hascoet pierrick.hasc...@abilis.com Cc: Devin Heitmueller dheitmuel...@kernellabs.com Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Ok, i'll resend them again. Should I replay to every patch with something like [RESEND PATCH nn/mm]..., right ? Peter -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
On Oct 15 Piotr Chmura wrote: Staging submission: PCTV 74e driver (as102) From: Devin Heitmuellerdheitmuel...@kernellabs.com pull as102 driver from This is driver for PCTV 74e DVB-T USB tuner, taken from [1], written by Devin Heitmueller using the GPL reference driver provided by Abilis. The only change needed to compile it in current git tree [2] was changing calls usb_buffer_alloc() and usb_buffer_free() to usb_alloc_coherent() and usb_free_coherent(). It's included in this patch. Patch was tested by me on amd64. [1]http://kernellabs.com/hg/~dheitmueller/v4l-dvb-as102-2/ [2] git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git kernel-3.1.0-git9+ Signed-off-by: Piotr Chmurachmoor...@poczta.onet.pl Cc: Devin Heitmuellerdheitmuel...@kernellabs.com Cc: Greg HKgre...@suse.de diff -Nur linux.clean/drivers/staging/as102/as102_drv.c linux.as102_initial/drivers/staging/as102/as102_drv.c --- linux.clean/drivers/staging/as102/as102_drv.c 1970-01-01 01:00:00.0 +0100 +++ linux.as102_initial/drivers/staging/as102/as102_drv.c 2011-10-14 17:55:02.0 +0200 @@ -0,0 +1,360 @@ +/* + * Abilis Systems Single DVB-T Receiver + * Copyright (C) 2008 Pierrick Hascoetpierrick.hasc...@abilis.com + * Copyright (C) 2010 Devin Heitmuellerdheitmuel...@kernellabs.com 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. -- Stefan Richter -=-==-== =-=- = http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
On Sun, Oct 16, 2011 at 4:57 AM, Stefan Richter stef...@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. 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 -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] Staging submission: PCTV 74e driver (as102)
Em 16-10-2011 09:45, Devin Heitmueller escreveu: On Sun, Oct 16, 2011 at 4:57 AM, Stefan Richter stef...@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@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
