nuvoton-cir: Add support for user configurable wake-up codes

Message ID 1388758723-21653-1-git-send-email-a.seppala@gmail.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Antti Seppälä Jan. 3, 2014, 2:18 p.m. UTC
  This patch introduces module parameters for setting wake-up codes to be
programmed into the hardware FIFO. This allows users to provide custom
IR sample sequences to trigger system wake-up from sleep states.

Usage:
   modprobe nuvoton-cir wake_samples=0x90,0x11,0xa1... (up to 67 bytes)

Here is a summary of module parameters introduced by this patch:
 * wake_samples: FIFO values to compare against received IR codes when
   in sleep state.

 * cmp_deep: Number of bytes the hardware will compare against. This is
   currently autodetected by the driver.

 * cmp_tolerance: The maximum allowed value difference between a single
   wake-up byte and a sample read from IR to be still considered a match.
   Default is 5.

Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
---
 drivers/media/rc/nuvoton-cir.c | 65 +++++++++++++++++++++++++++++++-----------
 drivers/media/rc/nuvoton-cir.h |  6 ++--
 2 files changed, 51 insertions(+), 20 deletions(-)
  

Comments

Mauro Carvalho Chehab Jan. 15, 2014, 7:35 p.m. UTC | #1
Em Fri,  3 Jan 2014 16:18:43 +0200
Antti Seppälä <a.seppala@gmail.com> escreveu:

> This patch introduces module parameters for setting wake-up codes to be
> programmed into the hardware FIFO. This allows users to provide custom
> IR sample sequences to trigger system wake-up from sleep states.
> 
> Usage:
>    modprobe nuvoton-cir wake_samples=0x90,0x11,0xa1... (up to 67 bytes)
> 
> Here is a summary of module parameters introduced by this patch:
>  * wake_samples: FIFO values to compare against received IR codes when
>    in sleep state.
> 
>  * cmp_deep: Number of bytes the hardware will compare against. This is
>    currently autodetected by the driver.
> 
>  * cmp_tolerance: The maximum allowed value difference between a single
>    wake-up byte and a sample read from IR to be still considered a match.
>    Default is 5.

Instead of adding it as a modprobe parameter, the better is to create
an special sysfs node for devices that support wake up.

Btw, there's another device driver also requiring a similar feature.

Regards,
Mauro

