Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver

["Marko Mäkelä" <marko.makela@iki.fi>]

Sat, Jul 02, 2022 at 09:14:59AM +0100, Sean Young wrote:
>Hi,
>
>On Tue, Jun 28, 2022 at 09:27:26AM +0300, Marko Mäkelä wrote:
>> Mon, Jun 27, 2022 at 11:53:52AM +0100, Sean Young wrote:
>> > Hi Marko,
>> >
>> > On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote:
>> > > I finally took the time to get a deeper understanding of the infrared remote
>> > > control subsystem. I think that I now understand the translation into
>> > > key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat()
>> > > will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will
>> > > handle the repeat. For lirc_scancode_event() it will never set the
>> > > LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does
>> > > support the toggle bit. That might qualify as a bug.
>> >
>> > You are right, this was missed. Patches welcome.
>>
>> Attached (for 5.19.0-rc3, on top of the two commits of this patch series).
>>
>> I thought that it would be the least amount of trouble to slightly change
>> the interpretation of the "toggle" parameter of
>> rc_keydown(). My intention was to use the values 1 and 2 when the toggle
>> flag is present. Any nonzero values would work.
>
>I don't understand why this is needed.

For protocols that do not use a toggle bit, the last parameter of 
rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() 
will be issued when needed. For those protocols, I thought that we would 
not want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under 
any circumstances.

>A patch needs to be tested. Just rebuild the entire kernel and boot 
>from that.

Yes, I will do that for revising my patch.

>> -	rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle);
>> +	rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key),
>> +		   1 + toggle);
>
>You can't change the toggle value because you want a repeat flag. This makes
>no sense.
[snip]
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
>>  {
>>  	bool new_event = (!dev->keypressed		 ||
>>  			  dev->last_protocol != protocol ||
>> -			  dev->last_scancode != scancode ||
>> -			  dev->last_toggle   != toggle);
>> +			  dev->last_scancode != scancode);
>> +	bool repeat_event = !new_event && toggle && dev->last_toggle == toggle;
>
>Why this change?
>
>>  	struct lirc_scancode sc = {
>>  		.scancode = scancode, .rc_proto = protocol,
>> -		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
>> +		.flags = (toggle > 1 ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
>> +			 (repeat_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
>
>Why not simply (!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0) and be done with it?

Drivers that invoke rc_repeat() do not want rc_keydown() to ever set 
LIRC_SCANCODE_FLAG_REPEAT. The patch slightly changed the meaning of 
toggle: it *must* be 0 if and only if the protocol does not implement a 
toggle bit. If it does, the values must alternate between 1 and some 
greater-than-1 value.

A cleaner alternative could be to retain the interface of rc_keydown() 
as is, and add a new function, say, rc_keydown_or_repeat(), which would 
generate key-repeat events from the toggle bit.

Best regards,

	Marko