From: "Frank Schäfer" <fschaefer.oss@googlemail.com>
To: dheitmueller@kernellabs.com
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>, linux-media@vger.kernel.org
Subject: Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers
Date: Tue, 30 Oct 2012 19:29:26 +0200 [thread overview]
Message-ID: <50900E76.8080903@googlemail.com> (raw)
In-Reply-To: <CAGoCfiw+G2CnGJSum2k9M80XizKSTfw34gXZOkOZBp_OvSTtjQ@mail.gmail.com>
Am 30.10.2012 15:08, schrieb Devin Heitmueller:
> On Tue, Oct 30, 2012 at 12:06 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Did a git bisect. The last patch where the bug doesn't occur is this
>> changeset:
>> em28xx: add module parameter for selection of the preferred USB transfer type
>>
>> That means that this changeset broke it:
>>
>> em28xx: use common urb data copying function for vbi and non-vbi devices
> Oh good, when I saw the subject line for that patch, I got worried.
> Looking at the patch, it seems like he just calls the VBI version for
> both cases assuming the VBI version is a complete superset of the
> non-VBI version, which it is clearly not.
Devin, I've read the code carefully and both functions are basically
doing the same, except that the non-VBI-version bails out when it
detects a VBI-header (see the FIXME statement ;) ).
Btw: the header checks at the end of the non-VBI-version are incomplete,
which corrupts frames from time to time when using bulk transfers.
Fixing this would make both functions even more similar.
> That whole patch should just be reverted. If he's going to spend the
> time to refactor the code to allow the VBI version to be used for both
> then fine, but blindly calling the VBI version without making real
> code changes is *NOT* going to work.
That's all plain wrong.
a) Nothing needs to be reverted, because these patches are not yet in
the repo.
b) I'm doing nothing blindly here, I tested the patch carefully and it
works with Silvercrest webcam (em2820).
c) the patch not just calls the VBI-version of the function, it also
fixes/improves some issues. And (as mentioned above), it does a few
things better than non-VBI-version, so I don't need to fix the latter.
> Frank, good job in naming your patch - it made me scream "WAIT!" when
> I saw it.
Ehm... ??? That sounded a bit different in your previous mail...
Citation:
"This is generally good stuff. When I originally added the VBI support,
I kept the URB handlers separate initially to reduce the risk of breaking
existing devices, and always assumed that at some point the two routines
would be merged."
> Bad job for blindly submitting a code change without any
> idea whether it actually worked. ;-)
See above, it's plain wrong.
> I know developers have the tendency to look at code and say "oh,
> that's ugly, I should change that." However it's more important that
> it actually work than it be pretty.
I agree.
And some developers have the tendency to just add tons of very similar
new code which is basically doing the same just because it saves time. ;)
But thinking about things a few minutes longer / doing some more testing
at the beginning saves much more time in the long run...
Devin, your whole message really isn't helpfull.
Instead of making lots of wrong assertions, it would be nice to get some
constructive comments regarding the code changes or hardware behavior.
Regards,
Frank
> Devin
>
next prev parent reply other threads:[~2012-10-30 18:29 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-21 16:52 [PATCH 00/23] em28xx: add support fur USB bulk transfers Frank Schäfer
2012-10-21 16:52 ` [PATCH 01/23] em28xx: fix wrong data offset for non-interlaced mode in em28xx_copy_video Frank Schäfer
2012-10-21 16:52 ` [PATCH 02/23] em28xx: clarify meaning of field 'progressive' in struct em28xx Frank Schäfer
2012-10-21 16:52 ` [PATCH 03/23] em28xx: rename isoc packet number constants and parameters Frank Schäfer
2012-10-21 16:52 ` [PATCH 04/23] em28xx: rename struct em28xx_usb_isoc_bufs to em28xx_usb_bufs Frank Schäfer
2012-10-21 16:52 ` [PATCH 05/23] em28xx: rename struct em28xx_usb_isoc_ctl to em28xx_usb_ctl Frank Schäfer
2012-10-21 16:52 ` [PATCH 06/23] em28xx: remove obsolete #define EM28XX_URB_TIMEOUT Frank Schäfer
2012-10-21 16:52 ` [PATCH 07/23] em28xx: update description of em28xx_irq_callback Frank Schäfer
2012-10-21 16:52 ` [PATCH 08/23] em28xx: rename function em28xx_uninit_isoc to em28xx_uninit_usb_xfer Frank Schäfer
2012-10-21 16:52 ` [PATCH 09/23] em28xx: create a common function for isoc and bulk URB allocation and setup Frank Schäfer
2012-10-21 16:52 ` [PATCH 10/23] em28xx: create a common function for isoc and bulk USB transfer initialization Frank Schäfer
2012-10-21 16:52 ` Frank Schäfer
2012-10-21 16:52 ` [PATCH 11/23] em28xx: clear USB halt/stall condition in em28xx_init_usb_xfer when using bulk transfers Frank Schäfer
2012-10-21 16:52 ` [PATCH 12/23] em28xx: remove double checks for urb->status == -ENOENT in urb_data_copy functions Frank Schäfer
2012-10-21 16:52 ` [PATCH 13/23] em28xx: rename function em28xx_isoc_copy and extend for USB bulk transfers Frank Schäfer
2012-10-21 16:52 ` [PATCH 14/23] em28xx: rename function em28xx_isoc_copy_vbi " Frank Schäfer
2012-10-21 16:52 ` [PATCH 15/23] em28xx: rename function em28xx_dvb_isoc_copy " Frank Schäfer
2012-10-21 16:52 ` [PATCH 16/23] em28xx: rename usb debugging module parameter and macro Frank Schäfer
2012-10-21 16:52 ` [PATCH 17/23] em28xx: rename some USB parameter fields in struct em28xx to clarify their role Frank Schäfer
2012-10-21 16:52 ` [PATCH 18/23] em28xx: add fields for analog and DVB USB transfer type selection to struct em28xx Frank Schäfer
2012-10-21 16:52 ` [PATCH 19/23] em28xx: set USB alternate settings for analog video bulk transfers properly Frank Schäfer
2012-10-21 16:52 ` [PATCH 20/23] em28xx: improve USB endpoint logic, also use bulk transfers Frank Schäfer
2012-10-21 16:52 ` [PATCH 21/23] em28xx: add module parameter for selection of the preferred USB transfer type Frank Schäfer
2012-10-21 16:52 ` [PATCH 22/23] em28xx: use common urb data copying function for vbi and non-vbi devices Frank Schäfer
2012-10-21 16:52 ` [PATCH 23/23] em28xx: enable VBI-support for em2840 devices Frank Schäfer
2012-10-21 18:13 ` [PATCH 00/23] em28xx: add support fur USB bulk transfers Devin Heitmueller
2012-10-23 19:02 ` Frank Schäfer
[not found] ` <20121028175752.447c39d5@redhat.com>
2012-10-29 15:33 ` Frank Schäfer
2012-10-29 20:03 ` Mauro Carvalho Chehab
2012-10-29 21:14 ` Frank Schäfer
2012-10-30 3:00 ` Mauro Carvalho Chehab
2012-10-30 4:06 ` Mauro Carvalho Chehab
2012-10-30 13:08 ` Devin Heitmueller
2012-10-30 14:03 ` Mauro Carvalho Chehab
2012-10-30 17:29 ` Frank Schäfer [this message]
2012-10-30 17:18 ` Frank Schäfer
2012-11-08 18:03 ` Frank Schäfer
2012-11-09 15:02 ` Mauro Carvalho Chehab
2012-11-09 16:03 ` Frank Schäfer
2012-10-31 1:39 ` Benny Amorsen
2012-10-31 10:57 ` Ezequiel Garcia
2012-10-31 12:03 ` Frank Schäfer
2012-10-31 20:24 ` Benny Amorsen
2012-10-31 20:22 ` Benny Amorsen
2012-10-31 11:50 ` Frank Schäfer
2012-10-31 19:58 ` Benny Amorsen
2012-10-31 20:25 ` Ezequiel Garcia
2012-10-31 22:26 ` Benny Amorsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50900E76.8080903@googlemail.com \
--to=fschaefer.oss@googlemail.com \
--cc=dheitmueller@kernellabs.com \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).