| Message ID | 20220705085358.44418-1-marko.makela@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rc: Always report LIRC repeat flag | expand |
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
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 };
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(-)