> 
> Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
> ---
>  drivers/media/rc/nuvoton-cir.c | 65 +++++++++++++++++++++++++++++++-----------
>  drivers/media/rc/nuvoton-cir.h |  6 ++--
>  2 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
> index 21ee0dc..b11ee43 100644
> --- a/drivers/media/rc/nuvoton-cir.c
> +++ b/drivers/media/rc/nuvoton-cir.c
> @@ -39,6 +39,15 @@
>  
>  #include "nuvoton-cir.h"
>  
> +/* debugging module parameter */
> +static int debug;
> +
> +/* Wake up configuration parameters */
> +static unsigned char wake_samples[WAKE_FIFO_LEN];
> +static unsigned int num_wake_samples;
> +static unsigned char cmp_deep;
> +static unsigned char cmp_tolerance = CIR_WAKE_CMP_TOLERANCE;
> +
>  /* write val to config reg */
>  static inline void nvt_cr_write(struct nvt_dev *nvt, u8 val, u8 reg)
>  {
> @@ -418,13 +427,38 @@ static void nvt_cir_regs_init(struct nvt_dev *nvt)
>  
>  static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
>  {
> +	int i;
> +	u8 cmp_reg = CIR_WAKE_FIFO_CMP_BYTES;
> +	u8 ircon_reg = CIR_WAKE_IRCON_RXEN | CIR_WAKE_IRCON_R |
> +		       CIR_WAKE_IRCON_RXINV | CIR_WAKE_IRCON_SAMPLE_PERIOD_SEL;
> +	/*
> +	 * Enable TX and RX, specific carrier on = low, off = high, and set
> +	 * sample period (currently 50us)
> +	 */
> +	nvt_cir_wake_reg_write(nvt, ircon_reg | CIR_WAKE_IRCON_MODE1,
> +			       CIR_WAKE_IRCON);
> +
> +	/* clear cir wake rx fifo */
> +	nvt_clear_cir_wake_fifo(nvt);
> +
> +	/* Write samples from module parameter to fifo */
> +	for (i = 0; i < num_wake_samples; i++)
> +		nvt_cir_wake_reg_write(nvt, wake_samples[i],
> +				       CIR_WAKE_WR_FIFO_DATA);
> +
> +	/* Switch cir to wakeup mode and disable fifo writing */
> +	nvt_cir_wake_reg_write(nvt, ircon_reg | CIR_WAKE_IRCON_MODE0,
> +			       CIR_WAKE_IRCON);
> +
>  	/* set number of bytes needed for wake from s3 (default 65) */
> -	nvt_cir_wake_reg_write(nvt, CIR_WAKE_FIFO_CMP_BYTES,
> -			       CIR_WAKE_FIFO_CMP_DEEP);
> +	if (cmp_deep)
> +		cmp_reg = cmp_deep;
> +	else if (num_wake_samples)
> +		cmp_reg = num_wake_samples;
> +	nvt_cir_wake_reg_write(nvt, cmp_reg, CIR_WAKE_FIFO_CMP_DEEP);
>  
>  	/* set tolerance/variance allowed per byte during wake compare */
> -	nvt_cir_wake_reg_write(nvt, CIR_WAKE_CMP_TOLERANCE,
> -			       CIR_WAKE_FIFO_CMP_TOL);
> +	nvt_cir_wake_reg_write(nvt, cmp_tolerance, CIR_WAKE_FIFO_CMP_TOL);
>  
>  	/* set sample limit count (PE interrupt raised when reached) */
>  	nvt_cir_wake_reg_write(nvt, CIR_RX_LIMIT_COUNT >> 8, CIR_WAKE_SLCH);
> @@ -434,18 +468,6 @@ static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
>  	nvt_cir_wake_reg_write(nvt, CIR_WAKE_FIFOCON_RX_TRIGGER_LEV,
>  			       CIR_WAKE_FIFOCON);
>  
> -	/*
> -	 * Enable TX and RX, specific carrier on = low, off = high, and set
> -	 * sample period (currently 50us)
> -	 */
> -	nvt_cir_wake_reg_write(nvt, CIR_WAKE_IRCON_MODE0 | CIR_WAKE_IRCON_RXEN |
> -			       CIR_WAKE_IRCON_R | CIR_WAKE_IRCON_RXINV |
> -			       CIR_WAKE_IRCON_SAMPLE_PERIOD_SEL,
> -			       CIR_WAKE_IRCON);
> -
> -	/* clear cir wake rx fifo */
> -	nvt_clear_cir_wake_fifo(nvt);
> -
>  	/* clear any and all stray interrupts */
>  	nvt_cir_wake_reg_write(nvt, 0xff, CIR_WAKE_IRSTS);
>  }
> @@ -1232,6 +1254,17 @@ static void nvt_exit(void)
>  module_param(debug, int, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(debug, "Enable debugging output");
>  
> +module_param_array(wake_samples, byte, &num_wake_samples, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(wake_samples, "FIFO sample bytes triggering wake");
> +
> +module_param(cmp_deep, byte, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(cmp_deep, "How many bytes need to compare\n"
> +			   "\t\t(0 = auto (default))");
> +
> +module_param(cmp_tolerance, byte, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(cmp_tolerance, "Data tolerance to each wake sample byte\n"
> +				"\t\t(default = 5)");
> +
>  MODULE_DEVICE_TABLE(pnp, nvt_ids);
>  MODULE_DESCRIPTION("Nuvoton W83667HG-A & W83677HG-I CIR driver");
>  
> diff --git a/drivers/media/rc/nuvoton-cir.h b/drivers/media/rc/nuvoton-cir.h
> index 07e8310..8209f84 100644
> --- a/drivers/media/rc/nuvoton-cir.h
> +++ b/drivers/media/rc/nuvoton-cir.h
> @@ -31,10 +31,6 @@
>  /* platform driver name to register */
>  #define NVT_DRIVER_NAME "nuvoton-cir"
>  
> -/* debugging module parameter */
> -static int debug;
> -
> -
>  #define nvt_pr(level, text, ...) \
>  	printk(level KBUILD_MODNAME ": " text, ## __VA_ARGS__)
>  
> @@ -64,6 +60,8 @@ static int debug;
>  #define TX_BUF_LEN 256
>  #define RX_BUF_LEN 32
>  
> +#define WAKE_FIFO_LEN 67
> +
>  struct nvt_dev {
>  	struct pnp_dev *pdev;
>  	struct rc_dev *rdev;
  
Antti Seppälä Jan. 20, 2014, 7:39 p.m. UTC | #2
This patch series introduces a simple sysfs file interface for reading
and writing wakeup scancodes to rc drivers.

This is an improved version of my previous patch for nuvoton-cir which
did the same thing via module parameters. This is a more generic
approach allowing other drivers to utilize the interface as well.

I did not port winbond-cir to this method of wakeup scancode setting yet
because I don't have the hardware to test it and I wanted first to get
some comments about how the patch series looks. I did however write a
simple support to read and write scancodes to rc-loopback module.

Antti Seppälä (4):
  rc-core: Add defintions needed for sysfs callback
  rc-core: Add support for reading/writing wakeup scancodes via sysfs
  rc-loopback: Add support for reading/writing wakeup scancodes via
    sysfs
  nuvoton-cir: Add support for reading/writing wakeup scancodes via
    sysfs

 drivers/media/rc/nuvoton-cir.c |  81 ++++++++++++++++++++++++++
 drivers/media/rc/nuvoton-cir.h |   2 +
 drivers/media/rc/rc-loopback.c |  31 ++++++++++
 drivers/media/rc/rc-main.c     | 129 +++++++++++++++++++++++++++++++++++++++++
 include/media/rc-core.h        |  13 +++++
 5 files changed, 256 insertions(+)
  
Sean Young Jan. 21, 2014, 12:28 p.m. UTC | #3
On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
> This patch series introduces a simple sysfs file interface for reading
> and writing wakeup scancodes to rc drivers.
> 
> This is an improved version of my previous patch for nuvoton-cir which
> did the same thing via module parameters. This is a more generic
> approach allowing other drivers to utilize the interface as well.
> 
> I did not port winbond-cir to this method of wakeup scancode setting yet
> because I don't have the hardware to test it and I wanted first to get
> some comments about how the patch series looks. I did however write a
> simple support to read and write scancodes to rc-loopback module.

Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?

This is needed for winbond-cir; I guess this should be another sysfs
file, something like "wakeup_protocol". Even if the nuvoton can only
handle one IR protocol, maybe it should be exported (readonly) via
sysfs?

I'm happy to help with a winbond-cir implementation; I have the hardware.


Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antti Seppälä Jan. 22, 2014, 3:46 p.m. UTC | #4
On 21 January 2014 14:28, Sean Young <sean@mess.org> wrote:
> On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
>> This patch series introduces a simple sysfs file interface for reading
>> and writing wakeup scancodes to rc drivers.
>>
>> This is an improved version of my previous patch for nuvoton-cir which
>> did the same thing via module parameters. This is a more generic
>> approach allowing other drivers to utilize the interface as well.
>>
>> I did not port winbond-cir to this method of wakeup scancode setting yet
>> because I don't have the hardware to test it and I wanted first to get
>> some comments about how the patch series looks. I did however write a
>> simple support to read and write scancodes to rc-loopback module.
>
> Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?
>
> This is needed for winbond-cir; I guess this should be another sysfs
> file, something like "wakeup_protocol". Even if the nuvoton can only
> handle one IR protocol, maybe it should be exported (readonly) via
> sysfs?
>
> I'm happy to help with a winbond-cir implementation; I have the hardware.
>
>
> Sean

Nuvoton-cir doesn't care about the IR protocol because the hardware
compares raw IR pulse lengths and wakes the system if received pulse
is within certain tolerance of the one pre-programmed to the HW. This
approach is agnostic to the used IR protocol.

I glanced over the winbond-cir driver and porting the driver to use
sysfs for wakeup scancodes looks doable. Also a new sysfs entry for
setting the wakeup protocol would indeed be needed... I will take a
closer look at this when I have some more time.

-Antti
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Sean Young Jan. 22, 2014, 4:29 p.m. UTC | #5
On Wed, Jan 22, 2014 at 05:46:28PM +0200, Antti Seppälä wrote:
> On 21 January 2014 14:28, Sean Young <sean@mess.org> wrote:
> > On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
> >> This patch series introduces a simple sysfs file interface for reading
> >> and writing wakeup scancodes to rc drivers.
> >>
> >> This is an improved version of my previous patch for nuvoton-cir which
> >> did the same thing via module parameters. This is a more generic
> >> approach allowing other drivers to utilize the interface as well.
> >>
> >> I did not port winbond-cir to this method of wakeup scancode setting yet
> >> because I don't have the hardware to test it and I wanted first to get
> >> some comments about how the patch series looks. I did however write a
> >> simple support to read and write scancodes to rc-loopback module.
> >
> > Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?
> >
> > This is needed for winbond-cir; I guess this should be another sysfs
> > file, something like "wakeup_protocol". Even if the nuvoton can only
> > handle one IR protocol, maybe it should be exported (readonly) via
> > sysfs?
> >
> > I'm happy to help with a winbond-cir implementation; I have the hardware.
> >
> >
> > Sean
> 
> Nuvoton-cir doesn't care about the IR protocol because the hardware
> compares raw IR pulse lengths and wakes the system if received pulse
> is within certain tolerance of the one pre-programmed to the HW. This
> approach is agnostic to the used IR protocol.

Your patch talks about scancodes which is something entirely different.
This should be renamed to something better.

So with the nuvoton you program a set of pulses and spaces; with the
winbond you set the protocol and the scancode. I don't think there is
any shared code here. Maybe it's better to implement the wakeup 
sysfs files in the drivers themselves rather than in rcdev, I guess that
depends on whether there are other devices that implement similar 
functionality.


Sean

> I glanced over the winbond-cir driver and porting the driver to use
> sysfs for wakeup scancodes looks doable. Also a new sysfs entry for
> setting the wakeup protocol would indeed be needed... I will take a
> closer look at this when I have some more time.
> 
> -Antti
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antti Seppälä Jan. 22, 2014, 7:10 p.m. UTC | #6
On 22 January 2014 18:29, Sean Young <sean@mess.org> wrote:
> On Wed, Jan 22, 2014 at 05:46:28PM +0200, Antti Seppälä wrote:
>> On 21 January 2014 14:28, Sean Young <sean@mess.org> wrote:
>> > On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
>> >> This patch series introduces a simple sysfs file interface for reading
>> >> and writing wakeup scancodes to rc drivers.
>> >>
>> >> This is an improved version of my previous patch for nuvoton-cir which
>> >> did the same thing via module parameters. This is a more generic
>> >> approach allowing other drivers to utilize the interface as well.
>> >>
>> >> I did not port winbond-cir to this method of wakeup scancode setting yet
>> >> because I don't have the hardware to test it and I wanted first to get
>> >> some comments about how the patch series looks. I did however write a
>> >> simple support to read and write scancodes to rc-loopback module.
>> >
>> > Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?
>> >
>> > This is needed for winbond-cir; I guess this should be another sysfs
>> > file, something like "wakeup_protocol". Even if the nuvoton can only
>> > handle one IR protocol, maybe it should be exported (readonly) via
>> > sysfs?
>> >
>> > I'm happy to help with a winbond-cir implementation; I have the hardware.
>> >
>> >
>> > Sean
>>
>> Nuvoton-cir doesn't care about the IR protocol because the hardware
>> compares raw IR pulse lengths and wakes the system if received pulse
>> is within certain tolerance of the one pre-programmed to the HW. This
>> approach is agnostic to the used IR protocol.
>
> Your patch talks about scancodes which is something entirely different.
> This should be renamed to something better.
>

I agree that for the nuvoton my choice of wording (scancode) was a
poor one. Perhaps wakeup_code would suit both drivers?

> So with the nuvoton you program a set of pulses and spaces; with the
> winbond you set the protocol and the scancode. I don't think there is
> any shared code here. Maybe it's better to implement the wakeup
> sysfs files in the drivers themselves rather than in rcdev, I guess that
> depends on whether there are other devices that implement similar
> functionality.
>

The code to be shared is the logic of creating, parsing and formatting
the sysfs file. In the end the drivers are only interested in getting
a bunch of values to write to the hardware.

I was thinking about adding another file (wakeup_protocol sounds good)
which would tell what semantics are used to interpret the contents of
wakeup_code file (rc6, rc5, nec or raw). Would this be a decent
solution?

The other alternative is to push the sysfs handling to individual
drivers. I'm ok with either way. Which one should I pursue?

-Antti
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antti Palosaari Jan. 22, 2014, 7:21 p.m. UTC | #7
On 22.01.2014 21:10, Antti Seppälä wrote:
> On 22 January 2014 18:29, Sean Young <sean@mess.org> wrote:
>> On Wed, Jan 22, 2014 at 05:46:28PM +0200, Antti Seppälä wrote:
>>> On 21 January 2014 14:28, Sean Young <sean@mess.org> wrote:
>>>> On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
>>>>> This patch series introduces a simple sysfs file interface for reading
>>>>> and writing wakeup scancodes to rc drivers.
>>>>>
>>>>> This is an improved version of my previous patch for nuvoton-cir which
>>>>> did the same thing via module parameters. This is a more generic
>>>>> approach allowing other drivers to utilize the interface as well.
>>>>>
>>>>> I did not port winbond-cir to this method of wakeup scancode setting yet
>>>>> because I don't have the hardware to test it and I wanted first to get
>>>>> some comments about how the patch series looks. I did however write a
>>>>> simple support to read and write scancodes to rc-loopback module.
>>>>
>>>> Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?
>>>>
>>>> This is needed for winbond-cir; I guess this should be another sysfs
>>>> file, something like "wakeup_protocol". Even if the nuvoton can only
>>>> handle one IR protocol, maybe it should be exported (readonly) via
>>>> sysfs?
>>>>
>>>> I'm happy to help with a winbond-cir implementation; I have the hardware.
>>>>
>>>>
>>>> Sean
>>>
>>> Nuvoton-cir doesn't care about the IR protocol because the hardware
>>> compares raw IR pulse lengths and wakes the system if received pulse
>>> is within certain tolerance of the one pre-programmed to the HW. This
>>> approach is agnostic to the used IR protocol.
>>
>> Your patch talks about scancodes which is something entirely different.
>> This should be renamed to something better.
>>
>
> I agree that for the nuvoton my choice of wording (scancode) was a
> poor one. Perhaps wakeup_code would suit both drivers?
>
>> So with the nuvoton you program a set of pulses and spaces; with the
>> winbond you set the protocol and the scancode. I don't think there is
>> any shared code here. Maybe it's better to implement the wakeup
>> sysfs files in the drivers themselves rather than in rcdev, I guess that
>> depends on whether there are other devices that implement similar
>> functionality.
>>
>
> The code to be shared is the logic of creating, parsing and formatting
> the sysfs file. In the end the drivers are only interested in getting
> a bunch of values to write to the hardware.
>
> I was thinking about adding another file (wakeup_protocol sounds good)
> which would tell what semantics are used to interpret the contents of
> wakeup_code file (rc6, rc5, nec or raw). Would this be a decent
> solution?
>
> The other alternative is to push the sysfs handling to individual
> drivers. I'm ok with either way. Which one should I pursue?

RTL2832U supports also that function. Could you study it too at least 
check that implementation will be suitable for it?

regards
Antti
  
Sean Young Jan. 22, 2014, 9 p.m. UTC | #8
On Wed, Jan 22, 2014 at 09:10:58PM +0200, Antti Seppälä wrote:
> On 22 January 2014 18:29, Sean Young <sean@mess.org> wrote:
> > On Wed, Jan 22, 2014 at 05:46:28PM +0200, Antti Seppälä wrote:
> >> On 21 January 2014 14:28, Sean Young <sean@mess.org> wrote:
> >> > On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
> >> >> This patch series introduces a simple sysfs file interface for reading
> >> >> and writing wakeup scancodes to rc drivers.
> >> >>
> >> >> This is an improved version of my previous patch for nuvoton-cir which
> >> >> did the same thing via module parameters. This is a more generic
> >> >> approach allowing other drivers to utilize the interface as well.
> >> >>
> >> >> I did not port winbond-cir to this method of wakeup scancode setting yet
> >> >> because I don't have the hardware to test it and I wanted first to get
> >> >> some comments about how the patch series looks. I did however write a
> >> >> simple support to read and write scancodes to rc-loopback module.
> >> >
> >> > Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?
> >> >
> >> > This is needed for winbond-cir; I guess this should be another sysfs
> >> > file, something like "wakeup_protocol". Even if the nuvoton can only
> >> > handle one IR protocol, maybe it should be exported (readonly) via
> >> > sysfs?
> >> >
> >> > I'm happy to help with a winbond-cir implementation; I have the hardware.
> >> >
> >> >
> >> > Sean
> >>
> >> Nuvoton-cir doesn't care about the IR protocol because the hardware
> >> compares raw IR pulse lengths and wakes the system if received pulse
> >> is within certain tolerance of the one pre-programmed to the HW. This
> >> approach is agnostic to the used IR protocol.
> >
> > Your patch talks about scancodes which is something entirely different.
> > This should be renamed to something better.
> >
> 
> I agree that for the nuvoton my choice of wording (scancode) was a
> poor one. Perhaps wakeup_code would suit both drivers?
> 
> > So with the nuvoton you program a set of pulses and spaces; with the
> > winbond you set the protocol and the scancode. I don't think there is
> > any shared code here. Maybe it's better to implement the wakeup
> > sysfs files in the drivers themselves rather than in rcdev, I guess that
> > depends on whether there are other devices that implement similar
> > functionality.
> >
> 
> The code to be shared is the logic of creating, parsing and formatting
> the sysfs file. In the end the drivers are only interested in getting
> a bunch of values to write to the hardware.
> 
> I was thinking about adding another file (wakeup_protocol sounds good)
> which would tell what semantics are used to interpret the contents of
> wakeup_code file (rc6, rc5, nec or raw). Would this be a decent
> solution?

Good idea. I like it.

Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Jan. 22, 2014, 10:01 p.m. UTC | #9
Em Wed, 22 Jan 2014 21:00:24 +0000
Sean Young <sean@mess.org> escreveu:

> On Wed, Jan 22, 2014 at 09:10:58PM +0200, Antti Seppälä wrote:
> > On 22 January 2014 18:29, Sean Young <sean@mess.org> wrote:
> > > On Wed, Jan 22, 2014 at 05:46:28PM +0200, Antti Seppälä wrote:
> > >> On 21 January 2014 14:28, Sean Young <sean@mess.org> wrote:
> > >> > On Mon, Jan 20, 2014 at 09:39:43PM +0200, Antti Seppälä wrote:
> > >> >> This patch series introduces a simple sysfs file interface for reading
> > >> >> and writing wakeup scancodes to rc drivers.
> > >> >>
> > >> >> This is an improved version of my previous patch for nuvoton-cir which
> > >> >> did the same thing via module parameters. This is a more generic
> > >> >> approach allowing other drivers to utilize the interface as well.
> > >> >>
> > >> >> I did not port winbond-cir to this method of wakeup scancode setting yet
> > >> >> because I don't have the hardware to test it and I wanted first to get
> > >> >> some comments about how the patch series looks. I did however write a
> > >> >> simple support to read and write scancodes to rc-loopback module.
> > >> >
> > >> > Doesn't the nuvoton-cir driver need to know the IR protocol for wakeup?
> > >> >
> > >> > This is needed for winbond-cir; I guess this should be another sysfs
> > >> > file, something like "wakeup_protocol". Even if the nuvoton can only
> > >> > handle one IR protocol, maybe it should be exported (readonly) via
> > >> > sysfs?
> > >> >
> > >> > I'm happy to help with a winbond-cir implementation; I have the hardware.
> > >> >
> > >> >
> > >> > Sean
> > >>
> > >> Nuvoton-cir doesn't care about the IR protocol because the hardware
> > >> compares raw IR pulse lengths and wakes the system if received pulse
> > >> is within certain tolerance of the one pre-programmed to the HW. This
> > >> approach is agnostic to the used IR protocol.
> > >
> > > Your patch talks about scancodes which is something entirely different.
> > > This should be renamed to something better.
> > >
> > 
> > I agree that for the nuvoton my choice of wording (scancode) was a
> > poor one. Perhaps wakeup_code would suit both drivers?
> > 
> > > So with the nuvoton you program a set of pulses and spaces; with the
> > > winbond you set the protocol and the scancode. I don't think there is
> > > any shared code here. Maybe it's better to implement the wakeup
> > > sysfs files in the drivers themselves rather than in rcdev, I guess that
> > > depends on whether there are other devices that implement similar
> > > functionality.
> > >
> > 
> > The code to be shared is the logic of creating, parsing and formatting
> > the sysfs file. In the end the drivers are only interested in getting
> > a bunch of values to write to the hardware.
> > 
> > I was thinking about adding another file (wakeup_protocol sounds good)
> > which would tell what semantics are used to interpret the contents of
> > wakeup_code file (rc6, rc5, nec or raw). Would this be a decent
> > solution?
> 
> Good idea. I like it.

Not sure if you saw it, but there's already another patchset proposing
that, that got submitted before this changeset:
	https://patchwork.linuxtv.org/patch/21625/
	
> 
> Sean
  
Antti Seppälä Jan. 23, 2014, 7:11 p.m. UTC | #10
On 23 January 2014 00:01, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
> Not sure if you saw it, but there's already another patchset proposing
> that, that got submitted before this changeset:
>         https://patchwork.linuxtv.org/patch/21625/

I actually didn't notice that until now. Seems quite a similar kind of
approach with even more advanced features than what I had in mind
(namely the scancode filtering and masking).

However it looks like that patchset has the same drawback about not
knowing which protocol to use for the wakeup scancode as was pointed
from my patch.

I think I'll try to come up with a new patch addressing the comments
I've seen so far.

-Antti
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Feb. 4, 2014, 5:54 p.m. UTC | #11
Em Thu, 23 Jan 2014 21:11:09 +0200
Antti Seppälä <a.seppala@gmail.com> escreveu:

> On 23 January 2014 00:01, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
> > Not sure if you saw it, but there's already another patchset proposing
> > that, that got submitted before this changeset:
> >         https://patchwork.linuxtv.org/patch/21625/
> 
> I actually didn't notice that until now. Seems quite a similar kind of
> approach with even more advanced features than what I had in mind
> (namely the scancode filtering and masking).
> 
> However it looks like that patchset has the same drawback about not
> knowing which protocol to use for the wakeup scancode as was pointed
> from my patch.

Well, the protocol is important only if there are two or more active
RCs that produce the same IR code on different protocols.

Also, from the sysfs description made by James, it seems clear to me
that the protocol to be used is the current protocol.

I think is an unlikely border case to have some hardware that supports
more than one IR protocols enabled for the wakeup to work, so James
patch looks ok on my eyes.

Also, nothing prevents to add latter a wakeup_filter_protocol sysfs node
to allow to filter the wakeup scancode to a protocol set different than
the one(s) currently enabled.

> I think I'll try to come up with a new patch addressing the comments
> I've seen so far.

I guess I'll merge James approach, as there are a pile of other patches
depending on it. If we need latter to distinguish between current_protocol
and the wakeup one, as I said, a latter patch can add a
"wakeup_filter_protocol" sysfs node to specify it.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antti Seppälä Feb. 5, 2014, 7:03 a.m. UTC | #12
On 4 February 2014 19:54, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
> Em Thu, 23 Jan 2014 21:11:09 +0200
> Antti Seppälä <a.seppala@gmail.com> escreveu:
>
>> On 23 January 2014 00:01, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
>> > Not sure if you saw it, but there's already another patchset proposing
>> > that, that got submitted before this changeset:
>> >         https://patchwork.linuxtv.org/patch/21625/
>>
>> I actually didn't notice that until now. Seems quite a similar kind of
>> approach with even more advanced features than what I had in mind
>> (namely the scancode filtering and masking).
>>
>> However it looks like that patchset has the same drawback about not
>> knowing which protocol to use for the wakeup scancode as was pointed
>> from my patch.
>
> Well, the protocol is important only if there are two or more active
> RCs that produce the same IR code on different protocols.
>
> Also, from the sysfs description made by James, it seems clear to me
> that the protocol to be used is the current protocol.
>
> I think is an unlikely border case to have some hardware that supports
> more than one IR protocols enabled for the wakeup to work, so James
> patch looks ok on my eyes.
>
> Also, nothing prevents to add latter a wakeup_filter_protocol sysfs node
> to allow to filter the wakeup scancode to a protocol set different than
> the one(s) currently enabled.
>
>> I think I'll try to come up with a new patch addressing the comments
>> I've seen so far.
>
> I guess I'll merge James approach, as there are a pile of other patches
> depending on it. If we need latter to distinguish between current_protocol
> and the wakeup one, as I said, a latter patch can add a
> "wakeup_filter_protocol" sysfs node to specify it.
>
> Regards,
> Mauro

Hi Mauro.

How do you want to proceed with the nuvoton wakeup I initially set out to do?

To wake up with nuvoton-cir we need to program several raw ir
pulse/space lengths to the hardware and not a scancode. James's
approach doesn't support this.

To keep things simple maybe module parameters in my first patch
(http://www.mail-archive.com/linux-media@vger.kernel.org/msg69782.html)
are then the best thing to do for nuvoton? Other drivers can probably
adapt to the suggested filter api.

-Antti
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Feb. 5, 2014, 9:36 a.m. UTC | #13
Hi Antti,

Em Wed, 05 Feb 2014 09:03:29 +0200
Antti Seppälä <a.seppala@gmail.com> escreveu:

> On 4 February 2014 19:54, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
> > Em Thu, 23 Jan 2014 21:11:09 +0200
> > Antti Seppälä <a.seppala@gmail.com> escreveu:
> >
> >> On 23 January 2014 00:01, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
> >> > Not sure if you saw it, but there's already another patchset proposing
> >> > that, that got submitted before this changeset:
> >> >         https://patchwork.linuxtv.org/patch/21625/
> >>
> >> I actually didn't notice that until now. Seems quite a similar kind of
> >> approach with even more advanced features than what I had in mind
> >> (namely the scancode filtering and masking).
> >>
> >> However it looks like that patchset has the same drawback about not
> >> knowing which protocol to use for the wakeup scancode as was pointed
> >> from my patch.
> >
> > Well, the protocol is important only if there are two or more active
> > RCs that produce the same IR code on different protocols.
> >
> > Also, from the sysfs description made by James, it seems clear to me
> > that the protocol to be used is the current protocol.
> >
> > I think is an unlikely border case to have some hardware that supports
> > more than one IR protocols enabled for the wakeup to work, so James
> > patch looks ok on my eyes.
> >
> > Also, nothing prevents to add latter a wakeup_filter_protocol sysfs node
> > to allow to filter the wakeup scancode to a protocol set different than
> > the one(s) currently enabled.
> >
> >> I think I'll try to come up with a new patch addressing the comments
> >> I've seen so far.
> >
> > I guess I'll merge James approach, as there are a pile of other patches
> > depending on it. If we need latter to distinguish between current_protocol
> > and the wakeup one, as I said, a latter patch can add a
> > "wakeup_filter_protocol" sysfs node to specify it.
> >
> > Regards,
> > Mauro
> 
> Hi Mauro.
> 
> How do you want to proceed with the nuvoton wakeup I initially set out to do?
> 
> To wake up with nuvoton-cir we need to program several raw ir
> pulse/space lengths to the hardware and not a scancode. James's
> approach doesn't support this.
> 
> To keep things simple maybe module parameters in my first patch
> (http://www.mail-archive.com/linux-media@vger.kernel.org/msg69782.html)
> are then the best thing to do for nuvoton? Other drivers can probably
> adapt to the suggested filter api.

Well, you're basically telling that you need an IR encoder to convert
from protocol/scancode into pulse/space lengths. 

As we need/want a portable interface that will provide the same API
to  userspace, no matter what hardware, I can see two alternatives:

1) Use a lirc-like API for that, using the IR encoder code in userspace;

2) The same way as we mapped IR decoders, add IR encoders that can be
used either for IR TX or to provide IR wakeup codes.

IMO, (2) is a much more elegant option.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
James Hogan Feb. 5, 2014, 9:39 a.m. UTC | #14
Hi Antti,

On 05/02/14 07:03, Antti Seppälä wrote:
> To wake up with nuvoton-cir we need to program several raw ir
> pulse/space lengths to the hardware and not a scancode. James's
> approach doesn't support this.

Do the raw pulse/space lengths your hardware requires correspond to a
single IR packet (mapping to a single scancode)?

If so then my API is simply at a higher level of abstraction. I think
this has the following advantages:
* userspace sees a consistent interface at the same level of abstraction
as it already has access to from input subsystem (i.e. scancodes). I.e.
it doesn't need to care which IR device is in use, whether it does
raw/hardware decode, or the details of the timings of the current protocol.
* it supports hardware decoders which filter on the demodulated data
rather than the raw pulse/space lengths.

Of course to support this we'd need some per-protocol code to convert a
scancode back to pulse/space lengths. I'd like to think that code could
be generic, maybe as helper functions which multiple drivers could use,
which could also handle corner cases of the API in a consistent way
(e.g. user providing filter mask covering multiple scancodes, which
presumably pulse/space).

I see I've just crossed emails with Mauro who has just suggested
something similar. I agree that his (2) is the more elegant option.

Cheers
James

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
James Hogan Feb. 5, 2014, 9:42 a.m. UTC | #15
On 05/02/14 09:39, James Hogan wrote:
> Hi Antti,
> 
> On 05/02/14 07:03, Antti Seppälä wrote:
>> To wake up with nuvoton-cir we need to program several raw ir
>> pulse/space lengths to the hardware and not a scancode. James's
>> approach doesn't support this.
> 
> Do the raw pulse/space lengths your hardware requires correspond to a
> single IR packet (mapping to a single scancode)?
> 
> If so then my API is simply at a higher level of abstraction. I think
> this has the following advantages:
> * userspace sees a consistent interface at the same level of abstraction
> as it already has access to from input subsystem (i.e. scancodes). I.e.
> it doesn't need to care which IR device is in use, whether it does
> raw/hardware decode, or the details of the timings of the current protocol.
> * it supports hardware decoders which filter on the demodulated data
> rather than the raw pulse/space lengths.
> 
> Of course to support this we'd need some per-protocol code to convert a
> scancode back to pulse/space lengths. I'd like to think that code could
> be generic, maybe as helper functions which multiple drivers could use,
> which could also handle corner cases of the API in a consistent way
> (e.g. user providing filter mask covering multiple scancodes, which
> presumably pulse/space).

hmm, I didn't complete that sentence :(.
I meant:
..., which presumably pulse/space can't really represent very easily).

Cheers
James

> 
> I see I've just crossed emails with Mauro who has just suggested
> something similar. I agree that his (2) is the more elegant option.
> 
> Cheers
> James
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antti Seppälä Feb. 5, 2014, 6:16 p.m. UTC | #16
On 5 February 2014 11:42, James Hogan <james.hogan@imgtec.com> wrote:
> On 05/02/14 09:39, James Hogan wrote:
>> Hi Antti,
>>
>> On 05/02/14 07:03, Antti Seppälä wrote:
>>> To wake up with nuvoton-cir we need to program several raw ir
>>> pulse/space lengths to the hardware and not a scancode. James's
>>> approach doesn't support this.
>>
>> Do the raw pulse/space lengths your hardware requires correspond to a
>> single IR packet (mapping to a single scancode)?
>>
>> If so then my API is simply at a higher level of abstraction. I think
>> this has the following advantages:
>> * userspace sees a consistent interface at the same level of abstraction
>> as it already has access to from input subsystem (i.e. scancodes). I.e.
>> it doesn't need to care which IR device is in use, whether it does
>> raw/hardware decode, or the details of the timings of the current protocol.
>> * it supports hardware decoders which filter on the demodulated data
>> rather than the raw pulse/space lengths.
>>
>> Of course to support this we'd need some per-protocol code to convert a
>> scancode back to pulse/space lengths. I'd like to think that code could
>> be generic, maybe as helper functions which multiple drivers could use,
>> which could also handle corner cases of the API in a consistent way
>> (e.g. user providing filter mask covering multiple scancodes, which
>> presumably pulse/space).
>
> hmm, I didn't complete that sentence :(.
> I meant:
> ..., which presumably pulse/space can't really represent very easily).
>
> Cheers
> James
>
>>
>> I see I've just crossed emails with Mauro who has just suggested
>> something similar. I agree that his (2) is the more elegant option.
>>

Yes, in nuvoton the ir pulses correspond to a scancode (or part of a scancode)

After giving it some thought I agree that using scancodes is the most
elegant way for specifying wakeup commands. Too bad that nuvoton does
not work with scancodes.
I pretty much agree with Mauro that the right solution would be to
write an IR encoder and use it to convert the given scancode back to a
format understood by nuvoton.

Writing IR encoders for all the protocols and an encoder selector
functionality is quite labourous and sadly I don't have time for that
anytime soon. If anyone wants to step up I'd be more than happy to
help though :)

-Antti
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Feb. 5, 2014, 9:21 p.m. UTC | #17
Em Wed, 05 Feb 2014 20:16:04 +0200
Antti Seppälä <a.seppala@gmail.com> escreveu:

> On 5 February 2014 11:42, James Hogan <james.hogan@imgtec.com> wrote:
> > On 05/02/14 09:39, James Hogan wrote:
> >> Hi Antti,
> >>
> >> On 05/02/14 07:03, Antti Seppälä wrote:
> >>> To wake up with nuvoton-cir we need to program several raw ir
> >>> pulse/space lengths to the hardware and not a scancode. James's
> >>> approach doesn't support this.
> >>
> >> Do the raw pulse/space lengths your hardware requires correspond to a
> >> single IR packet (mapping to a single scancode)?
> >>
> >> If so then my API is simply at a higher level of abstraction. I think
> >> this has the following advantages:
> >> * userspace sees a consistent interface at the same level of abstraction
> >> as it already has access to from input subsystem (i.e. scancodes). I.e.
> >> it doesn't need to care which IR device is in use, whether it does
> >> raw/hardware decode, or the details of the timings of the current protocol.
> >> * it supports hardware decoders which filter on the demodulated data
> >> rather than the raw pulse/space lengths.
> >>
> >> Of course to support this we'd need some per-protocol code to convert a
> >> scancode back to pulse/space lengths. I'd like to think that code could
> >> be generic, maybe as helper functions which multiple drivers could use,
> >> which could also handle corner cases of the API in a consistent way
> >> (e.g. user providing filter mask covering multiple scancodes, which
> >> presumably pulse/space).
> >
> > hmm, I didn't complete that sentence :(.
> > I meant:
> > ..., which presumably pulse/space can't really represent very easily).
> >
> > Cheers
> > James
> >
> >>
> >> I see I've just crossed emails with Mauro who has just suggested
> >> something similar. I agree that his (2) is the more elegant option.
> >>
> 
> Yes, in nuvoton the ir pulses correspond to a scancode (or part of a scancode)
> 
> After giving it some thought I agree that using scancodes is the most
> elegant way for specifying wakeup commands. Too bad that nuvoton does
> not work with scancodes.
> I pretty much agree with Mauro that the right solution would be to
> write an IR encoder and use it to convert the given scancode back to a
> format understood by nuvoton.

Ok, as we all agreed, I'll merge the remaining patches from James.

> Writing IR encoders for all the protocols and an encoder selector
> functionality is quite labourous and sadly I don't have time for that
> anytime soon. If anyone wants to step up I'd be more than happy to
> help though :)

I suspect that writing one IR encoder should not be hard, as there
are already some on LIRC userspace.

I would love to have some time to write at least a few IR encoders,
but, unfortunately, I would not have any time soon.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
James Hogan Feb. 6, 2014, 10:46 a.m. UTC | #18
On 05/02/14 21:21, Mauro Carvalho Chehab wrote:
> Em Wed, 05 Feb 2014 20:16:04 +0200
> Antti Seppälä <a.seppala@gmail.com> escreveu:
> 
>> On 5 February 2014 11:42, James Hogan <james.hogan@imgtec.com> wrote:
>>> On 05/02/14 09:39, James Hogan wrote:
>>>> Hi Antti,
>>>>
>>>> On 05/02/14 07:03, Antti Seppälä wrote:
>>>>> To wake up with nuvoton-cir we need to program several raw ir
>>>>> pulse/space lengths to the hardware and not a scancode. James's
>>>>> approach doesn't support this.
>>>>
>>>> Do the raw pulse/space lengths your hardware requires correspond to a
>>>> single IR packet (mapping to a single scancode)?
>>>>
>>>> If so then my API is simply at a higher level of abstraction. I think
>>>> this has the following advantages:
>>>> * userspace sees a consistent interface at the same level of abstraction
>>>> as it already has access to from input subsystem (i.e. scancodes). I.e.
>>>> it doesn't need to care which IR device is in use, whether it does
>>>> raw/hardware decode, or the details of the timings of the current protocol.
>>>> * it supports hardware decoders which filter on the demodulated data
>>>> rather than the raw pulse/space lengths.
>>>>
>>>> Of course to support this we'd need some per-protocol code to convert a
>>>> scancode back to pulse/space lengths. I'd like to think that code could
>>>> be generic, maybe as helper functions which multiple drivers could use,
>>>> which could also handle corner cases of the API in a consistent way
>>>> (e.g. user providing filter mask covering multiple scancodes, which
>>>> presumably pulse/space).
>>>
>>> hmm, I didn't complete that sentence :(.
>>> I meant:
>>> ..., which presumably pulse/space can't really represent very easily).
>>>
>>> Cheers
>>> James
>>>
>>>>
>>>> I see I've just crossed emails with Mauro who has just suggested
>>>> something similar. I agree that his (2) is the more elegant option.
>>>>
>>
>> Yes, in nuvoton the ir pulses correspond to a scancode (or part of a scancode)
>>
>> After giving it some thought I agree that using scancodes is the most
>> elegant way for specifying wakeup commands. Too bad that nuvoton does
>> not work with scancodes.
>> I pretty much agree with Mauro that the right solution would be to
>> write an IR encoder and use it to convert the given scancode back to a
>> format understood by nuvoton.
> 
> Ok, as we all agreed, I'll merge the remaining patches from James.

Thanks :)

> 
>> Writing IR encoders for all the protocols and an encoder selector
>> functionality is quite labourous and sadly I don't have time for that
>> anytime soon. If anyone wants to step up I'd be more than happy to
>> help though :)
> 
> I suspect that writing one IR encoder should not be hard, as there
> are already some on LIRC userspace.
> 
> I would love to have some time to write at least a few IR encoders,
> but, unfortunately, I would not have any time soon.

For fun, I did some experimentation yesterday evening with adding basic
generic IR encoding (just NEC implemented so far). Encoders can
certainly be a lot simpler than decoders.

I'll submit a few RFC patches later so Antti can see whether it would be
suitable for nuvoton.

Cheers
James

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Feb. 6, 2014, 2:55 p.m. UTC | #19
Em Thu, 06 Feb 2014 10:46:04 +0000
James Hogan <james.hogan@imgtec.com> escreveu:

> On 05/02/14 21:21, Mauro Carvalho Chehab wrote:
> > Em Wed, 05 Feb 2014 20:16:04 +0200
> > Antti Seppälä <a.seppala@gmail.com> escreveu:
> > 
> >> On 5 February 2014 11:42, James Hogan <james.hogan@imgtec.com> wrote:
> >>> On 05/02/14 09:39, James Hogan wrote:
> >>>> Hi Antti,
> >>>>
> >>>> On 05/02/14 07:03, Antti Seppälä wrote:
> >>>>> To wake up with nuvoton-cir we need to program several raw ir
> >>>>> pulse/space lengths to the hardware and not a scancode. James's
> >>>>> approach doesn't support this.
> >>>>
> >>>> Do the raw pulse/space lengths your hardware requires correspond to a
> >>>> single IR packet (mapping to a single scancode)?
> >>>>
> >>>> If so then my API is simply at a higher level of abstraction. I think
> >>>> this has the following advantages:
> >>>> * userspace sees a consistent interface at the same level of abstraction
> >>>> as it already has access to from input subsystem (i.e. scancodes). I.e.
> >>>> it doesn't need to care which IR device is in use, whether it does
> >>>> raw/hardware decode, or the details of the timings of the current protocol.
> >>>> * it supports hardware decoders which filter on the demodulated data
> >>>> rather than the raw pulse/space lengths.
> >>>>
> >>>> Of course to support this we'd need some per-protocol code to convert a
> >>>> scancode back to pulse/space lengths. I'd like to think that code could
> >>>> be generic, maybe as helper functions which multiple drivers could use,
> >>>> which could also handle corner cases of the API in a consistent way
> >>>> (e.g. user providing filter mask covering multiple scancodes, which
> >>>> presumably pulse/space).
> >>>
> >>> hmm, I didn't complete that sentence :(.
> >>> I meant:
> >>> ..., which presumably pulse/space can't really represent very easily).
> >>>
> >>> Cheers
> >>> James
> >>>
> >>>>
> >>>> I see I've just crossed emails with Mauro who has just suggested
> >>>> something similar. I agree that his (2) is the more elegant option.
> >>>>
> >>
> >> Yes, in nuvoton the ir pulses correspond to a scancode (or part of a scancode)
> >>
> >> After giving it some thought I agree that using scancodes is the most
> >> elegant way for specifying wakeup commands. Too bad that nuvoton does
> >> not work with scancodes.
> >> I pretty much agree with Mauro that the right solution would be to
> >> write an IR encoder and use it to convert the given scancode back to a
> >> format understood by nuvoton.
> > 
> > Ok, as we all agreed, I'll merge the remaining patches from James.
> 
> Thanks :)

Well, Rob asked some changes at the DT binding pathes. I'll tag the
patches as changes requested. Please re-submit the remaining ones after
fixing the pointed issues and getting his ack. Except for that, the
series look OK on my eyes.

> > 
> >> Writing IR encoders for all the protocols and an encoder selector
> >> functionality is quite labourous and sadly I don't have time for that
> >> anytime soon. If anyone wants to step up I'd be more than happy to
> >> help though :)
> > 
> > I suspect that writing one IR encoder should not be hard, as there
> > are already some on LIRC userspace.
> > 
> > I would love to have some time to write at least a few IR encoders,
> > but, unfortunately, I would not have any time soon.
> 
> For fun, I did some experimentation yesterday evening with adding basic
> generic IR encoding (just NEC implemented so far). Encoders can
> certainly be a lot simpler than decoders.

Yes, you don't need to deal with time shifts there, nor with missing
events ;)

