From: Mauro Carvalho Chehab <mchehab@redhat.com>
To: "Frank Schäfer" <fschaefer.oss@googlemail.com>
Cc: linux-media@vger.kernel.org
Subject: Re: [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again
Date: Sat, 5 Jan 2013 11:26:17 -0200 [thread overview]
Message-ID: <20130105112617.79c3c57a@redhat.com> (raw)
In-Reply-To: <50E8236C.6070702@googlemail.com>
Em Sat, 05 Jan 2013 13:58:20 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 04.01.2013 22:12, schrieb Mauro Carvalho Chehab:
> > Em Fri, 28 Dec 2012 00:02:45 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Tested with device "Terratec Cinergy 200 USB".
> > Sorry, but this patch is completely wrong ;)
>
> Completely wrong ? That's not helpful...
> Please elaborate a bit more on this so that I can do things right next
> time. ;)
Sorry, I was too busy yesterday with the tests to elaborate it more.
In general, big patches like that to fix bug fixes are generally wrong:
they touch on a lot of the code and it is hard to be sure that it doesn't
come with regressions on it.
In this particular case, it was:
- mixing bug fixes with some other random stuff;
- moving only one part of the IR needed data elsewhere (it were
moving the IR tables, to the board descriptions, keeping them on
a separate part of the code, obfuscating the code);
- putting a large amount of the code inside an if, increasing the
driver's complexity with no need;
- initializing some data for IR that are never used, at em28xx_ir_init;
- not fixing the snapshot button.
The bug fix was as simple as:
1) move snapshot button register to happen before IR;
2) move I2C init to happen before the em2860/2874 IR init.
See:
http://git.linuxtv.org/media_tree.git/commitdiff/8303dc9952758ab3060a3ee9a19ecb6fec83c600
That's simple to review, and the produced patch can be accepted later at
stable, because it is not a code rewrite.
Of course, if we backport this to -stable, this one is also recommended:
http://git.linuxtv.org/media_tree.git/commitdiff/728f9778e273a11a65926ac21574e6ca8d911ebf
in order to autoload the RC extension automatically for I2C IR's, but this
is a separate bug.
Btw, I really prefer to have the RC tables for the I2C devices inside
em28xx-input, as:
1) there are other board-specific platform_data that needed to
be filled for the IR to work there;
2) we want to keep all those platform_data initialization together,
to make the code simpler to maintain;
3) moving all those data to em28xx cards struct is a bad idea, as
newer em28xx won't use I2C IR's, as the new chipsets have already
its own IR decoder. Moving those 4-5 fields to the board struct
would increase its size for every board. So, it would be a waste
of space.
Regards,
Mauro
next prev parent reply other threads:[~2013-01-05 13:26 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
2012-12-27 23:02 ` [PATCH 1/6] em28xx: simplify device state tracking Frank Schäfer
2012-12-27 23:02 ` [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect() Frank Schäfer
2013-01-02 19:40 ` Antti Palosaari
2013-01-02 21:18 ` Frank Schäfer
2012-12-27 23:02 ` [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
2013-01-04 21:12 ` Mauro Carvalho Chehab
2013-01-05 12:58 ` Frank Schäfer
2013-01-05 13:26 ` Mauro Carvalho Chehab [this message]
2013-01-05 13:51 ` Frank Schäfer
2012-12-27 23:02 ` [PATCH 4/6] em28xx: IR RC: get rid of function em28xx_get_key_terratec() Frank Schäfer
2013-01-05 2:41 ` Mauro Carvalho Chehab
2012-12-27 23:02 ` [PATCH 5/6] em28xx: IR RC: move assignment of get_key functions from *_change_protocol() functions to em28xx_ir_init() Frank Schäfer
2012-12-27 23:02 ` [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1() Frank Schäfer
2013-01-05 2:39 ` Mauro Carvalho Chehab
2013-01-05 13:32 ` Frank Schäfer
2013-01-05 15:25 ` Mauro Carvalho Chehab
2013-01-06 20:32 ` Frank Schäfer
2013-01-07 16:40 ` Mauro Carvalho Chehab
2013-01-08 17:39 ` Frank Schäfer
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=20130105112617.79c3c57a@redhat.com \
--to=mchehab@redhat.com \
--cc=fschaefer.oss@googlemail.com \
--cc=linux-media@vger.kernel.org \
/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).