Am 05.01.2013 16:25, schrieb Mauro Carvalho Chehab:
> Em Sat, 05 Jan 2013 14:32:30 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> 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@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.
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