Well, using the encoders also for TX (as an another ioctl at LIRC) 
would require some extra bits to handle things like carrier frequency,
duty cycle, etc, but lirc should be able to handle those already.

So, a future patch expanding its usage to lirc would just need to
add the proper binding.

Btw, I just noticed that the LIRC UAPI interface is still at the
wrong place. It is at:
	include/media/lirc.h
but it should be at:
	include/uapi/.../lirc.h

I'm wandering why nobody didn't notice it yet when compiling
Lirc userspace... Perhaps userspace is just keeping a copy of
that file without any process to sync it from the Kernel userspace
headers.

> I'll submit a few RFC patches later so Antti can see whether it would be
> suitable for nuvoton.

Thanks for that!

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 21ee0dc..b11ee43 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -39,6 +39,15 @@ 
 
 #include "nuvoton-cir.h"
 
+/* debugging module parameter */
+static int debug;
+
+/* Wake up configuration parameters */
+static unsigned char wake_samples[WAKE_FIFO_LEN];
+static unsigned int num_wake_samples;
+static unsigned char cmp_deep;
+static unsigned char cmp_tolerance = CIR_WAKE_CMP_TOLERANCE;
+
 /* write val to config reg */
 static inline void nvt_cr_write(struct nvt_dev *nvt, u8 val, u8 reg)
 {
@@ -418,13 +427,38 @@  static void nvt_cir_regs_init(struct nvt_dev *nvt)
 
 static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
 {
+	int i;
+	u8 cmp_reg = CIR_WAKE_FIFO_CMP_BYTES;
+	u8 ircon_reg = CIR_WAKE_IRCON_RXEN | CIR_WAKE_IRCON_R |
+		       CIR_WAKE_IRCON_RXINV | CIR_WAKE_IRCON_SAMPLE_PERIOD_SEL;
+	/*
+	 * Enable TX and RX, specific carrier on = low, off = high, and set
+	 * sample period (currently 50us)
+	 */
+	nvt_cir_wake_reg_write(nvt, ircon_reg | CIR_WAKE_IRCON_MODE1,
+			       CIR_WAKE_IRCON);
+
+	/* clear cir wake rx fifo */
+	nvt_clear_cir_wake_fifo(nvt);
+
+	/* Write samples from module parameter to fifo */
+	for (i = 0; i < num_wake_samples; i++)
+		nvt_cir_wake_reg_write(nvt, wake_samples[i],
+				       CIR_WAKE_WR_FIFO_DATA);
+
+	/* Switch cir to wakeup mode and disable fifo writing */
+	nvt_cir_wake_reg_write(nvt, ircon_reg | CIR_WAKE_IRCON_MODE0,
+			       CIR_WAKE_IRCON);
+
 	/* set number of bytes needed for wake from s3 (default 65) */
-	nvt_cir_wake_reg_write(nvt, CIR_WAKE_FIFO_CMP_BYTES,
-			       CIR_WAKE_FIFO_CMP_DEEP);
+	if (cmp_deep)
+		cmp_reg = cmp_deep;
+	else if (num_wake_samples)
+		cmp_reg = num_wake_samples;
+	nvt_cir_wake_reg_write(nvt, cmp_reg, CIR_WAKE_FIFO_CMP_DEEP);
 
 	/* set tolerance/variance allowed per byte during wake compare */
-	nvt_cir_wake_reg_write(nvt, CIR_WAKE_CMP_TOLERANCE,
-			       CIR_WAKE_FIFO_CMP_TOL);
+	nvt_cir_wake_reg_write(nvt, cmp_tolerance, CIR_WAKE_FIFO_CMP_TOL);
 
 	/* set sample limit count (PE interrupt raised when reached) */
 	nvt_cir_wake_reg_write(nvt, CIR_RX_LIMIT_COUNT >> 8, CIR_WAKE_SLCH);
@@ -434,18 +468,6 @@  static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
 	nvt_cir_wake_reg_write(nvt, CIR_WAKE_FIFOCON_RX_TRIGGER_LEV,
 			       CIR_WAKE_FIFOCON);
 
-	/*
-	 * Enable TX and RX, specific carrier on = low, off = high, and set
-	 * sample period (currently 50us)
-	 */
-	nvt_cir_wake_reg_write(nvt, CIR_WAKE_IRCON_MODE0 | CIR_WAKE_IRCON_RXEN |
-			       CIR_WAKE_IRCON_R | CIR_WAKE_IRCON_RXINV |
-			       CIR_WAKE_IRCON_SAMPLE_PERIOD_SEL,
-			       CIR_WAKE_IRCON);
-
-	/* clear cir wake rx fifo */
-	nvt_clear_cir_wake_fifo(nvt);
-
 	/* clear any and all stray interrupts */
 	nvt_cir_wake_reg_write(nvt, 0xff, CIR_WAKE_IRSTS);
 }
@@ -1232,6 +1254,17 @@  static void nvt_exit(void)
 module_param(debug, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(debug, "Enable debugging output");
 
+module_param_array(wake_samples, byte, &num_wake_samples, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(wake_samples, "FIFO sample bytes triggering wake");
+
+module_param(cmp_deep, byte, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(cmp_deep, "How many bytes need to compare\n"
+			   "\t\t(0 = auto (default))");
+
+module_param(cmp_tolerance, byte, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(cmp_tolerance, "Data tolerance to each wake sample byte\n"
+				"\t\t(default = 5)");
+
 MODULE_DEVICE_TABLE(pnp, nvt_ids);
 MODULE_DESCRIPTION("Nuvoton W83667HG-A & W83677HG-I CIR driver");
 
diff --git a/drivers/media/rc/nuvoton-cir.h b/drivers/media/rc/nuvoton-cir.h
index 07e8310..8209f84 100644
--- a/drivers/media/rc/nuvoton-cir.h
+++ b/drivers/media/rc/nuvoton-cir.h
@@ -31,10 +31,6 @@ 
 /* platform driver name to register */
 #define NVT_DRIVER_NAME "nuvoton-cir"
 
-/* debugging module parameter */
-static int debug;
-
-
 #define nvt_pr(level, text, ...) \
 	printk(level KBUILD_MODNAME ": " text, ## __VA_ARGS__)
 
@@ -64,6 +60,8 @@  static int debug;
 #define TX_BUF_LEN 256
 #define RX_BUF_LEN 32
 
+#define WAKE_FIFO_LEN 67
+
 struct nvt_dev {
 	struct pnp_dev *pdev;
 	struct rc_dev *rdev;