media: rc: Always report LIRC repeat flag

  The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
Previously it was only set by rc_repeat(), but not all protocol
decoders invoke that function.

Signed-off-by: Marko Mäkelä <marko.makela@iki.fi>
---
 drivers/media/rc/rc-main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Hi Marko,

On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
> The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
> Previously it was only set by rc_repeat(), but not all protocol
> decoders invoke that function.

This should say _why_ you are making this change, not _what_ the change
is. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

This also applies the first line of the commit.

Thanks
Sean

> 
> Signed-off-by: Marko Mäkelä <marko.makela@iki.fi>
> ---
>  drivers/media/rc/rc-main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index b90438a71c80..d914197245eb 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
>  			  dev->last_toggle   != toggle);
>  	struct lirc_scancode sc = {
>  		.scancode = scancode, .rc_proto = protocol,
> -		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
> +		.flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
> +		(!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
>  		.keycode = keycode
>  	};
>  
> -- 
> 2.36.1
  

Hi Sean,

Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
>On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
>> The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
>> Previously it was only set by rc_repeat(), but not all protocol
>> decoders invoke that function.
>
>This should say _why_ you are making this change, not _what_ the change
>is.

How would you find the following?

---
media: lirc: ensure lirc device receives repeats

Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in 
the LIRC messages.

Commit b66218fddfd29f315a103db811152ab0c95fb054
("media: lirc: ensure lirc device receives nec repeats") fixed it up for
those protocol drivers that may call rc_repeat().
---

Would you prefer to be mentioned as a co-developer?

Best regards,

	Marko
  

Hi Marko,

On Wed, Jul 06, 2022 at 07:39:17PM +0300, Marko Mäkelä wrote:
> Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
> > On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
> > > The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
> > > Previously it was only set by rc_repeat(), but not all protocol
> > > decoders invoke that function.
> > 
> > This should say _why_ you are making this change, not _what_ the change
> > is.
> 
> How would you find the following?
> 
> ---
> media: lirc: ensure lirc device receives repeats
> 
> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in the
> LIRC messages.
> 
> Commit b66218fddfd29f315a103db811152ab0c95fb054
> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> those protocol drivers that may call rc_repeat().
> ---

That's no good. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

The heading is called "Describe your changes".


Sean
  

Hi Sean,

Thu, Jul 07, 2022 at 09:57:10AM +0100, Sean Young wrote:
>Hi Marko,
>
>On Wed, Jul 06, 2022 at 07:39:17PM +0300, Marko Mäkelä wrote:
>> Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
>> > On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
>> > > The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
>> > > Previously it was only set by rc_repeat(), but not all protocol
>> > > decoders invoke that function.
>> >
>> > This should say _why_ you are making this change, not _what_ the change
>> > is.
>>
>> How would you find the following?
>>
>> ---
>> media: lirc: ensure lirc device receives repeats
>>
>> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
>> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in the
>> LIRC messages.
>>
>> Commit b66218fddfd29f315a103db811152ab0c95fb054
>> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
>> those protocol drivers that may call rc_repeat().
>> ---
>
>That's no good. See:
>
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
>The heading is called "Describe your changes".

I see. A quick read of "git log --oneline drivers/media/rc" suggests 
that the first line of the commit message is expected to be a summary 
_what_ the change is, not _why_ it was made. Would the commit message be 
acceptable after adding a "why" part right after the heading line, like 
this? If not, I would appreciate specific suggestions.

---
media: lirc: ensure lirc device receives repeats

For remote controls using RC5 and similar protocols that include a
"toggle" flag, the LIRC device never set the "repeat" flag to distinguish
repeated messages that were sent several times per second due to a
long keypress, and messages sent due to repeated short keypresses.

While a user-space program may implement logic around the "toggle" flag
to distinguish long keypresses, it would be simpler to be able to rely 
on the "repeat" flag for any type of protocol.

Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in
the LIRC messages.

Commit b66218fddfd29f315a103db811152ab0c95fb054
("media: lirc: ensure lirc device receives nec repeats") fixed it up for
those protocol drivers that may call rc_repeat().
---

Best regards,

	Marko
  

Hi Marko,

On Thu, Jul 07, 2022 at 02:09:49PM +0300, Marko Mäkelä wrote:
> Thu, Jul 07, 2022 at 09:57:10AM +0100, Sean Young wrote:
> > On Wed, Jul 06, 2022 at 07:39:17PM +0300, Marko Mäkelä wrote:
> > > Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
> > > > On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
> > > > > The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
> > > > > Previously it was only set by rc_repeat(), but not all protocol
> > > > > decoders invoke that function.
> > > >
> > > > This should say _why_ you are making this change, not _what_ the change
> > > > is.
> > > 
> > > How would you find the following?
> > > 
> > > ---
> > > media: lirc: ensure lirc device receives repeats
> > > 
> > > Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> > > reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in the
> > > LIRC messages.
> > > 
> > > Commit b66218fddfd29f315a103db811152ab0c95fb054
> > > ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> > > those protocol drivers that may call rc_repeat().
> > > ---
> > 
> > That's no good. See:
> > 
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> > 
> > The heading is called "Describe your changes".
> 
> I see. A quick read of "git log --oneline drivers/media/rc" suggests that
> the first line of the commit message is expected to be a summary _what_ the
> change is, not _why_ it was made. Would the commit message be acceptable
> after adding a "why" part right after the heading line, like this? If not, I
> would appreciate specific suggestions.

This is much better, thank you.

> ---
> media: lirc: ensure lirc device receives repeats
> 
> For remote controls using RC5 and similar protocols that include a
> "toggle" flag, the LIRC device never set the "repeat" flag to distinguish
> repeated messages that were sent several times per second due to a
> long keypress, and messages sent due to repeated short keypresses.
> 
> While a user-space program may implement logic around the "toggle" flag
> to distinguish long keypresses, it would be simpler to be able to rely on
> the "repeat" flag for any type of protocol.

I'm not sure how relevant the toggle is. This change is relevant for all 
protocols that do not use rc_repeat() and simply repeat the original
message when a key is being held down. This includes the sony protocol, 
imon, and the nec protocol (in case the remote does *not* use the repeat
message).

> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in
> the LIRC messages.
> 
> Commit b66218fddfd29f315a103db811152ab0c95fb054
> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> those protocol drivers that may call rc_repeat().
> ---

Thanks

Sean