Re: [GIT PULL FOR v4.17] rc changes

[Sean Young]

On Thu, Feb 15, 2018 at 09:16:18AM -0200, Mauro Carvalho Chehab wrote:
> Em Wed, 14 Feb 2018 21:32:07 +
> Sean Young  escreveu:
> 
> > Hi Mauro,
> > 
> > On Wed, Feb 14, 2018 at 04:44:48PM -0200, Mauro Carvalho Chehab wrote:
> > > Hi Sean,
> > > 
> > > Em Mon, 12 Feb 2018 20:03:18 +
> > > Sean Young  escreveu:
> > >   
> > > > Hi Mauro,
> > > > 
> > > > Just very minor changes this time (other stuff is not ready yet). I 
> > > > would
> > > > really appreciate if you could cast an extra critical eye on the commit 
> > > > "no need to check for transitions", just to be sure it is the right 
> > > > change.  
> > > 
> > > Did you send all patches in separate? This is important to allow us
> > > to comment on an specific issue inside a patch...  
> > 
> > All the patches were emailed to linux-media, some of them on the same day
> > as the pull request. Maybe I should wait longer. The patch below was sent
> > out on the 28th of January.
> > 
> > > >   media: rc: no need to check for transitions  
> 
> No need to wait longer. Yet, it seems that I lost the above patch, as I
> couldn't find anything on my email with the above subject.
> 
> Perhaps the e-mail got lost somehow on my inbox.

Actually, this is my bad. I changed the subject line of the commit after
sending it to the list, without resending it. I should have sent out a v2.

> > > I don't remember the exact reason for that, but, as far as I
> > > remember, on a few devices, a pulse (or space) event could be
> > > broken into two consecutive events of the same type, e. g.,
> > > a pulse with a 125 ms could be broken into two pulses, like
> > > one with 100 ms and the other with 25 ms.  
> > 
> > If that is the case, then the IR decoders could not deal with this anyway.
> > For example, the first state transition rc6 is:
> > 
> > if (!eq_margin(ev.duration, RC6_PREFIX_PULSE, RC6_UNIT))
> > 
> > So if ev.duration is not the complete duration, then decoding will fail;
> > I tried to explain in the commit message that if this was the case, then
> > decoding would not work so the check was unnecessary.
> > 
> > > That's said, I'm not sure if the current implementation are
> > > adding the timings for both pulses into a single one.  
> > 
> > That depends on whether the driver uses ir_raw_event_store() or
> > ir_raw_event_store_with_filter(). The latter exists precisely for this
> > reason.
> 
> OK.
> 
> > 
> > > For now, I'll keep this patch out of the merge.  
> > 
> > Ok. So in summary, I think:
> > 
> > 1. Any driver which produces consequentive pulse events is broken
> >and should be fixed;
> 
> Agreed.
> 
> > 2. The IR decoders cannot deal with consequentive pulses and the current
> >prev_ev code does not help with this (possibly in very special
> >cases).
> 
> Ok. Yet, maybe it would worth to produce a warning if this happen, and
> reset the state machine, as it would help to identify problems at
> the driver or at the hardware.

That's not a bad idea. I will look a this.

Thanks
Sean

Re: [GIT PULL FOR v4.17] rc changes

[Mauro Carvalho Chehab]

Em Wed, 14 Feb 2018 21:32:07 +
Sean Young  escreveu:

> Hi Mauro,
> 
> On Wed, Feb 14, 2018 at 04:44:48PM -0200, Mauro Carvalho Chehab wrote:
> > Hi Sean,
> > 
> > Em Mon, 12 Feb 2018 20:03:18 +
> > Sean Young  escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > Just very minor changes this time (other stuff is not ready yet). I would
> > > really appreciate if you could cast an extra critical eye on the commit 
> > > "no need to check for transitions", just to be sure it is the right 
> > > change.  
> > 
> > Did you send all patches in separate? This is important to allow us
> > to comment on an specific issue inside a patch...  
> 
> All the patches were emailed to linux-media, some of them on the same day
> as the pull request. Maybe I should wait longer. The patch below was sent
> out on the 28th of January.
> 
> > >   media: rc: no need to check for transitions  

No need to wait longer. Yet, it seems that I lost the above patch, as I
couldn't find anything on my email with the above subject.

Perhaps the e-mail got lost somehow on my inbox.

