Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab:
> Em Fri, 28 Dec 2012 00:02:48 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
