| Message ID | 1356649368-5426-7-git-send-email-fschaefer.oss@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Fri, 28 Dec 2012 00:02:48 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > - return valid key code when button is hold > - debug: print key code only when a button is pressed > > Tested with device "Terratec Cinergy 200 USB" (em28xx). > > Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > --- > drivers/media/i2c/ir-kbd-i2c.c | 15 +++++---------- > 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) > > diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c > index 08ae067..2984b7d 100644 > --- a/drivers/media/i2c/ir-kbd-i2c.c > +++ b/drivers/media/i2c/ir-kbd-i2c.c > @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) > return -EIO; > } > > - /* it seems that 0xFE indicates that a button is still hold > - down, while 0xff indicates that no button is hold > - down. 0xfe sequences are sometimes interrupted by 0xFF */ > - > - dprintk(2,"key %02x\n", b); > - > - if (b == 0xff) > + if (b == 0xff) /* no button */ > return 0; > > - if (b == 0xfe) > - /* keep old data */ > - return 1; > + if (b == 0xfe) /* button is still hold */ > + b = ir->rc->last_scancode; /* keep old data */ > + > + dprintk(2,"key %02x\n", b); > > *ir_key = b; > *ir_raw = b; Don't do that. This piece of code is old, and it was added there before the em28xx driver. Originally, the ir-i2c-kbd were used by bttv and saa7134 drivers and the code there were auto-detecting the I2C IR hardware decoding chips that used to be very common on media devices. I'm almost sure that the original device that started using this code is this model: drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS", That's why it is called as KNC1, but there are other cards that use it as well. I think I have one bttv using it. Not sure. The routine on em28xx is a fork of the original one, that was changed to work with the devices there. FYI, most of those I2C IR codes are provided by some generic 8-bits micro-processor, generally labeled with weird names, like KS007. The code inside those can be different, depending on the firmware inside, and also its I2C address. That's one of the reasons why we moved the code that used to be inside ir-i2c-kbd into the drivers that actually use it, like em28xx: this way, we can track its usage and fix, as the remaining get_key code inside-i2c-kbd are old, auto-detected, nobody knows precisely what devices use them, and the current developers don't own the hardware where they're used. In other words, please, don't touch at the get_key routines inside ir-kbd-i2c. If you find a bug, please fix at em28xx-input instead, if you find a bug.
Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab: > Em Fri, 28 Dec 2012 00:02:48 +0100 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> - return valid key code when button is hold >> - debug: print key code only when a button is pressed >> >> Tested with device "Terratec Cinergy 200 USB" (em28xx). >> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> >> --- >> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++---------- >> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) >> >> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c >> index 08ae067..2984b7d 100644 >> --- a/drivers/media/i2c/ir-kbd-i2c.c >> +++ b/drivers/media/i2c/ir-kbd-i2c.c >> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) >> return -EIO; >> } >> >> - /* it seems that 0xFE indicates that a button is still hold >> - down, while 0xff indicates that no button is hold >> - down. 0xfe sequences are sometimes interrupted by 0xFF */ >> - >> - dprintk(2,"key %02x\n", b); >> - >> - if (b == 0xff) >> + if (b == 0xff) /* no button */ >> return 0; >> >> - if (b == 0xfe) >> - /* keep old data */ >> - return 1; >> + if (b == 0xfe) /* button is still hold */ >> + b = ir->rc->last_scancode; /* keep old data */ >> + >> + dprintk(2,"key %02x\n", b); >> >> *ir_key = b; >> *ir_raw = b; > Don't do that. This piece of code is old, and it was added there > before the em28xx driver. Originally, the ir-i2c-kbd were used by > bttv and saa7134 drivers and the code there were auto-detecting the > I2C IR hardware decoding chips that used to be very common on media > devices. I'm almost sure that the original device that started using > this code is this model: > > drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS", > > That's why it is called as KNC1, but there are other cards that use > it as well. I think I have one bttv using it. Not sure. > > The routine on em28xx is a fork of the original one, that was changed > to work with the devices there. Indeed, it's a fork, 100% identical: static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) { unsigned char b; /* poll IR chip */ if (1 != i2c_master_recv(ir->c, &b, 1)) { i2cdprintk("read error\n"); return -EIO; } /* it seems that 0xFE indicates that a button is still hold down, while 0xff indicates that no button is hold down. 0xfe sequences are sometimes interrupted by 0xFF */ i2cdprintk("key %02x\n", b); if (b == 0xff) return 0; if (b == 0xfe) /* keep old data */ return 1; *ir_key = b; *ir_raw = b; return 1; } static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) { unsigned char b; /* poll IR chip */ if (1 != i2c_master_recv(ir->c, &b, 1)) { dprintk(1,"read error\n"); return -EIO; } /* it seems that 0xFE indicates that a button is still hold down, while 0xff indicates that no button is hold down. 0xfe sequences are sometimes interrupted by 0xFF */ dprintk(2,"key %02x\n", b); if (b == 0xff) return 0; if (b == 0xfe) /* keep old data */ return 1; *ir_key = b; *ir_raw = b; return 1; } Why should we keep two 100% identical functions ? See patch 4/6. I'm 99% sure that both devices are absolutely identical. Concerning the fix I'm suggesting here: First of all, I have to say that the Terratec RC works even without this patch. Nevertheless, I think the function should really return valid values for ir_key and ir_raw when 0xfe=button hold is received. Especially because the function succeeds. This also allows us to make u32 ir_key, ir_raw in ir_key_poll() in ir-kbd-i2c.c non-static. While I agree that we should be careful, I can't see how this can cause any trouble. The second thing is the small fix for the key code debug output. Don't you think it makes sense ? Regards, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 05 Jan 2013 14:32:30 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab: > > Em Fri, 28 Dec 2012 00:02:48 +0100 > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > > >> - return valid key code when button is hold > >> - debug: print key code only when a button is pressed > >> > >> Tested with device "Terratec Cinergy 200 USB" (em28xx). > >> > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > >> --- > >> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++---------- > >> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) > >> > >> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c > >> index 08ae067..2984b7d 100644 > >> --- a/drivers/media/i2c/ir-kbd-i2c.c > >> +++ b/drivers/media/i2c/ir-kbd-i2c.c > >> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) > >> return -EIO; > >> } > >> > >> - /* it seems that 0xFE indicates that a button is still hold > >> - down, while 0xff indicates that no button is hold > >> - down. 0xfe sequences are sometimes interrupted by 0xFF */ > >> - > >> - dprintk(2,"key %02x\n", b); > >> - > >> - if (b == 0xff) > >> + if (b == 0xff) /* no button */ > >> return 0; > >> > >> - if (b == 0xfe) > >> - /* keep old data */ > >> - return 1; > >> + if (b == 0xfe) /* button is still hold */ > >> + b = ir->rc->last_scancode; /* keep old data */ > >> + > >> + dprintk(2,"key %02x\n", b); > >> > >> *ir_key = b; > >> *ir_raw = b; > > Don't do that. This piece of code is old, and it was added there > > before the em28xx driver. Originally, the ir-i2c-kbd were used by > > bttv and saa7134 drivers and the code there were auto-detecting the > > I2C IR hardware decoding chips that used to be very common on media > > devices. I'm almost sure that the original device that started using > > this code is this model: > > > > drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS", > > > > That's why it is called as KNC1, but there are other cards that use > > it as well. I think I have one bttv using it. Not sure. > > > > The routine on em28xx is a fork of the original one, that was changed > > to work with the devices there. > > Indeed, it's a fork, 100% identical: > > > static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32 > *ir_raw) > { > unsigned char b; > > /* poll IR chip */ > if (1 != i2c_master_recv(ir->c, &b, 1)) { > i2cdprintk("read error\n"); > return -EIO; > } > > /* it seems that 0xFE indicates that a button is still hold > down, while 0xff indicates that no button is hold > down. 0xfe sequences are sometimes interrupted by 0xFF */ > > i2cdprintk("key %02x\n", b); > > if (b == 0xff) > return 0; > > if (b == 0xfe) > /* keep old data */ > return 1; > > *ir_key = b; > *ir_raw = b; > return 1; > } > > > > > static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) > { > unsigned char b; > > /* poll IR chip */ > if (1 != i2c_master_recv(ir->c, &b, 1)) { > dprintk(1,"read error\n"); > return -EIO; > } > > /* it seems that 0xFE indicates that a button is still hold > down, while 0xff indicates that no button is hold > down. 0xfe sequences are sometimes interrupted by 0xFF */ > > dprintk(2,"key %02x\n", b); > > if (b == 0xff) > return 0; > > if (b == 0xfe) > /* keep old data */ > return 1; > > *ir_key = b; > *ir_raw = b; > return 1; > } > > > > Why should we keep two 100% identical functions ? See patch 4/6. > I'm 99% sure that both devices are absolutely identical. 99% sure is not enough. A simple firmware difference at the microprocessor is enough to make the devices different. Also, this was widely discussed several years ago, when we decided to cleanup the I2C code. We then invested lot of efforts to move those get_keys away from ir-i2c-kbd. The only things left there are the ones we identified that were needed by auto-detection mode on old devices that we don't have. What was decided is to move everything that we know to the *-input driver, keeping there only the legacy stuff. > Concerning the fix I'm suggesting here: > First of all, I have to say that the Terratec RC works even without this > patch. > Nevertheless, I think the function should really return valid values for > ir_key and ir_raw when 0xfe=button hold is received. Especially because > the function succeeds. > This also allows us to make u32 ir_key, ir_raw in ir_key_poll() in > ir-kbd-i2c.c non-static. > While I agree that we should be careful, I can't see how this can cause > any trouble. Ok, the net effect is the same, except that the current way is faster, as it will skip some code that would simply put the value that it is already at ir_key/ir_raw again. As this polling code is known to cause performance loss on some machines, the quickest, the best. Also, the better is to report long press events on a different way, as the input core can handle those on a different way (that's why there are there keyup/keydown kABI calls). A patch for better handle rc=1 return code at ir-i2c-kbd is needed. In any case, I don't see any need for patches 4/6 or 6/6. > The second thing is the small fix for the key code debug output. Don't > you think it makes sense ? Now that we have "ir-keycode -t", all those key/scancode printk's inside the driver are pretty much useless, as both are reported as events. In the past, when most of the RC code were written, those prints were needed, as the scancode weren't reported to userspace. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 05.01.2013 16:25, schrieb Mauro Carvalho Chehab: > Em Sat, 05 Jan 2013 14:32:30 +0100 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab: >>> Em Fri, 28 Dec 2012 00:02:48 +0100 >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: >>> >>>> - return valid key code when button is hold >>>> - debug: print key code only when a button is pressed >>>> >>>> Tested with device "Terratec Cinergy 200 USB" (em28xx). >>>> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> >>>> --- >>>> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++---------- >>>> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) >>>> >>>> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c >>>> index 08ae067..2984b7d 100644 >>>> --- a/drivers/media/i2c/ir-kbd-i2c.c >>>> +++ b/drivers/media/i2c/ir-kbd-i2c.c >>>> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) >>>> return -EIO; >>>> } >>>> >>>> - /* it seems that 0xFE indicates that a button is still hold >>>> - down, while 0xff indicates that no button is hold >>>> - down. 0xfe sequences are sometimes interrupted by 0xFF */ >>>> - >>>> - dprintk(2,"key %02x\n", b); >>>> - >>>> - if (b == 0xff) >>>> + if (b == 0xff) /* no button */ >>>> return 0; >>>> >>>> - if (b == 0xfe) >>>> - /* keep old data */ >>>> - return 1; >>>> + if (b == 0xfe) /* button is still hold */ >>>> + b = ir->rc->last_scancode; /* keep old data */ >>>> + >>>> + dprintk(2,"key %02x\n", b); >>>> >>>> *ir_key = b; >>>> *ir_raw = b; >>> Don't do that. This piece of code is old, and it was added there >>> before the em28xx driver. Originally, the ir-i2c-kbd were used by >>> bttv and saa7134 drivers and the code there were auto-detecting the >>> I2C IR hardware decoding chips that used to be very common on media >>> devices. I'm almost sure that the original device that started using >>> this code is this model: >>> >>> drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS", >>> >>> That's why it is called as KNC1, but there are other cards that use >>> it as well. I think I have one bttv using it. Not sure. >>> >>> The routine on em28xx is a fork of the original one, that was changed >>> to work with the devices there. >> Indeed, it's a fork, 100% identical: >> >> >> static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32 >> *ir_raw) >> { >> unsigned char b; >> >> /* poll IR chip */ >> if (1 != i2c_master_recv(ir->c, &b, 1)) { >> i2cdprintk("read error\n"); >> return -EIO; >> } >> >> /* it seems that 0xFE indicates that a button is still hold >> down, while 0xff indicates that no button is hold >> down. 0xfe sequences are sometimes interrupted by 0xFF */ >> >> i2cdprintk("key %02x\n", b); >> >> if (b == 0xff) >> return 0; >> >> if (b == 0xfe) >> /* keep old data */ >> return 1; >> >> *ir_key = b; >> *ir_raw = b; >> return 1; >> } >> >> >> >> >> static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) >> { >> unsigned char b; >> >> /* poll IR chip */ >> if (1 != i2c_master_recv(ir->c, &b, 1)) { >> dprintk(1,"read error\n"); >> return -EIO; >> } >> >> /* it seems that 0xFE indicates that a button is still hold >> down, while 0xff indicates that no button is hold >> down. 0xfe sequences are sometimes interrupted by 0xFF */ >> >> dprintk(2,"key %02x\n", b); >> >> if (b == 0xff) >> return 0; >> >> if (b == 0xfe) >> /* keep old data */ >> return 1; >> >> *ir_key = b; >> *ir_raw = b; >> return 1; >> } >> >> >> >> Why should we keep two 100% identical functions ? See patch 4/6. >> I'm 99% sure that both devices are absolutely identical. > 99% sure is not enough. A simple firmware difference at the microprocessor > is enough to make the devices different. I agree, but that's irrelevant. What counts is that the _code_ ist 100% identical. > Also, this was widely discussed several years ago, when we decided to cleanup > the I2C code. We then invested lot of efforts to move those get_keys away > from ir-i2c-kbd. The only things left there are the ones we identified that > were needed by auto-detection mode on old devices that we don't have. > > What was decided is to move everything that we know to the *-input driver, > keeping there only the legacy stuff. Uhm... ok. My assumption was, that the goal is the opposite (move as much common code as possible to i2c-ir-kbd). I'm a bit puzzled about this decision... Okay.... but then... why do we still use ir-kbd-i2c in em28xx ? We can easily get rid of it. Everything we need is already on board. I can send a patch if you want. > >> Concerning the fix I'm suggesting here: >> First of all, I have to say that the Terratec RC works even without this >> patch. >> Nevertheless, I think the function should really return valid values for >> ir_key and ir_raw when 0xfe=button hold is received. Especially because >> the function succeeds. >> This also allows us to make u32 ir_key, ir_raw in ir_key_poll() in >> ir-kbd-i2c.c non-static. >> While I agree that we should be careful, I can't see how this can cause >> any trouble. > Ok, the net effect is the same, except that the current way is faster, as it > will skip some code that would simply put the value that it is already at > ir_key/ir_raw again. Faster... ok :) How much ? ;) I would say its ugly coding. And a potential source of a regression. > As this polling code is known to cause performance loss on some machines, > the quickest, the best. Also, the better is to report long press events > on a different way, as the input core can handle those on a different way > (that's why there are there keyup/keydown kABI calls). A patch for better > handle rc=1 return code at ir-i2c-kbd is needed. Sounds good. > In any case, I don't see any need for patches 4/6 or 6/6. > >> The second thing is the small fix for the key code debug output. Don't >> you think it makes sense ? > Now that we have "ir-keycode -t", all those key/scancode printk's inside > the driver are pretty much useless, as both are reported as events. > > In the past, when most of the RC code were written, those prints were Then we should remove them. Regards, Frank > needed, as the scancode weren't reported to userspace. > > Regards, > Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sun, 06 Jan 2013 21:32:31 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > Am 05.01.2013 16:25, schrieb Mauro Carvalho Chehab: > > Em Sat, 05 Jan 2013 14:32:30 +0100 > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > > >> Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab: > >>> Em Fri, 28 Dec 2012 00:02:48 +0100 > >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >>> > >>>> - return valid key code when button is hold > >>>> - debug: print key code only when a button is pressed > >>>> > >>>> Tested with device "Terratec Cinergy 200 USB" (em28xx). > >>>> > >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > >>>> --- > >>>> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++---------- > >>>> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) > >>>> > >>>> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c > >>>> index 08ae067..2984b7d 100644 > >>>> --- a/drivers/media/i2c/ir-kbd-i2c.c > >>>> +++ b/drivers/media/i2c/ir-kbd-i2c.c > >>>> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) > >>>> return -EIO; > >>>> } > >>>> > >>>> - /* it seems that 0xFE indicates that a button is still hold > >>>> - down, while 0xff indicates that no button is hold > >>>> - down. 0xfe sequences are sometimes interrupted by 0xFF */ > >>>> - > >>>> - dprintk(2,"key %02x\n", b); > >>>> - > >>>> - if (b == 0xff) > >>>> + if (b == 0xff) /* no button */ > >>>> return 0; > >>>> > >>>> - if (b == 0xfe) > >>>> - /* keep old data */ > >>>> - return 1; > >>>> + if (b == 0xfe) /* button is still hold */ > >>>> + b = ir->rc->last_scancode; /* keep old data */ > >>>> + > >>>> + dprintk(2,"key %02x\n", b); > >>>> > >>>> *ir_key = b; > >>>> *ir_raw = b; > >>> Don't do that. This piece of code is old, and it was added there > >>> before the em28xx driver. Originally, the ir-i2c-kbd were used by > >>> bttv and saa7134 drivers and the code there were auto-detecting the > >>> I2C IR hardware decoding chips that used to be very common on media > >>> devices. I'm almost sure that the original device that started using > >>> this code is this model: > >>> > >>> drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS", > >>> > >>> That's why it is called as KNC1, but there are other cards that use > >>> it as well. I think I have one bttv using it. Not sure. > >>> > >>> The routine on em28xx is a fork of the original one, that was changed > >>> to work with the devices there. > >> Indeed, it's a fork, 100% identical: > >> > >> > >> static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32 > >> *ir_raw) > >> { > >> unsigned char b; > >> > >> /* poll IR chip */ > >> if (1 != i2c_master_recv(ir->c, &b, 1)) { > >> i2cdprintk("read error\n"); > >> return -EIO; > >> } > >> > >> /* it seems that 0xFE indicates that a button is still hold > >> down, while 0xff indicates that no button is hold > >> down. 0xfe sequences are sometimes interrupted by 0xFF */ > >> > >> i2cdprintk("key %02x\n", b); > >> > >> if (b == 0xff) > >> return 0; > >> > >> if (b == 0xfe) > >> /* keep old data */ > >> return 1; > >> > >> *ir_key = b; > >> *ir_raw = b; > >> return 1; > >> } > >> > >> > >> > >> > >> static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) > >> { > >> unsigned char b; > >> > >> /* poll IR chip */ > >> if (1 != i2c_master_recv(ir->c, &b, 1)) { > >> dprintk(1,"read error\n"); > >> return -EIO; > >> } > >> > >> /* it seems that 0xFE indicates that a button is still hold > >> down, while 0xff indicates that no button is hold > >> down. 0xfe sequences are sometimes interrupted by 0xFF */ > >> > >> dprintk(2,"key %02x\n", b); > >> > >> if (b == 0xff) > >> return 0; > >> > >> if (b == 0xfe) > >> /* keep old data */ > >> return 1; > >> > >> *ir_key = b; > >> *ir_raw = b; > >> return 1; > >> } > >> > >> > >> > >> Why should we keep two 100% identical functions ? See patch 4/6. > >> I'm 99% sure that both devices are absolutely identical. > > 99% sure is not enough. A simple firmware difference at the microprocessor > > is enough to make the devices different. > > I agree, but that's irrelevant. What counts is that the _code_ ist 100% > identical. > > > Also, this was widely discussed several years ago, when we decided to cleanup > > the I2C code. We then invested lot of efforts to move those get_keys away > > from ir-i2c-kbd. The only things left there are the ones we identified that > > were needed by auto-detection mode on old devices that we don't have. > > > > What was decided is to move everything that we know to the *-input driver, > > keeping there only the legacy stuff. > > Uhm... ok. > My assumption was, that the goal is the opposite (move as much common > code as possible to i2c-ir-kbd). > I'm a bit puzzled about this decision... > > Okay.... but then... why do we still use ir-kbd-i2c in em28xx ? > We can easily get rid of it. Everything we need is already on board. > > I can send a patch if you want. No. We don't want to mix I2C client code inside the I2C bus drivers. If we had started to code it right now, we would likely have used a different approach for those I2C IR's, but it is not valuable enough to change it right now, as it works, and there's nothing new happening here. The RC trend is to not use hardware decoding anymore, as this doesn't follow Microsoft's MCE architecture. All newer chipsets I'm aware of just sends a sequence of pulse/space duration, and let the kernel to decode. Empia seems to be late on following this market trend. Even so, I don't see any new board design with an IR I2C hardware for years. So, the better is to just not re-engineer the things that are working, in order to avoid breaking them. > > In any case, I don't see any need for patches 4/6 or 6/6. > > > >> The second thing is the small fix for the key code debug output. Don't > >> you think it makes sense ? > > Now that we have "ir-keycode -t", all those key/scancode printk's inside > > the driver are pretty much useless, as both are reported as events. > > > > In the past, when most of the RC code were written, those prints were > > Then we should remove them. That makes sense on my eyes. Yet, writing/reviewing such cleanup patches would have a very low priority. Btw, all drivers have a lot of printk's message from the time they were written that can be cleaned up, especially nowadays, as several of those printk's can be replaced by Kernel tracing facilities. If one is willing to do it, I expect that it would be reviewing all of them and not just those ones.
Am 07.01.2013 17:40, schrieb Mauro Carvalho Chehab: >>> Also, this was widely discussed several years ago, when we decided to cleanup >>> the I2C code. We then invested lot of efforts to move those get_keys away >>> from ir-i2c-kbd. The only things left there are the ones we identified that >>> were needed by auto-detection mode on old devices that we don't have. >>> >>> What was decided is to move everything that we know to the *-input driver, >>> keeping there only the legacy stuff. >> Uhm... ok. >> My assumption was, that the goal is the opposite (move as much common >> code as possible to i2c-ir-kbd). >> I'm a bit puzzled about this decision... >> >> Okay.... but then... why do we still use ir-kbd-i2c in em28xx ? >> We can easily get rid of it. Everything we need is already on board. >> >> I can send a patch if you want. > No. We don't want to mix I2C client code inside the I2C bus drivers. Well, you can't have both at the same time. :D Either do it in a separate module (ir-kbd-i2c) or in the em28xx driver !? ;) > If we had started to code it right now, we would likely have used a > different approach for those I2C IR's, but it is not valuable enough > to change it right now, as it works, and there's nothing new happening > here. > > The RC trend is to not use hardware decoding anymore, as this doesn't > follow Microsoft's MCE architecture. All newer chipsets I'm aware of > just sends a sequence of pulse/space duration, and let the kernel to > decode. Empia seems to be late on following this market trend. Even so, > I don't see any new board design with an IR I2C hardware for years. True, but doesn't change the fact that the current code can be improved. I think it's valuable enough, as we can get rid of a module dependency here. > So, the better is to just not re-engineer the things that are working, > in order to avoid breaking them. Yeah, we should _always_ be careful. But it's definitely not re-engineering. The key polling and decoding functions are already there. The main change is, that em28xx-input would call them itself instead of ir-kbd-i2c, which is what it already does with the built-in decoders. For maximum safety, we can use the same key handling function as ir-kbd-i2c. If we don't do it now (yet we have devices for testing), it will likely never happen. > >>> In any case, I don't see any need for patches 4/6 or 6/6. >>> >>>> The second thing is the small fix for the key code debug output. Don't >>>> you think it makes sense ? >>> Now that we have "ir-keycode -t", all those key/scancode printk's inside >>> the driver are pretty much useless, as both are reported as events. >>> >>> In the past, when most of the RC code were written, those prints were >> Then we should remove them. > That makes sense on my eyes. > > Yet, writing/reviewing such cleanup patches would have a very low priority. > Btw, all drivers have a lot of printk's message from the time they were > written that can be cleaned up, especially nowadays, as several of those > printk's can be replaced by Kernel tracing facilities. > > If one is willing to do it, I expect that it would be reviewing all > of them and not just those ones. Sure. Regards, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c index 08ae067..2984b7d 100644 --- a/drivers/media/i2c/ir-kbd-i2c.c +++ b/drivers/media/i2c/ir-kbd-i2c.c @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw) return -EIO; } - /* it seems that 0xFE indicates that a button is still hold - down, while 0xff indicates that no button is hold - down. 0xfe sequences are sometimes interrupted by 0xFF */ - - dprintk(2,"key %02x\n", b); - - if (b == 0xff) + if (b == 0xff) /* no button */ return 0; - if (b == 0xfe) - /* keep old data */ - return 1; + if (b == 0xfe) /* button is still hold */ + b = ir->rc->last_scancode; /* keep old data */ + + dprintk(2,"key %02x\n", b); *ir_key = b; *ir_raw = b;
- return valid key code when button is hold - debug: print key code only when a button is pressed Tested with device "Terratec Cinergy 200 USB" (em28xx). Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> --- drivers/media/i2c/ir-kbd-i2c.c | 15 +++++---------- 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)