> > 
> > I don't remember the exact reason for that, but, as far as I
> > remember, on a few devices, a pulse (or space) event could be
> > broken into two consecutive events of the same type, e. g.,
> > a pulse with a 125 ms could be broken into two pulses, like
> > one with 100 ms and the other with 25 ms.  
> 
> If that is the case, then the IR decoders could not deal with this anyway.
> For example, the first state transition rc6 is:
> 
>   if (!eq_margin(ev.duration, RC6_PREFIX_PULSE, RC6_UNIT))
> 
> So if ev.duration is not the complete duration, then decoding will fail;
> I tried to explain in the commit message that if this was the case, then
> decoding would not work so the check was unnecessary.
> 
> > That's said, I'm not sure if the current implementation are
> > adding the timings for both pulses into a single one.  
> 
> That depends on whether the driver uses ir_raw_event_store() or
> ir_raw_event_store_with_filter(). The latter exists precisely for this
> reason.

OK.

> 
> > For now, I'll keep this patch out of the merge.  
> 
> Ok. So in summary, I think:
> 
> 1. Any driver which produces consequentive pulse events is broken
>and should be fixed;

Agreed.

> 2. The IR decoders cannot deal with consequentive pulses and the current
>prev_ev code does not help with this (possibly in very special
>cases).

Ok. Yet, maybe it would worth to produce a warning if this happen, and
reset the state machine, as it would help to identify problems at
the driver or at the hardware.


Thanks,
Mauro

Re: [GIT PULL FOR v4.17] rc changes

[Sean Young]

On Wed, Feb 14, 2018 at 04:49:08PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 12 Feb 2018 20:03:18 +
> Sean Young  escreveu:
> 
> >   media: rc: unnecessary use of do_div
> > 
> > From c52920c524d96db55b9b82440504a7ec40df0b32 Mon Sep 17 00:00:00 2001
> > From: Sean Young 
> > Date: Sun, 11 Feb 2018 17:23:14 +
> > Subject: media: rc: unnecessary use of do_div
> > Cc: Linux Media Mailing List ,
> > Mauro Carvalho Chehab 
> > 
> > No need to use do_div() when the remainder is thrown away.
> 
> That's not true at all! We need do_div() every time we're dividing an u64
> number, as otherwise, it will cause link errors when built with 32 bits.

I completely missed that. Thank you!


Sean

Re: [GIT PULL FOR v4.17] rc changes

[Sean Young]

Hi Mauro,

On Wed, Feb 14, 2018 at 04:44:48PM -0200, Mauro Carvalho Chehab wrote:
> Hi Sean,
> 
> Em Mon, 12 Feb 2018 20:03:18 +
> Sean Young  escreveu:
> 
> > Hi Mauro,
> > 
> > Just very minor changes this time (other stuff is not ready yet). I would
> > really appreciate if you could cast an extra critical eye on the commit 
> > "no need to check for transitions", just to be sure it is the right change.
> 
> Did you send all patches in separate? This is important to allow us
> to comment on an specific issue inside a patch...

All the patches were emailed to linux-media, some of them on the same day
as the pull request. Maybe I should wait longer. The patch below was sent
out on the 28th of January.

> >   media: rc: no need to check for transitions
> 
> I don't remember the exact reason for that, but, as far as I
> remember, on a few devices, a pulse (or space) event could be
> broken into two consecutive events of the same type, e. g.,
> a pulse with a 125 ms could be broken into two pulses, like
> one with 100 ms and the other with 25 ms.

If that is the case, then the IR decoders could not deal with this anyway.
For example, the first state transition rc6 is:

if (!eq_margin(ev.duration, RC6_PREFIX_PULSE, RC6_UNIT))

So if ev.duration is not the complete duration, then decoding will fail;
I tried to explain in the commit message that if this was the case, then
decoding would not work so the check was unnecessary.

> That's said, I'm not sure if the current implementation are
> adding the timings for both pulses into a single one.

That depends on whether the driver uses ir_raw_event_store() or
ir_raw_event_store_with_filter(). The latter exists precisely for this
reason.

> For now, I'll keep this patch out of the merge.

Ok. So in summary, I think:

1. Any driver which produces consequentive pulse events is broken
   and should be fixed;
2. The IR decoders cannot deal with consequentive pulses and the current
   prev_ev code does not help with this (possibly in very special
   cases).


Sean

Re: [GIT PULL FOR v4.17] rc changes

[Mauro Carvalho Chehab]

Em Mon, 12 Feb 2018 20:03:18 +
Sean Young  escreveu:

>   media: rc: unnecessary use of do_div
> 
> From c52920c524d96db55b9b82440504a7ec40df0b32 Mon Sep 17 00:00:00 2001
> From: Sean Young 
> Date: Sun, 11 Feb 2018 17:23:14 +
> Subject: media: rc: unnecessary use of do_div
> Cc: Linux Media Mailing List ,
> Mauro Carvalho Chehab 
> 
> No need to use do_div() when the remainder is thrown away.

That's not true at all! We need do_div() every time we're dividing an u64
number, as otherwise, it will cause link errors when built with 32 bits.



Thanks,
Mauro

Re: [GIT PULL FOR v4.17] rc changes

[Mauro Carvalho Chehab]

Hi Sean,

Em Mon, 12 Feb 2018 20:03:18 +
Sean Young  escreveu:

> Hi Mauro,
> 
> Just very minor changes this time (other stuff is not ready yet). I would
> really appreciate if you could cast an extra critical eye on the commit 
> "no need to check for transitions", just to be sure it is the right change.

Did you send all patches in separate? This is important to allow us
to comment on an specific issue inside a patch...

>   media: rc: no need to check for transitions

I don't remember the exact reason for that, but, as far as I
remember, on a few devices, a pulse (or space) event could be
broken into two consecutive events of the same type, e. g.,
a pulse with a 125 ms could be broken into two pulses, like
one with 100 ms and the other with 25 ms.

That's said, I'm not sure if the current implementation are
adding the timings for both pulses into a single one.

For now, I'll keep this patch out of the merge.

Thanks,
Mauro

[GIT PULL FOR v4.17] rc changes

[Sean Young]

Hi Mauro,

Just very minor changes this time (other stuff is not ready yet). I would
really appreciate if you could cast an extra critical eye on the commit 
"no need to check for transitions", just to be sure it is the right change.

Thanks,

Sean

The following changes since commit 61605f79d576b0c2bea98fd0d1b2b3eeebb682f0:

  Merge tag 'v4.16-rc1' into patchwork (2018-02-12 07:52:06 -0500)

are available in the Git repository at:

  git://linuxtv.org/syoung/media_tree.git for-v4.17a

for you to fetch changes up to 42ed0e7cd25fe42cf2ba16b0962fe018676b4da6:

  media: rc: get start time just before calling driver tx (2018-02-12 14:56:59 
+)


Alexey Khoroshilov (1):
  media: rc: ir-hix5hd2: fix error handling of clk_prepare_enable()

Andi Kleen (1):
  media: rc: don't mark IR decoders default y

Sean Young (8):
  media: rc: ir-spi: fix duty cycle
  media: rc: no need to check for transitions
  media: rc: unnecessary use of do_div
  media: rc: replace IR_dprintk() with dev_dbg in IR decoders
  media: rc: remove IR_dprintk() from rc-core
  media: rc: remove obsolete comment
  media: rc: remove useless if statement
  media: rc: get start time just before calling driver tx

 drivers/media/rc/Kconfig  | 11 -
 drivers/media/rc/ir-hix5hd2.c | 35 +++---
 drivers/media/rc/ir-jvc-decoder.c | 14 +++---
 drivers/media/rc/ir-mce_kbd-decoder.c | 66 -
 drivers/media/rc/ir-nec-decoder.c | 20 
 drivers/media/rc/ir-rc5-decoder.c | 15 +++---
 drivers/media/rc/ir-rc6-decoder.c | 35 ++
 drivers/media/rc/ir-sanyo-decoder.c   | 18 +++
 drivers/media/rc/ir-sharp-decoder.c   | 17 +++
 drivers/media/rc/ir-sony-decoder.c| 14 +++---
 drivers/media/rc/ir-spi.c | 24 +
 drivers/media/rc/ir-xmp-decoder.c | 29 +--
 drivers/media/rc/lirc_dev.c   | 24 +
 drivers/media/rc/rc-core-priv.h   | 13 -
 drivers/media/rc/rc-ir-raw.c  |  7 ++-
 drivers/media/rc/rc-main.c| 91 +--
 include/media/rc-core.h   |  7 ---
 17 files changed, 196 insertions(+), 244 deletions(-)