[RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[Sean Young]

This patch series tries to fix the lirc interface and extend it so it can
be used to send/recv scancodes in addition to raw IR:

 - Make it possible to receive scancodes from hardware that generates 
   scancodes (with rc protocol information)
 - Make it possible to receive decoded scancodes from raw IR, from the 
   in-kernel decoders (not mce mouse/keyboard). Now you can take any
   remote, run ir-rec and you will get both the raw IR and the decoded
   scancodes plus rc protocol information.
 - Make it possible to send scancodes; in kernel-encoding of IR
 - Make it possible to send scancodes for hardware that can only do that
   (so that lirc_zilog can be moved out of staging, not completed yet)
 - Possibly the lirc interface can be used for cec?

This requires a little refactoring.
 - All rc-core devices will have a lirc device associated with them
 - The rc-core <-> lirc bridge no longer is a "decoder", this never made 
   sense and caused confusion

This requires new API for rc-core lirc devices.
 - New feature LIRC_CAN_REC_SCANCODE and LIRC_CAN_SEND_SCANCODE
 - REC_MODE and SEND_MODE do not enable LIRC_MODE_SCANCODE by default since 
   this would confuse existing userspace code
 - For each scancode we need: 
   - rc protocol (RC_TYPE_*) 
   - toggle and repeat bit for some protocols
   - 32 bit scancode

A separate patch will introduce new v4l-utils tools ir-rec and ir-send for 
displaying and sending IR, raw or scancode.

There are few FIXMEs in the code, I am sending this for early feedback. For
example there are no encoders for most IR protocols (just nec).

However the first four patches can be merged as-is should it be accepted.

Sean Young (6):
  [PATCH 1/6] [media] lirc: remove broken features
  [PATCH 2/6] [media] lirc: LIRC_[SG]ET_SEND_MODE should return -ENOSYS
  [PATCH 3/6] [media] rc: lirc bridge should not be a raw decoder
  [PATCH 4/6] [media] rc: lirc is not a protocol or a keymap
  [PATCH 5/6] [media] lirc: pass IR scancodes to userspace via lirc
    bridge
  [PATCH 6/6] [media] rc: teach lirc how to send scancodes

 .../DocBook/media/v4l/lirc_device_interface.xml    |  82 ++++----
 drivers/media/rc/Kconfig                           |  19 +-
 drivers/media/rc/Makefile                          |   6 +-
 drivers/media/rc/ir-lirc-codec.c                   | 211 ++++++++++++---------
 drivers/media/rc/ir-nec-decoder.c                  |  50 +++++
 drivers/media/rc/keymaps/Makefile                  |   1 -
 drivers/media/rc/keymaps/rc-lirc.c                 |  42 ----
 drivers/media/rc/lirc_dev.c                        |  18 +-
 drivers/media/rc/rc-core-priv.h                    |  43 +++--
 drivers/media/rc/rc-ir-raw.c                       |  23 ++-
 drivers/media/rc/rc-main.c                         |  29 ++-
 drivers/media/rc/st_rc.c                           |   2 +-
 include/media/lirc.h                               |  31 ++-
 include/media/rc-core.h                            |   9 +
 include/media/rc-map.h                             |  42 ++--
 15 files changed, 336 insertions(+), 272 deletions(-)
 delete mode 100644 drivers/media/rc/keymaps/rc-lirc.c

-- 
2.1.0

[RFC PATCH 1/6] [media] lirc: remove broken features

[Sean Young]

The LIRC_GET_FEATURES ioctl returns an int which represents features as
bitwise flags. Two features use duplicate values so they are unusable.
These are also features which have never been implemented in drivers
according to kernel/lirc git history, so noone ever noticed that they're
half-baked.

LIRC_CAN_NOTIFY_DECODE has the same value as LIRC_CAN_SET_REC_CARRIER, so
this feature cannot be detected properly. The LIRC_NOTIFY_DECODE ioctl was
never implemented so remove it. The intent was that a led would be flashed
when the ioctl is called. IR receivers with a led have this handled via
the led interface and the rc-feedback led trigger.

LIRC_CAN_SET_REC_DUTY_CYCLE has the same value as LIRC_CAN_MEASURE_CARRIER,
so there is no way to detect it. The idea was that the
LIRC_SET_REC_DUTY_CYCLE and LIRC_SET_REC_DUTY_CYCLE_RANGE ioctls could be
used to setup a filter for a lower and upper bound of a duty cycle. No
hardware has implemented this; why would you.

Since there never was, or will be, an implementation of these, this is not
an ABI change.

Signed-off-by: Sean Young <sean@mess.org>
---
 .../DocBook/media/v4l/lirc_device_interface.xml    | 16 ++++------------
 include/media/lirc.h                               | 22 ++++------------------
 2 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/lirc_device_interface.xml b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
index 34cada2..25926bd 100644
--- a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
+++ b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
@@ -101,7 +101,7 @@ on working with the default settings initially.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
-    <term>LIRC_{G,S}ET_{SEND,REC}_DUTY_CYCLE</term>
+    <term>LIRC_{G,S}ET_SEND_DUTY_CYCLE</term>
     <listitem>
       <para>Get/set the duty cycle (from 0 to 100) of the carrier signal. Currently,
       no special meaning is defined for 0 or 100, but this could be used to switch
@@ -204,22 +204,14 @@ on working with the default settings initially.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
-    <term>LIRC_SET_REC_{DUTY_CYCLE,CARRIER}_RANGE</term>
+    <term>LIRC_SET_REC_CARRIER_RANGE</term>
     <listitem>
-      <para>To set a range use LIRC_SET_REC_DUTY_CYCLE_RANGE/LIRC_SET_REC_CARRIER_RANGE
-      with the lower bound first and later LIRC_SET_REC_DUTY_CYCLE/LIRC_SET_REC_CARRIER
+      <para>To set a range use LIRC_SET_REC_CARRIER_RANGE
+      with the lower bound first and later LIRC_SET_REC_CARRIER
       with the upper bound.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
-    <term>LIRC_NOTIFY_DECODE</term>
-    <listitem>
-      <para>This ioctl is called by lircd whenever a successful decoding of an
-      incoming IR signal could be done. This can be used by supporting hardware
-      to give visual feedback to the user e.g. by flashing a LED.</para>
-    </listitem>
-  </varlistentry>
-  <varlistentry>
     <term>LIRC_SETUP_{START,END}</term>
     <listitem>
       <para>Setting of several driver parameters can be optimized by encapsulating
diff --git a/include/media/lirc.h b/include/media/lirc.h
index 4b3ab29..7b845f8 100644
--- a/include/media/lirc.h
+++ b/include/media/lirc.h
@@ -67,23 +67,17 @@
 
 #define LIRC_CAN_REC_MASK              LIRC_MODE2REC(LIRC_CAN_SEND_MASK)
 
-#define LIRC_CAN_SET_REC_CARRIER       (LIRC_CAN_SET_SEND_CARRIER << 16)
-#define LIRC_CAN_SET_REC_DUTY_CYCLE    (LIRC_CAN_SET_SEND_DUTY_CYCLE << 16)
-
-#define LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE 0x40000000
 #define LIRC_CAN_SET_REC_CARRIER_RANGE    0x80000000
 #define LIRC_CAN_GET_REC_RESOLUTION       0x20000000
 #define LIRC_CAN_SET_REC_TIMEOUT          0x10000000
 #define LIRC_CAN_SET_REC_FILTER           0x08000000
-
-#define LIRC_CAN_MEASURE_CARRIER          0x02000000
 #define LIRC_CAN_USE_WIDEBAND_RECEIVER    0x04000000
+#define LIRC_CAN_MEASURE_CARRIER          0x02000000
+#define LIRC_CAN_SET_REC_CARRIER          0x01000000
 
 #define LIRC_CAN_SEND(x) ((x)&LIRC_CAN_SEND_MASK)
 #define LIRC_CAN_REC(x) ((x)&LIRC_CAN_REC_MASK)
 
-#define LIRC_CAN_NOTIFY_DECODE            0x01000000
-
 /*** IOCTL commands for lirc driver ***/
 
 #define LIRC_GET_FEATURES              _IOR('i', 0x00000000, __u32)
@@ -93,7 +87,6 @@
 #define LIRC_GET_SEND_CARRIER          _IOR('i', 0x00000003, __u32)
 #define LIRC_GET_REC_CARRIER           _IOR('i', 0x00000004, __u32)
 #define LIRC_GET_SEND_DUTY_CYCLE       _IOR('i', 0x00000005, __u32)
-#define LIRC_GET_REC_DUTY_CYCLE        _IOR('i', 0x00000006, __u32)
 #define LIRC_GET_REC_RESOLUTION        _IOR('i', 0x00000007, __u32)
 
 #define LIRC_GET_MIN_TIMEOUT           _IOR('i', 0x00000008, __u32)
@@ -113,7 +106,6 @@
 #define LIRC_SET_SEND_CARRIER          _IOW('i', 0x00000013, __u32)
 #define LIRC_SET_REC_CARRIER           _IOW('i', 0x00000014, __u32)
 #define LIRC_SET_SEND_DUTY_CYCLE       _IOW('i', 0x00000015, __u32)
-#define LIRC_SET_REC_DUTY_CYCLE        _IOW('i', 0x00000016, __u32)
 #define LIRC_SET_TRANSMITTER_MASK      _IOW('i', 0x00000017, __u32)
 
 /*
@@ -149,17 +141,11 @@
 #define LIRC_SET_MEASURE_CARRIER_MODE	_IOW('i', 0x0000001d, __u32)
 
 /*
- * to set a range use
- * LIRC_SET_REC_DUTY_CYCLE_RANGE/LIRC_SET_REC_CARRIER_RANGE with the
- * lower bound first and later
- * LIRC_SET_REC_DUTY_CYCLE/LIRC_SET_REC_CARRIER with the upper bound
+ * to set a range use LIRC_SET_REC_CARRIER_RANGE with the
+ * lower bound first and later LIRC_SET_REC_CARRIER with the upper bound
  */
-
-#define LIRC_SET_REC_DUTY_CYCLE_RANGE  _IOW('i', 0x0000001e, __u32)
 #define LIRC_SET_REC_CARRIER_RANGE     _IOW('i', 0x0000001f, __u32)
 
-#define LIRC_NOTIFY_DECODE             _IO('i', 0x00000020)
-
 #define LIRC_SETUP_START               _IO('i', 0x00000021)
 #define LIRC_SETUP_END                 _IO('i', 0x00000022)
 
-- 
2.1.0

Re: [RFC PATCH 1/6] [media] lirc: remove broken features

[Mauro Carvalho Chehab]

Sean,

First of all, sorry for a late answer... I got too busy this year, and I
wanted to have more time to better review those RC stuff, are there aren't
many developers reviewing RC code nowadays.

Em Thu, 19 Mar 2015 21:50:12 +0000
Sean Young <sean@mess.org> escreveu:

> The LIRC_GET_FEATURES ioctl returns an int which represents features as
> bitwise flags. Two features use duplicate values so they are unusable.
> These are also features which have never been implemented in drivers
> according to kernel/lirc git history, so noone ever noticed that they're
> half-baked.
> 
> LIRC_CAN_NOTIFY_DECODE has the same value as LIRC_CAN_SET_REC_CARRIER, so
> this feature cannot be detected properly. The LIRC_NOTIFY_DECODE ioctl was
> never implemented so remove it. The intent was that a led would be flashed
> when the ioctl is called. IR receivers with a led have this handled via
> the led interface and the rc-feedback led trigger.

The problem with broken API items is that people might be writing some random
code that would use it, like this:

http://sourceforge.net/p/lirc/tickets/_discuss/thread/551aa5b5/d7b7/attachment/lsplugin.c

(from a quick Google search - no, I've no idea if this is used on other
software).

> 
> LIRC_CAN_SET_REC_DUTY_CYCLE has the same value as LIRC_CAN_MEASURE_CARRIER,
> so there is no way to detect it. The idea was that the
> LIRC_SET_REC_DUTY_CYCLE and LIRC_SET_REC_DUTY_CYCLE_RANGE ioctls could be
> used to setup a filter for a lower and upper bound of a duty cycle. No
> hardware has implemented this; why would you.
> 
> Since there never was, or will be, an implementation of these, this is not
> an ABI change.

It is probably safe to get rid of the unused ioctls, but we cannot get
rid of those defines. At most, we could put them into a block like:

#ifndef __KERNEL
/*
 * DON'T USE THOSE!!!
 *
#define...

#endif

Regards,
Mauro

> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  .../DocBook/media/v4l/lirc_device_interface.xml    | 16 ++++------------
>  include/media/lirc.h                               | 22 ++++------------------
>  2 files changed, 8 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/lirc_device_interface.xml b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> index 34cada2..25926bd 100644
> --- a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> +++ b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> @@ -101,7 +101,7 @@ on working with the default settings initially.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> -    <term>LIRC_{G,S}ET_{SEND,REC}_DUTY_CYCLE</term>
> +    <term>LIRC_{G,S}ET_SEND_DUTY_CYCLE</term>
>      <listitem>
>        <para>Get/set the duty cycle (from 0 to 100) of the carrier signal. Currently,
>        no special meaning is defined for 0 or 100, but this could be used to switch
> @@ -204,22 +204,14 @@ on working with the default settings initially.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> -    <term>LIRC_SET_REC_{DUTY_CYCLE,CARRIER}_RANGE</term>
> +    <term>LIRC_SET_REC_CARRIER_RANGE</term>
>      <listitem>
> -      <para>To set a range use LIRC_SET_REC_DUTY_CYCLE_RANGE/LIRC_SET_REC_CARRIER_RANGE
> -      with the lower bound first and later LIRC_SET_REC_DUTY_CYCLE/LIRC_SET_REC_CARRIER
> +      <para>To set a range use LIRC_SET_REC_CARRIER_RANGE
> +      with the lower bound first and later LIRC_SET_REC_CARRIER
>        with the upper bound.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> -    <term>LIRC_NOTIFY_DECODE</term>
> -    <listitem>
> -      <para>This ioctl is called by lircd whenever a successful decoding of an
> -      incoming IR signal could be done. This can be used by supporting hardware
> -      to give visual feedback to the user e.g. by flashing a LED.</para>
> -    </listitem>
> -  </varlistentry>
> -  <varlistentry>
>      <term>LIRC_SETUP_{START,END}</term>
>      <listitem>
>        <para>Setting of several driver parameters can be optimized by encapsulating
> diff --git a/include/media/lirc.h b/include/media/lirc.h
> index 4b3ab29..7b845f8 100644
> --- a/include/media/lirc.h
> +++ b/include/media/lirc.h
> @@ -67,23 +67,17 @@
>  
>  #define LIRC_CAN_REC_MASK              LIRC_MODE2REC(LIRC_CAN_SEND_MASK)
>  
> -#define LIRC_CAN_SET_REC_CARRIER       (LIRC_CAN_SET_SEND_CARRIER << 16)
> -#define LIRC_CAN_SET_REC_DUTY_CYCLE    (LIRC_CAN_SET_SEND_DUTY_CYCLE << 16)
> -
> -#define LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE 0x40000000
>  #define LIRC_CAN_SET_REC_CARRIER_RANGE    0x80000000
>  #define LIRC_CAN_GET_REC_RESOLUTION       0x20000000
>  #define LIRC_CAN_SET_REC_TIMEOUT          0x10000000
>  #define LIRC_CAN_SET_REC_FILTER           0x08000000
> -
> -#define LIRC_CAN_MEASURE_CARRIER          0x02000000
>  #define LIRC_CAN_USE_WIDEBAND_RECEIVER    0x04000000
> +#define LIRC_CAN_MEASURE_CARRIER          0x02000000
> +#define LIRC_CAN_SET_REC_CARRIER          0x01000000
>  
>  #define LIRC_CAN_SEND(x) ((x)&LIRC_CAN_SEND_MASK)
>  #define LIRC_CAN_REC(x) ((x)&LIRC_CAN_REC_MASK)
>  
> -#define LIRC_CAN_NOTIFY_DECODE            0x01000000
> -
>  /*** IOCTL commands for lirc driver ***/
>  
>  #define LIRC_GET_FEATURES              _IOR('i', 0x00000000, __u32)
> @@ -93,7 +87,6 @@
>  #define LIRC_GET_SEND_CARRIER          _IOR('i', 0x00000003, __u32)
>  #define LIRC_GET_REC_CARRIER           _IOR('i', 0x00000004, __u32)
>  #define LIRC_GET_SEND_DUTY_CYCLE       _IOR('i', 0x00000005, __u32)
> -#define LIRC_GET_REC_DUTY_CYCLE        _IOR('i', 0x00000006, __u32)
>  #define LIRC_GET_REC_RESOLUTION        _IOR('i', 0x00000007, __u32)
>  
>  #define LIRC_GET_MIN_TIMEOUT           _IOR('i', 0x00000008, __u32)
> @@ -113,7 +106,6 @@
>  #define LIRC_SET_SEND_CARRIER          _IOW('i', 0x00000013, __u32)
>  #define LIRC_SET_REC_CARRIER           _IOW('i', 0x00000014, __u32)
>  #define LIRC_SET_SEND_DUTY_CYCLE       _IOW('i', 0x00000015, __u32)
> -#define LIRC_SET_REC_DUTY_CYCLE        _IOW('i', 0x00000016, __u32)
>  #define LIRC_SET_TRANSMITTER_MASK      _IOW('i', 0x00000017, __u32)
>  
>  /*
> @@ -149,17 +141,11 @@
>  #define LIRC_SET_MEASURE_CARRIER_MODE	_IOW('i', 0x0000001d, __u32)
>  
>  /*
> - * to set a range use
> - * LIRC_SET_REC_DUTY_CYCLE_RANGE/LIRC_SET_REC_CARRIER_RANGE with the
> - * lower bound first and later
> - * LIRC_SET_REC_DUTY_CYCLE/LIRC_SET_REC_CARRIER with the upper bound
> + * to set a range use LIRC_SET_REC_CARRIER_RANGE with the
> + * lower bound first and later LIRC_SET_REC_CARRIER with the upper bound
>   */
> -
> -#define LIRC_SET_REC_DUTY_CYCLE_RANGE  _IOW('i', 0x0000001e, __u32)
>  #define LIRC_SET_REC_CARRIER_RANGE     _IOW('i', 0x0000001f, __u32)
>  
> -#define LIRC_NOTIFY_DECODE             _IO('i', 0x00000020)
> -
>  #define LIRC_SETUP_START               _IO('i', 0x00000021)
>  #define LIRC_SETUP_END                 _IO('i', 0x00000022)
>  

[RFC PATCH 2/6] [media] lirc: LIRC_[SG]ET_SEND_MODE should return -ENOSYS

[Sean Young]

If the device cannot transmit then -ENOSYS should be returned. Also clarify
that the ioctl should return modes, not features. The values happen to be
identical.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/ir-lirc-codec.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 98893a8..17fd956 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -207,12 +207,19 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 
 	/* legacy support */
 	case LIRC_GET_SEND_MODE:
-		val = LIRC_CAN_SEND_PULSE & LIRC_CAN_SEND_MASK;
+		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
+			return -ENOSYS;
+
+		val = LIRC_MODE_PULSE;
 		break;
 
 	case LIRC_SET_SEND_MODE:
-		if (val != (LIRC_MODE_PULSE & LIRC_CAN_SEND_MASK))
+		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
+			return -ENOSYS;
+
+		if (val != LIRC_MODE_PULSE)
 			return -EINVAL;
+
 		return 0;
 
 	/* TX settings */
-- 
2.1.0

Re: [RFC PATCH 2/6] [media] lirc: LIRC_[SG]ET_SEND_MODE should return -ENOSYS

[Mauro Carvalho Chehab]

Em Thu, 19 Mar 2015 21:50:13 +0000
Sean Young <sean@mess.org> escreveu:

> If the device cannot transmit then -ENOSYS should be returned. Also clarify
> that the ioctl should return modes, not features. The values happen to be
> identical.

Makes sense to me. Yet, applying it (without patch 1) causes compilation to
break.

I would put this at the top of the series, as this actually seems to be
a bug fix that it could eventually make sense to backport.

So, better to keep this patch independent.

> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/ir-lirc-codec.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 98893a8..17fd956 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -207,12 +207,19 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  
>  	/* legacy support */
>  	case LIRC_GET_SEND_MODE:
> -		val = LIRC_CAN_SEND_PULSE & LIRC_CAN_SEND_MASK;
> +		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
> +			return -ENOSYS;
> +
> +		val = LIRC_MODE_PULSE;
>  		break;
>  
>  	case LIRC_SET_SEND_MODE:
> -		if (val != (LIRC_MODE_PULSE & LIRC_CAN_SEND_MASK))
> +		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
> +			return -ENOSYS;
> +
> +		if (val != LIRC_MODE_PULSE)
>  			return -EINVAL;
> +
>  		return 0;
>  
>  	/* TX settings */

[RFC PATCH 3/6] [media] rc: lirc bridge should not be a raw decoder

[Sean Young]

The lirc bridge exists as a raw decoder. We would like to make the bridge
to also work for scancode drivers in further commits, so it cannot be
a raw decoder.

Note that rc-code, lirc_dev, ir-lirc-codec are now calling functions of
each other, so they've been merged into one module rc-core to avoid
circular dependencies.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/Kconfig         |  15 +++--
 drivers/media/rc/Makefile        |   6 +-
 drivers/media/rc/ir-lirc-codec.c | 117 ++++++++++++---------------------------
 drivers/media/rc/lirc_dev.c      |  18 +-----
 drivers/media/rc/rc-core-priv.h  |  38 ++++++-------
 drivers/media/rc/rc-ir-raw.c     |   4 +-
 drivers/media/rc/rc-main.c       |  21 +++++--
 include/media/rc-core.h          |   7 +++
 8 files changed, 92 insertions(+), 134 deletions(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index ddfab25..efdd6f7 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -6,14 +6,8 @@ config RC_CORE
 
 source "drivers/media/rc/keymaps/Kconfig"
 
-menuconfig RC_DECODERS
-        bool "Remote controller decoders"
-	depends on RC_CORE
-	default y
-
-if RC_DECODERS
 config LIRC
-	tristate "LIRC interface driver"
+	bool "LIRC interface driver"
 	depends on RC_CORE
 
 	---help---
@@ -24,7 +18,7 @@ config LIRC
 	   encoding for IR transmitting (aka "blasting").
 
 config IR_LIRC_CODEC
-	tristate "Enable IR to LIRC bridge"
+	bool "Enable IR to LIRC bridge"
 	depends on RC_CORE
 	depends on LIRC
 	default y
@@ -33,7 +27,12 @@ config IR_LIRC_CODEC
 	   Enable this option to pass raw IR to and from userspace via
 	   the LIRC interface.
 
+menuconfig RC_DECODERS
+	bool "Remote controller decoders"
+	depends on RC_CORE
+	default y
 
+if RC_DECODERS
 config IR_NEC_DECODER
 	tristate "Enable IR raw decoder for the NEC protocol"
 	depends on RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 379a5c0..c8e7b38 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -1,9 +1,10 @@
-rc-core-objs	:= rc-main.o rc-ir-raw.o
 
 obj-y += keymaps/
 
 obj-$(CONFIG_RC_CORE) += rc-core.o
-obj-$(CONFIG_LIRC) += lirc_dev.o
+rc-core-y := rc-main.o rc-ir-raw.o
+rc-core-$(CONFIG_LIRC) += lirc_dev.o
+rc-core-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o
 obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
 obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
 obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
@@ -12,7 +13,6 @@ obj-$(CONFIG_IR_SONY_DECODER) += ir-sony-decoder.o
 obj-$(CONFIG_IR_SANYO_DECODER) += ir-sanyo-decoder.o
 obj-$(CONFIG_IR_SHARP_DECODER) += ir-sharp-decoder.o
 obj-$(CONFIG_IR_MCE_KBD_DECODER) += ir-mce_kbd-decoder.o
-obj-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o
 obj-$(CONFIG_IR_XMP_DECODER) += ir-xmp-decoder.o
 
 # stand-alone IR receivers/transmitters
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 17fd956..475f6af 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -14,7 +14,6 @@
 
 #include <linux/sched.h>
 #include <linux/wait.h>
-#include <linux/module.h>
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
 #include <media/rc-core.h>
@@ -30,15 +29,12 @@
  *
  * This function returns -EINVAL if the lirc interfaces aren't wired up.
  */
-static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
+int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 {
-	struct lirc_codec *lirc = &dev->raw->lirc;
+	struct lirc_driver *lirc = dev->lirc;
 	int sample;
 
-	if (!(dev->enabled_protocols & RC_BIT_LIRC))
-		return 0;
-
-	if (!dev->raw->lirc.drv || !dev->raw->lirc.drv->rbuf)
+	if (!lirc || !lirc->rbuf)
 		return -EINVAL;
 
 	/* Packet start */
@@ -59,14 +55,14 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 	/* Packet end */
 	} else if (ev.timeout) {
 
-		if (lirc->gap)
+		if (dev->gap)
 			return 0;
 
-		lirc->gap_start = ktime_get();
-		lirc->gap = true;
-		lirc->gap_duration = ev.duration;
+		dev->gap_start = ktime_get();
+		dev->gap = true;
+		dev->gap_duration = ev.duration;
 
-		if (!lirc->send_timeout_reports)
+		if (!dev->send_timeout_reports)
 			return 0;
 
 		sample = LIRC_TIMEOUT(ev.duration / 1000);
@@ -75,21 +71,21 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 	/* Normal sample */
 	} else {
 
-		if (lirc->gap) {
+		if (dev->gap) {
 			int gap_sample;
 
-			lirc->gap_duration += ktime_to_ns(ktime_sub(ktime_get(),
-				lirc->gap_start));
+			dev->gap_duration += ktime_to_ns(
+				ktime_sub(ktime_get(), dev->gap_start));
 
 			/* Convert to ms and cap by LIRC_VALUE_MASK */
-			do_div(lirc->gap_duration, 1000);
-			lirc->gap_duration = min(lirc->gap_duration,
-							(u64)LIRC_VALUE_MASK);
+			do_div(dev->gap_duration, 1000);
+			dev->gap_duration = min_t(u64, dev->gap_duration,
+							LIRC_VALUE_MASK);
 
-			gap_sample = LIRC_SPACE(lirc->gap_duration);
-			lirc_buffer_write(dev->raw->lirc.drv->rbuf,
+			gap_sample = LIRC_SPACE(dev->gap_duration);
+			lirc_buffer_write(lirc->rbuf,
 						(unsigned char *) &gap_sample);
-			lirc->gap = false;
+			dev->gap = false;
 		}
 
 		sample = ev.pulse ? LIRC_PULSE(ev.duration / 1000) :
@@ -98,9 +94,9 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			   TO_US(ev.duration), TO_STR(ev.pulse));
 	}
 
-	lirc_buffer_write(dev->raw->lirc.drv->rbuf,
+	lirc_buffer_write(lirc->rbuf,
 			  (unsigned char *) &sample);
-	wake_up(&dev->raw->lirc.drv->rbuf->wait_poll);
+	wake_up(&lirc->rbuf->wait_poll);
 
 	return 0;
 }
@@ -108,7 +104,6 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 				   size_t n, loff_t *ppos)
 {
-	struct lirc_codec *lirc;
 	struct rc_dev *dev;
 	unsigned int *txbuf; /* buffer with values to transmit */
 	ssize_t ret = -EINVAL;
@@ -120,8 +115,8 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 
 	start = ktime_get();
 
-	lirc = lirc_get_pdata(file);
-	if (!lirc)
+	dev = lirc_get_pdata(file);
+	if (!dev || !dev->lirc)
 		return -EFAULT;
 
 	if (n < sizeof(unsigned) || n % sizeof(unsigned))
@@ -135,12 +130,6 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 	if (IS_ERR(txbuf))
 		return PTR_ERR(txbuf);
 
-	dev = lirc->dev;
-	if (!dev) {
-		ret = -EFAULT;
-		goto out;
-	}
-
 	if (!dev->tx_ir) {
 		ret = -ENOSYS;
 		goto out;
@@ -183,18 +172,13 @@ out:
 static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 			unsigned long arg)
 {
-	struct lirc_codec *lirc;
 	struct rc_dev *dev;
 	u32 __user *argp = (u32 __user *)(arg);
 	int ret = 0;
 	__u32 val = 0, tmp;
 
-	lirc = lirc_get_pdata(filep);
-	if (!lirc)
-		return -EFAULT;
-
-	dev = lirc->dev;
-	if (!dev)
+	dev  = lirc_get_pdata(filep);
+	if (!dev || !dev->lirc)
 		return -EFAULT;
 
 	if (_IOC_DIR(cmd) & _IOC_WRITE) {
@@ -253,14 +237,14 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 			return -EINVAL;
 
 		return dev->s_rx_carrier_range(dev,
-					       dev->raw->lirc.carrier_low,
+					       dev->carrier_low,
 					       val);
 
 	case LIRC_SET_REC_CARRIER_RANGE:
 		if (val <= 0)
 			return -EINVAL;
 
-		dev->raw->lirc.carrier_low = val;
+		dev->carrier_low = val;
 		return 0;
 
 	case LIRC_GET_REC_RESOLUTION:
@@ -306,7 +290,7 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 		break;
 
 	case LIRC_SET_REC_TIMEOUT_REPORTS:
-		lirc->send_timeout_reports = !!val;
+		dev->send_timeout_reports = !!val;
 		break;
 
 	default:
@@ -343,7 +327,7 @@ static const struct file_operations lirc_fops = {
 	.llseek		= no_llseek,
 };
 
-static int ir_lirc_register(struct rc_dev *dev)
+int ir_lirc_register(struct rc_dev *dev)
 {
 	struct lirc_driver *drv;
 	struct lirc_buffer *rbuf;
@@ -390,7 +374,7 @@ static int ir_lirc_register(struct rc_dev *dev)
 		 dev->driver_name);
 	drv->minor = -1;
 	drv->features = features;
-	drv->data = &dev->raw->lirc;
+	drv->data = dev;
 	drv->rbuf = rbuf;
 	drv->set_use_inc = &ir_lirc_open;
 	drv->set_use_dec = &ir_lirc_close;
@@ -406,8 +390,7 @@ static int ir_lirc_register(struct rc_dev *dev)
 		goto lirc_register_failed;
 	}
 
-	dev->raw->lirc.drv = drv;
-	dev->raw->lirc.dev = dev;
+	dev->lirc = drv;
 	return 0;
 
 lirc_register_failed:
@@ -419,41 +402,11 @@ rbuf_alloc_failed:
 	return rc;
 }
 
-static int ir_lirc_unregister(struct rc_dev *dev)
+void ir_lirc_unregister(struct rc_dev *dev)
 {
-	struct lirc_codec *lirc = &dev->raw->lirc;
-
-	lirc_unregister_driver(lirc->drv->minor);
-	lirc_buffer_free(lirc->drv->rbuf);
-	kfree(lirc->drv);
-
-	return 0;
-}
-
-static struct ir_raw_handler lirc_handler = {
-	.protocols	= RC_BIT_LIRC,
-	.decode		= ir_lirc_decode,
-	.raw_register	= ir_lirc_register,
-	.raw_unregister	= ir_lirc_unregister,
-};
-
-static int __init ir_lirc_codec_init(void)
-{
-	ir_raw_handler_register(&lirc_handler);
-
-	printk(KERN_INFO "IR LIRC bridge handler initialized\n");
-	return 0;
-}
-
-static void __exit ir_lirc_codec_exit(void)
-{
-	ir_raw_handler_unregister(&lirc_handler);
+	if (dev->lirc) {
+		lirc_unregister_driver(dev->lirc->minor);
+		lirc_buffer_free(dev->lirc->rbuf);
+		kfree(dev->lirc);
+	}
 }
-
-module_init(ir_lirc_codec_init);
-module_exit(ir_lirc_codec_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Jarod Wilson <jarod@redhat.com>");
-MODULE_AUTHOR("Red Hat Inc. (http://www.redhat.com)");
-MODULE_DESCRIPTION("LIRC IR handler bridge");
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 4de0e85..44a61e81 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -39,8 +39,6 @@
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
 
-static bool debug;
-
 #define IRCTL_DEV_NAME	"BaseRemoteCtl"
 #define NOPLUG		-1
 #define LOGHEAD		"lirc_dev (%s[%d]): "
@@ -785,8 +783,7 @@ ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
 }
 EXPORT_SYMBOL(lirc_dev_fop_write);
 
-
-static int __init lirc_dev_init(void)
+int __init lirc_dev_init(void)
 {
 	int retval;
 
@@ -813,21 +810,10 @@ error:
 	return retval;
 }
 
-
-
-static void __exit lirc_dev_exit(void)
+void __exit lirc_dev_exit(void)
 {
 	class_destroy(lirc_class);
 	unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
 	printk(KERN_INFO "lirc_dev: module unloaded\n");
 }
 
-module_init(lirc_dev_init);
-module_exit(lirc_dev_exit);
-
-MODULE_DESCRIPTION("LIRC base driver module");
-MODULE_AUTHOR("Artur Lipowski");
-MODULE_LICENSE("GPL");
-
-module_param(debug, bool, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(debug, "Enable debugging messages");
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index b68d4f76..732479d 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -26,7 +26,7 @@ struct ir_raw_handler {
 	u64 protocols; /* which are handled by this handler */
 	int (*decode)(struct rc_dev *dev, struct ir_raw_event event);
 
-	/* These two should only be used by the lirc decoder */
+	/* These two should only be used by the mce kbd decoder */
 	int (*raw_register)(struct rc_dev *dev);
 	int (*raw_unregister)(struct rc_dev *dev);
 };
@@ -99,17 +99,6 @@ struct ir_raw_event_ctrl {
 		unsigned count;
 		unsigned wanted_bits;
 	} mce_kbd;
-	struct lirc_codec {
-		struct rc_dev *dev;
-		struct lirc_driver *drv;
-		int carrier_low;
-
-		ktime_t gap_start;
-		u64 gap_duration;
-		bool gap;
-		bool send_timeout_reports;
-
-	} lirc;
 	struct xmp_dec {
 		int state;
 		unsigned count;
@@ -223,13 +212,6 @@ static inline void load_sharp_decode(void) { }
 static inline void load_mce_kbd_decode(void) { }
 #endif
 
-/* from ir-lirc-codec.c */
-#ifdef CONFIG_IR_LIRC_CODEC_MODULE
-#define load_lirc_codec()	request_module_nowait("ir-lirc-codec")
-#else
-static inline void load_lirc_codec(void) { }
-#endif
-
 /* from ir-xmp-decoder.c */
 #ifdef CONFIG_IR_XMP_DECODER_MODULE
 #define load_xmp_decode()      request_module_nowait("ir-xmp-decoder")
@@ -237,5 +219,23 @@ static inline void load_lirc_codec(void) { }
 static inline void load_xmp_decode(void) { }
 #endif
 
+#ifdef CONFIG_IR_LIRC_CODEC
+void ir_lirc_unregister(struct rc_dev *dev);
+int ir_lirc_register(struct rc_dev *dev);
+int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev);
+#else
+static inline void ir_lirc_unregister(struct rc_dev *dev) {}
+static inline int ir_lirc_register(struct rc_dev *dev) { return 0; }
+static inline int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
+								{ return 0; }
+#endif
+
+#ifdef CONFIG_LIRC
+int __init lirc_dev_init(void);
+void __exit lirc_dev_exit(void);
+#else
+int __init lirc_dev_init(void) { return 0; }
+void __exit lirc_dev_exit(void) {}
+#endif
 
 #endif /* _RC_CORE_PRIV */
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index b732ac6..d298be7 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -57,6 +57,9 @@ static int ir_raw_event_thread(void *data)
 		retval = kfifo_out(&raw->kfifo, &ev, sizeof(ev));
 		spin_unlock_irq(&raw->lock);
 
+		if (raw->dev->lirc)
+			ir_lirc_decode(raw->dev, ev);
+
 		mutex_lock(&ir_raw_handler_lock);
 		list_for_each_entry(handler, &ir_raw_handler_list, list)
 			handler->decode(raw->dev, ev);
@@ -360,7 +363,6 @@ void ir_raw_init(void)
 	load_sanyo_decode();
 	load_sharp_decode();
 	load_mce_kbd_decode();
-	load_lirc_codec();
 	load_xmp_decode();
 
 	/* If needed, we may later add some init code. In this case,
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index f8c5e47..128909c 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -732,7 +732,6 @@ int rc_open(struct rc_dev *rdev)
 
 	return rval;
 }
-EXPORT_SYMBOL_GPL(rc_open);
 
 static int ir_open(struct input_dev *idev)
 {
@@ -752,7 +751,6 @@ void rc_close(struct rc_dev *rdev)
 		mutex_unlock(&rdev->lock);
 	}
 }
-EXPORT_SYMBOL_GPL(rc_close);
 
 static void ir_close(struct input_dev *idev)
 {
@@ -1419,15 +1417,17 @@ int rc_register_device(struct rc_dev *dev)
 		mutex_lock(&dev->lock);
 		if (rc < 0)
 			goto out_input;
+
+		rc = ir_lirc_register(dev);
+		if (rc < 0)
+			goto out_raw;
 	}
 
 	if (dev->change_protocol) {
 		u64 rc_type = (1ll << rc_map->rc_type);
-		if (dev->driver_type == RC_DRIVER_IR_RAW)
-			rc_type |= RC_BIT_LIRC;
 		rc = dev->change_protocol(dev, &rc_type);
 		if (rc < 0)
-			goto out_raw;
+			goto out_lirc;
 		dev->enabled_protocols = rc_type;
 	}
 
@@ -1441,6 +1441,8 @@ int rc_register_device(struct rc_dev *dev)
 
 	return 0;
 
+out_lirc:
+	ir_lirc_unregister(dev);
 out_raw:
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		ir_raw_event_unregister(dev);
@@ -1470,6 +1472,8 @@ void rc_unregister_device(struct rc_dev *dev)
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		ir_raw_event_unregister(dev);
 
+	ir_lirc_unregister(dev);
+
 	/* Freeing the table should also call the stop callback */
 	ir_free_table(&dev->rc_map);
 	IR_dprintk(1, "Freed keycode table\n");
@@ -1495,6 +1499,12 @@ static int __init rc_core_init(void)
 		printk(KERN_ERR "rc_core: unable to register rc class\n");
 		return rc;
 	}
+	rc = lirc_dev_init();
+	if (rc) {
+		class_unregister(&rc_class);
+		printk(KERN_ERR "rc_core: unable to register lirc class\n");
+		return rc;
+	}
 
 	led_trigger_register_simple("rc-feedback", &led_feedback);
 	rc_map_register(&empty_map);
@@ -1507,6 +1517,7 @@ static void __exit rc_core_exit(void)
 	class_unregister(&rc_class);
 	led_trigger_unregister_simple(led_feedback);
 	rc_map_unregister(&empty_map);
+	lirc_dev_exit();
 }
 
 subsys_initcall(rc_core_init);
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 2c7fbca..e3f217c 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -156,6 +156,13 @@ struct rc_dev {
 	u32				max_timeout;
 	u32				rx_resolution;
 	u32				tx_resolution;
+	/* lirc state */
+	struct lirc_driver		*lirc;
+	int				carrier_low;
+	ktime_t				gap_start;
+	u64				gap_duration;
+	bool				gap;
+	bool				send_timeout_reports;
 	int				(*change_protocol)(struct rc_dev *dev, u64 *rc_type);
 	int				(*change_wakeup_protocol)(struct rc_dev *dev, u64 *rc_type);
 	int				(*open)(struct rc_dev *dev);
-- 
2.1.0

Re: [RFC PATCH 3/6] [media] rc: lirc bridge should not be a raw decoder

[Mauro Carvalho Chehab]

Em Thu, 19 Mar 2015 21:50:14 +0000
Sean Young <sean@mess.org> escreveu:

> The lirc bridge exists as a raw decoder. We would like to make the bridge
> to also work for scancode drivers in further commits, so it cannot be
> a raw decoder.
> 
> Note that rc-code, lirc_dev, ir-lirc-codec are now calling functions of
> each other, so they've been merged into one module rc-core to avoid
> circular dependencies.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/Kconfig         |  15 +++--
>  drivers/media/rc/Makefile        |   6 +-
>  drivers/media/rc/ir-lirc-codec.c | 117 ++++++++++++---------------------------
>  drivers/media/rc/lirc_dev.c      |  18 +-----
>  drivers/media/rc/rc-core-priv.h  |  38 ++++++-------
>  drivers/media/rc/rc-ir-raw.c     |   4 +-
>  drivers/media/rc/rc-main.c       |  21 +++++--
>  include/media/rc-core.h          |   7 +++
>  8 files changed, 92 insertions(+), 134 deletions(-)
> 
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index ddfab25..efdd6f7 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -6,14 +6,8 @@ config RC_CORE
>  
>  source "drivers/media/rc/keymaps/Kconfig"
>  
> -menuconfig RC_DECODERS
> -        bool "Remote controller decoders"
> -	depends on RC_CORE
> -	default y
> -
> -if RC_DECODERS
>  config LIRC
> -	tristate "LIRC interface driver"
> +	bool "LIRC interface driver"
>  	depends on RC_CORE
>  
>  	---help---
> @@ -24,7 +18,7 @@ config LIRC
>  	   encoding for IR transmitting (aka "blasting").
>  
>  config IR_LIRC_CODEC
> -	tristate "Enable IR to LIRC bridge"
> +	bool "Enable IR to LIRC bridge"
>  	depends on RC_CORE
>  	depends on LIRC
>  	default y
> @@ -33,7 +27,12 @@ config IR_LIRC_CODEC
>  	   Enable this option to pass raw IR to and from userspace via
>  	   the LIRC interface.
>  
> +menuconfig RC_DECODERS
> +	bool "Remote controller decoders"
> +	depends on RC_CORE
> +	default y
>  
> +if RC_DECODERS
>  config IR_NEC_DECODER
>  	tristate "Enable IR raw decoder for the NEC protocol"
>  	depends on RC_CORE
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index 379a5c0..c8e7b38 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -1,9 +1,10 @@
> -rc-core-objs	:= rc-main.o rc-ir-raw.o
>  
>  obj-y += keymaps/
>  
>  obj-$(CONFIG_RC_CORE) += rc-core.o
> -obj-$(CONFIG_LIRC) += lirc_dev.o
> +rc-core-y := rc-main.o rc-ir-raw.o
> +rc-core-$(CONFIG_LIRC) += lirc_dev.o
> +rc-core-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o

It seems OK to me to not consider LIRC as a decoder, but I wouldn't add
it inside RC core. The best seems to convert lirc dev to be a 
separate module.

Thanks!
Mauro


>  obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
>  obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
>  obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
> @@ -12,7 +13,6 @@ obj-$(CONFIG_IR_SONY_DECODER) += ir-sony-decoder.o
>  obj-$(CONFIG_IR_SANYO_DECODER) += ir-sanyo-decoder.o
>  obj-$(CONFIG_IR_SHARP_DECODER) += ir-sharp-decoder.o
>  obj-$(CONFIG_IR_MCE_KBD_DECODER) += ir-mce_kbd-decoder.o
> -obj-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o
>  obj-$(CONFIG_IR_XMP_DECODER) += ir-xmp-decoder.o
>  
>  # stand-alone IR receivers/transmitters
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 17fd956..475f6af 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -14,7 +14,6 @@
>  
>  #include <linux/sched.h>
>  #include <linux/wait.h>
> -#include <linux/module.h>
>  #include <media/lirc.h>
>  #include <media/lirc_dev.h>
>  #include <media/rc-core.h>
> @@ -30,15 +29,12 @@
>   *
>   * This function returns -EINVAL if the lirc interfaces aren't wired up.
>   */
> -static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
> +int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  {
> -	struct lirc_codec *lirc = &dev->raw->lirc;
> +	struct lirc_driver *lirc = dev->lirc;
>  	int sample;
>  
> -	if (!(dev->enabled_protocols & RC_BIT_LIRC))
> -		return 0;
> -
> -	if (!dev->raw->lirc.drv || !dev->raw->lirc.drv->rbuf)
> +	if (!lirc || !lirc->rbuf)
>  		return -EINVAL;
>  
>  	/* Packet start */
> @@ -59,14 +55,14 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  	/* Packet end */
>  	} else if (ev.timeout) {
>  
> -		if (lirc->gap)
> +		if (dev->gap)
>  			return 0;
>  
> -		lirc->gap_start = ktime_get();
> -		lirc->gap = true;
> -		lirc->gap_duration = ev.duration;
> +		dev->gap_start = ktime_get();
> +		dev->gap = true;
> +		dev->gap_duration = ev.duration;
>  
> -		if (!lirc->send_timeout_reports)
> +		if (!dev->send_timeout_reports)
>  			return 0;
>  
>  		sample = LIRC_TIMEOUT(ev.duration / 1000);
> @@ -75,21 +71,21 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  	/* Normal sample */
>  	} else {
>  
> -		if (lirc->gap) {
> +		if (dev->gap) {
>  			int gap_sample;
>  
> -			lirc->gap_duration += ktime_to_ns(ktime_sub(ktime_get(),
> -				lirc->gap_start));
> +			dev->gap_duration += ktime_to_ns(
> +				ktime_sub(ktime_get(), dev->gap_start));
>  
>  			/* Convert to ms and cap by LIRC_VALUE_MASK */
> -			do_div(lirc->gap_duration, 1000);
> -			lirc->gap_duration = min(lirc->gap_duration,
> -							(u64)LIRC_VALUE_MASK);
> +			do_div(dev->gap_duration, 1000);
> +			dev->gap_duration = min_t(u64, dev->gap_duration,
> +							LIRC_VALUE_MASK);
>  
> -			gap_sample = LIRC_SPACE(lirc->gap_duration);
> -			lirc_buffer_write(dev->raw->lirc.drv->rbuf,
> +			gap_sample = LIRC_SPACE(dev->gap_duration);
> +			lirc_buffer_write(lirc->rbuf,
>  						(unsigned char *) &gap_sample);
> -			lirc->gap = false;
> +			dev->gap = false;
>  		}
>  
>  		sample = ev.pulse ? LIRC_PULSE(ev.duration / 1000) :
> @@ -98,9 +94,9 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  			   TO_US(ev.duration), TO_STR(ev.pulse));
>  	}
>  
> -	lirc_buffer_write(dev->raw->lirc.drv->rbuf,
> +	lirc_buffer_write(lirc->rbuf,
>  			  (unsigned char *) &sample);
> -	wake_up(&dev->raw->lirc.drv->rbuf->wait_poll);
> +	wake_up(&lirc->rbuf->wait_poll);
>  
>  	return 0;
>  }
> @@ -108,7 +104,6 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  				   size_t n, loff_t *ppos)
>  {
> -	struct lirc_codec *lirc;
>  	struct rc_dev *dev;
>  	unsigned int *txbuf; /* buffer with values to transmit */
>  	ssize_t ret = -EINVAL;
> @@ -120,8 +115,8 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  
>  	start = ktime_get();
>  
> -	lirc = lirc_get_pdata(file);
> -	if (!lirc)
> +	dev = lirc_get_pdata(file);
> +	if (!dev || !dev->lirc)
>  		return -EFAULT;
>  
>  	if (n < sizeof(unsigned) || n % sizeof(unsigned))
> @@ -135,12 +130,6 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  	if (IS_ERR(txbuf))
>  		return PTR_ERR(txbuf);
>  
> -	dev = lirc->dev;
> -	if (!dev) {
> -		ret = -EFAULT;
> -		goto out;
> -	}
> -
>  	if (!dev->tx_ir) {
>  		ret = -ENOSYS;
>  		goto out;
> @@ -183,18 +172,13 @@ out:
>  static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  			unsigned long arg)
>  {
> -	struct lirc_codec *lirc;
>  	struct rc_dev *dev;
>  	u32 __user *argp = (u32 __user *)(arg);
>  	int ret = 0;
>  	__u32 val = 0, tmp;
>  
> -	lirc = lirc_get_pdata(filep);
> -	if (!lirc)
> -		return -EFAULT;
> -
> -	dev = lirc->dev;
> -	if (!dev)
> +	dev  = lirc_get_pdata(filep);
> +	if (!dev || !dev->lirc)
>  		return -EFAULT;
>  
>  	if (_IOC_DIR(cmd) & _IOC_WRITE) {
> @@ -253,14 +237,14 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  			return -EINVAL;
>  
>  		return dev->s_rx_carrier_range(dev,
> -					       dev->raw->lirc.carrier_low,
> +					       dev->carrier_low,
>  					       val);
>  
>  	case LIRC_SET_REC_CARRIER_RANGE:
>  		if (val <= 0)
>  			return -EINVAL;
>  
> -		dev->raw->lirc.carrier_low = val;
> +		dev->carrier_low = val;
>  		return 0;
>  
>  	case LIRC_GET_REC_RESOLUTION:
> @@ -306,7 +290,7 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  		break;
>  
>  	case LIRC_SET_REC_TIMEOUT_REPORTS:
> -		lirc->send_timeout_reports = !!val;
> +		dev->send_timeout_reports = !!val;
>  		break;
>  
>  	default:
> @@ -343,7 +327,7 @@ static const struct file_operations lirc_fops = {
>  	.llseek		= no_llseek,
>  };
>  
> -static int ir_lirc_register(struct rc_dev *dev)
> +int ir_lirc_register(struct rc_dev *dev)
>  {
>  	struct lirc_driver *drv;
>  	struct lirc_buffer *rbuf;
> @@ -390,7 +374,7 @@ static int ir_lirc_register(struct rc_dev *dev)
>  		 dev->driver_name);
>  	drv->minor = -1;
>  	drv->features = features;
> -	drv->data = &dev->raw->lirc;
> +	drv->data = dev;
>  	drv->rbuf = rbuf;
>  	drv->set_use_inc = &ir_lirc_open;
>  	drv->set_use_dec = &ir_lirc_close;
> @@ -406,8 +390,7 @@ static int ir_lirc_register(struct rc_dev *dev)
>  		goto lirc_register_failed;
>  	}
>  
> -	dev->raw->lirc.drv = drv;
> -	dev->raw->lirc.dev = dev;
> +	dev->lirc = drv;
>  	return 0;
>  
>  lirc_register_failed:
> @@ -419,41 +402,11 @@ rbuf_alloc_failed:
>  	return rc;
>  }
>  
> -static int ir_lirc_unregister(struct rc_dev *dev)
> +void ir_lirc_unregister(struct rc_dev *dev)
>  {
> -	struct lirc_codec *lirc = &dev->raw->lirc;
> -
> -	lirc_unregister_driver(lirc->drv->minor);
> -	lirc_buffer_free(lirc->drv->rbuf);
> -	kfree(lirc->drv);
> -
> -	return 0;
> -}
> -
> -static struct ir_raw_handler lirc_handler = {
> -	.protocols	= RC_BIT_LIRC,
> -	.decode		= ir_lirc_decode,
> -	.raw_register	= ir_lirc_register,
> -	.raw_unregister	= ir_lirc_unregister,
> -};
> -
> -static int __init ir_lirc_codec_init(void)
> -{
> -	ir_raw_handler_register(&lirc_handler);
> -
> -	printk(KERN_INFO "IR LIRC bridge handler initialized\n");
> -	return 0;
> -}
> -
> -static void __exit ir_lirc_codec_exit(void)
> -{
> -	ir_raw_handler_unregister(&lirc_handler);
> +	if (dev->lirc) {
> +		lirc_unregister_driver(dev->lirc->minor);
> +		lirc_buffer_free(dev->lirc->rbuf);
> +		kfree(dev->lirc);
> +	}
>  }
> -
> -module_init(ir_lirc_codec_init);
> -module_exit(ir_lirc_codec_exit);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Jarod Wilson <jarod@redhat.com>");
> -MODULE_AUTHOR("Red Hat Inc. (http://www.redhat.com)");
> -MODULE_DESCRIPTION("LIRC IR handler bridge");
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 4de0e85..44a61e81 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -39,8 +39,6 @@
>  #include <media/lirc.h>
>  #include <media/lirc_dev.h>
>  
> -static bool debug;
> -
>  #define IRCTL_DEV_NAME	"BaseRemoteCtl"
>  #define NOPLUG		-1
>  #define LOGHEAD		"lirc_dev (%s[%d]): "
> @@ -785,8 +783,7 @@ ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
>  }
>  EXPORT_SYMBOL(lirc_dev_fop_write);
>  
> -
> -static int __init lirc_dev_init(void)
> +int __init lirc_dev_init(void)
>  {
>  	int retval;
>  
> @@ -813,21 +810,10 @@ error:
>  	return retval;
>  }
>  
> -
> -
> -static void __exit lirc_dev_exit(void)
> +void __exit lirc_dev_exit(void)
>  {
>  	class_destroy(lirc_class);
>  	unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
>  	printk(KERN_INFO "lirc_dev: module unloaded\n");
>  }
>  
> -module_init(lirc_dev_init);
> -module_exit(lirc_dev_exit);
> -
> -MODULE_DESCRIPTION("LIRC base driver module");
> -MODULE_AUTHOR("Artur Lipowski");
> -MODULE_LICENSE("GPL");
> -
> -module_param(debug, bool, S_IRUGO | S_IWUSR);
> -MODULE_PARM_DESC(debug, "Enable debugging messages");
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index b68d4f76..732479d 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -26,7 +26,7 @@ struct ir_raw_handler {
>  	u64 protocols; /* which are handled by this handler */
>  	int (*decode)(struct rc_dev *dev, struct ir_raw_event event);
>  
> -	/* These two should only be used by the lirc decoder */
> +	/* These two should only be used by the mce kbd decoder */
>  	int (*raw_register)(struct rc_dev *dev);
>  	int (*raw_unregister)(struct rc_dev *dev);
>  };
> @@ -99,17 +99,6 @@ struct ir_raw_event_ctrl {
>  		unsigned count;
>  		unsigned wanted_bits;
>  	} mce_kbd;
> -	struct lirc_codec {
> -		struct rc_dev *dev;
> -		struct lirc_driver *drv;
> -		int carrier_low;
> -
> -		ktime_t gap_start;
> -		u64 gap_duration;
> -		bool gap;
> -		bool send_timeout_reports;
> -
> -	} lirc;
>  	struct xmp_dec {
>  		int state;
>  		unsigned count;
> @@ -223,13 +212,6 @@ static inline void load_sharp_decode(void) { }
>  static inline void load_mce_kbd_decode(void) { }
>  #endif
>  
> -/* from ir-lirc-codec.c */
> -#ifdef CONFIG_IR_LIRC_CODEC_MODULE
> -#define load_lirc_codec()	request_module_nowait("ir-lirc-codec")
> -#else
> -static inline void load_lirc_codec(void) { }
> -#endif
> -
>  /* from ir-xmp-decoder.c */
>  #ifdef CONFIG_IR_XMP_DECODER_MODULE
>  #define load_xmp_decode()      request_module_nowait("ir-xmp-decoder")
> @@ -237,5 +219,23 @@ static inline void load_lirc_codec(void) { }
>  static inline void load_xmp_decode(void) { }
>  #endif
>  
> +#ifdef CONFIG_IR_LIRC_CODEC
> +void ir_lirc_unregister(struct rc_dev *dev);
> +int ir_lirc_register(struct rc_dev *dev);
> +int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev);
> +#else
> +static inline void ir_lirc_unregister(struct rc_dev *dev) {}
> +static inline int ir_lirc_register(struct rc_dev *dev) { return 0; }
> +static inline int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
> +								{ return 0; }
> +#endif
> +
> +#ifdef CONFIG_LIRC
> +int __init lirc_dev_init(void);
> +void __exit lirc_dev_exit(void);
> +#else
> +int __init lirc_dev_init(void) { return 0; }
> +void __exit lirc_dev_exit(void) {}
> +#endif
>  
>  #endif /* _RC_CORE_PRIV */
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index b732ac6..d298be7 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -57,6 +57,9 @@ static int ir_raw_event_thread(void *data)
>  		retval = kfifo_out(&raw->kfifo, &ev, sizeof(ev));
>  		spin_unlock_irq(&raw->lock);
>  
> +		if (raw->dev->lirc)
> +			ir_lirc_decode(raw->dev, ev);
> +
>  		mutex_lock(&ir_raw_handler_lock);
>  		list_for_each_entry(handler, &ir_raw_handler_list, list)
>  			handler->decode(raw->dev, ev);
> @@ -360,7 +363,6 @@ void ir_raw_init(void)
>  	load_sanyo_decode();
>  	load_sharp_decode();
>  	load_mce_kbd_decode();
> -	load_lirc_codec();
>  	load_xmp_decode();
>  
>  	/* If needed, we may later add some init code. In this case,
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index f8c5e47..128909c 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -732,7 +732,6 @@ int rc_open(struct rc_dev *rdev)
>  
>  	return rval;
>  }
> -EXPORT_SYMBOL_GPL(rc_open);
>  
>  static int ir_open(struct input_dev *idev)
>  {
> @@ -752,7 +751,6 @@ void rc_close(struct rc_dev *rdev)
>  		mutex_unlock(&rdev->lock);
>  	}
>  }
> -EXPORT_SYMBOL_GPL(rc_close);
>  
>  static void ir_close(struct input_dev *idev)
>  {
> @@ -1419,15 +1417,17 @@ int rc_register_device(struct rc_dev *dev)
>  		mutex_lock(&dev->lock);
>  		if (rc < 0)
>  			goto out_input;
> +
> +		rc = ir_lirc_register(dev);
> +		if (rc < 0)
> +			goto out_raw;
>  	}
>  
>  	if (dev->change_protocol) {
>  		u64 rc_type = (1ll << rc_map->rc_type);
> -		if (dev->driver_type == RC_DRIVER_IR_RAW)
> -			rc_type |= RC_BIT_LIRC;
>  		rc = dev->change_protocol(dev, &rc_type);
>  		if (rc < 0)
> -			goto out_raw;
> +			goto out_lirc;
>  		dev->enabled_protocols = rc_type;
>  	}
>  
> @@ -1441,6 +1441,8 @@ int rc_register_device(struct rc_dev *dev)
>  
>  	return 0;
>  
> +out_lirc:
> +	ir_lirc_unregister(dev);
>  out_raw:
>  	if (dev->driver_type == RC_DRIVER_IR_RAW)
>  		ir_raw_event_unregister(dev);
> @@ -1470,6 +1472,8 @@ void rc_unregister_device(struct rc_dev *dev)
>  	if (dev->driver_type == RC_DRIVER_IR_RAW)
>  		ir_raw_event_unregister(dev);
>  
> +	ir_lirc_unregister(dev);
> +
>  	/* Freeing the table should also call the stop callback */
>  	ir_free_table(&dev->rc_map);
>  	IR_dprintk(1, "Freed keycode table\n");
> @@ -1495,6 +1499,12 @@ static int __init rc_core_init(void)
>  		printk(KERN_ERR "rc_core: unable to register rc class\n");
>  		return rc;
>  	}
> +	rc = lirc_dev_init();
> +	if (rc) {
> +		class_unregister(&rc_class);
> +		printk(KERN_ERR "rc_core: unable to register lirc class\n");
> +		return rc;
> +	}
>  
>  	led_trigger_register_simple("rc-feedback", &led_feedback);
>  	rc_map_register(&empty_map);
> @@ -1507,6 +1517,7 @@ static void __exit rc_core_exit(void)
>  	class_unregister(&rc_class);
>  	led_trigger_unregister_simple(led_feedback);
>  	rc_map_unregister(&empty_map);
> +	lirc_dev_exit();
>  }
>  
>  subsys_initcall(rc_core_init);
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index 2c7fbca..e3f217c 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -156,6 +156,13 @@ struct rc_dev {
>  	u32				max_timeout;
>  	u32				rx_resolution;
>  	u32				tx_resolution;
> +	/* lirc state */
> +	struct lirc_driver		*lirc;
> +	int				carrier_low;
> +	ktime_t				gap_start;
> +	u64				gap_duration;
> +	bool				gap;
> +	bool				send_timeout_reports;
>  	int				(*change_protocol)(struct rc_dev *dev, u64 *rc_type);
>  	int				(*change_wakeup_protocol)(struct rc_dev *dev, u64 *rc_type);
>  	int				(*open)(struct rc_dev *dev);

[RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[Sean Young]

Since the lirc bridge is not a decoder we can remove its protocol. The
keymap existed only to select the protocol.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/keymaps/Makefile  |  1 -
 drivers/media/rc/keymaps/rc-lirc.c | 42 --------------------------------------
 drivers/media/rc/rc-main.c         |  1 -
 drivers/media/rc/st_rc.c           |  2 +-
 include/media/rc-map.h             | 42 +++++++++++++++++---------------------
 5 files changed, 20 insertions(+), 68 deletions(-)
 delete mode 100644 drivers/media/rc/keymaps/rc-lirc.c

diff --git a/drivers/media/rc/keymaps/Makefile b/drivers/media/rc/keymaps/Makefile
index abf6079..661cd25 100644
--- a/drivers/media/rc/keymaps/Makefile
+++ b/drivers/media/rc/keymaps/Makefile
@@ -51,7 +51,6 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
 			rc-kworld-pc150u.o \
 			rc-kworld-plus-tv-analog.o \
 			rc-leadtek-y04g0051.o \
-			rc-lirc.o \
 			rc-lme2510.o \
 			rc-manli.o \
 			rc-medion-x10.o \
diff --git a/drivers/media/rc/keymaps/rc-lirc.c b/drivers/media/rc/keymaps/rc-lirc.c
deleted file mode 100644
index fbf08fa..0000000
--- a/drivers/media/rc/keymaps/rc-lirc.c
+++ /dev/null
@@ -1,42 +0,0 @@
-/* rc-lirc.c - Empty dummy keytable, for use when its preferred to pass
- * all raw IR data to the lirc userspace decoder.
- *
- * Copyright (c) 2010 by Jarod Wilson <jarod@redhat.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#include <media/rc-core.h>
-#include <linux/module.h>
-
-static struct rc_map_table lirc[] = {
-	{ },
-};
-
-static struct rc_map_list lirc_map = {
-	.map = {
-		.scan    = lirc,
-		.size    = ARRAY_SIZE(lirc),
-		.rc_type = RC_TYPE_LIRC,
-		.name    = RC_MAP_LIRC,
-	}
-};
-
-static int __init init_rc_map_lirc(void)
-{
-	return rc_map_register(&lirc_map);
-}
-
-static void __exit exit_rc_map_lirc(void)
-{
-	rc_map_unregister(&lirc_map);
-}
-
-module_init(init_rc_map_lirc)
-module_exit(exit_rc_map_lirc)
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Jarod Wilson <jarod@redhat.com>");
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 128909c..e717dc9 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -797,7 +797,6 @@ static struct {
 	{ RC_BIT_SANYO,		"sanyo"		},
 	{ RC_BIT_SHARP,		"sharp"		},
 	{ RC_BIT_MCE_KBD,	"mce_kbd"	},
-	{ RC_BIT_LIRC,		"lirc"		},
 	{ RC_BIT_XMP,		"xmp"		},
 };
 
diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
index 0e758ae..4834e78 100644
--- a/drivers/media/rc/st_rc.c
+++ b/drivers/media/rc/st_rc.c
@@ -295,7 +295,7 @@ static int st_rc_probe(struct platform_device *pdev)
 	rdev->open = st_rc_open;
 	rdev->close = st_rc_close;
 	rdev->driver_name = IR_ST_NAME;
-	rdev->map_name = RC_MAP_LIRC;
+	rdev->map_name = RC_MAP_EMPTY;
 	rdev->input_name = "ST Remote Control Receiver";
 
 	/* enable wake via this device */
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index e7a1514..dfca14b 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -14,30 +14,28 @@
 enum rc_type {
 	RC_TYPE_UNKNOWN		= 0,	/* Protocol not known */
 	RC_TYPE_OTHER		= 1,	/* Protocol known but proprietary */
-	RC_TYPE_LIRC		= 2,	/* Pass raw IR to lirc userspace */
-	RC_TYPE_RC5		= 3,	/* Philips RC5 protocol */
-	RC_TYPE_RC5X		= 4,	/* Philips RC5x protocol */
-	RC_TYPE_RC5_SZ		= 5,	/* StreamZap variant of RC5 */
-	RC_TYPE_JVC		= 6,	/* JVC protocol */
-	RC_TYPE_SONY12		= 7,	/* Sony 12 bit protocol */
-	RC_TYPE_SONY15		= 8,	/* Sony 15 bit protocol */
-	RC_TYPE_SONY20		= 9,	/* Sony 20 bit protocol */
-	RC_TYPE_NEC		= 10,	/* NEC protocol */
-	RC_TYPE_SANYO		= 11,	/* Sanyo protocol */
-	RC_TYPE_MCE_KBD		= 12,	/* RC6-ish MCE keyboard/mouse */
-	RC_TYPE_RC6_0		= 13,	/* Philips RC6-0-16 protocol */
-	RC_TYPE_RC6_6A_20	= 14,	/* Philips RC6-6A-20 protocol */
-	RC_TYPE_RC6_6A_24	= 15,	/* Philips RC6-6A-24 protocol */
-	RC_TYPE_RC6_6A_32	= 16,	/* Philips RC6-6A-32 protocol */
-	RC_TYPE_RC6_MCE		= 17,	/* MCE (Philips RC6-6A-32 subtype) protocol */
-	RC_TYPE_SHARP		= 18,	/* Sharp protocol */
-	RC_TYPE_XMP		= 19,	/* XMP protocol */
+	RC_TYPE_RC5		= 2,	/* Philips RC5 protocol */
+	RC_TYPE_RC5X		= 3,	/* Philips RC5x protocol */
+	RC_TYPE_RC5_SZ		= 4,	/* StreamZap variant of RC5 */
+	RC_TYPE_JVC		= 5,	/* JVC protocol */
+	RC_TYPE_SONY12		= 6,	/* Sony 12 bit protocol */
+	RC_TYPE_SONY15		= 7,	/* Sony 15 bit protocol */
+	RC_TYPE_SONY20		= 8,	/* Sony 20 bit protocol */
+	RC_TYPE_NEC		= 9,	/* NEC protocol */
+	RC_TYPE_SANYO		= 10,	/* Sanyo protocol */
+	RC_TYPE_MCE_KBD		= 11,	/* RC6-ish MCE keyboard/mouse */
+	RC_TYPE_RC6_0		= 12,	/* Philips RC6-0-16 protocol */
+	RC_TYPE_RC6_6A_20	= 13,	/* Philips RC6-6A-20 protocol */
+	RC_TYPE_RC6_6A_24	= 14,	/* Philips RC6-6A-24 protocol */
+	RC_TYPE_RC6_6A_32	= 15,	/* Philips RC6-6A-32 protocol */
+	RC_TYPE_RC6_MCE		= 16,	/* MCE (Philips RC6-6A-32 subtype) protocol */
+	RC_TYPE_SHARP		= 17,	/* Sharp protocol */
+	RC_TYPE_XMP		= 18,	/* XMP protocol */
 };
 
 #define RC_BIT_NONE		0
 #define RC_BIT_UNKNOWN		(1 << RC_TYPE_UNKNOWN)
 #define RC_BIT_OTHER		(1 << RC_TYPE_OTHER)
-#define RC_BIT_LIRC		(1 << RC_TYPE_LIRC)
 #define RC_BIT_RC5		(1 << RC_TYPE_RC5)
 #define RC_BIT_RC5X		(1 << RC_TYPE_RC5X)
 #define RC_BIT_RC5_SZ		(1 << RC_TYPE_RC5_SZ)
@@ -56,9 +54,8 @@ enum rc_type {
 #define RC_BIT_SHARP		(1 << RC_TYPE_SHARP)
 #define RC_BIT_XMP		(1 << RC_TYPE_XMP)
 
-#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_LIRC | \
-			 RC_BIT_RC5 | RC_BIT_RC5X | RC_BIT_RC5_SZ | \
-			 RC_BIT_JVC | \
+#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_RC5 | \
+			 RC_BIT_RC5X | RC_BIT_RC5_SZ | RC_BIT_JVC | \
 			 RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
 			 RC_BIT_NEC | RC_BIT_SANYO | RC_BIT_MCE_KBD | \
 			 RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
@@ -160,7 +157,6 @@ void rc_map_init(void);
 #define RC_MAP_KWORLD_PC150U             "rc-kworld-pc150u"
 #define RC_MAP_KWORLD_PLUS_TV_ANALOG     "rc-kworld-plus-tv-analog"
 #define RC_MAP_LEADTEK_Y04G0051          "rc-leadtek-y04g0051"
-#define RC_MAP_LIRC                      "rc-lirc"
 #define RC_MAP_LME2510                   "rc-lme2510"
 #define RC_MAP_MANLI                     "rc-manli"
 #define RC_MAP_MEDION_X10                "rc-medion-x10"
-- 
2.1.0

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[Mauro Carvalho Chehab]

Em Thu, 19 Mar 2015 21:50:15 +0000
Sean Young <sean@mess.org> escreveu:

> Since the lirc bridge is not a decoder we can remove its protocol. The
> keymap existed only to select the protocol.

This changes the userspace interface, as now it is possible to enable/disable
LIRC handling from a given IR via /proc interface.

> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/keymaps/Makefile  |  1 -
>  drivers/media/rc/keymaps/rc-lirc.c | 42 --------------------------------------
>  drivers/media/rc/rc-main.c         |  1 -
>  drivers/media/rc/st_rc.c           |  2 +-
>  include/media/rc-map.h             | 42 +++++++++++++++++---------------------
>  5 files changed, 20 insertions(+), 68 deletions(-)
>  delete mode 100644 drivers/media/rc/keymaps/rc-lirc.c
> 
> diff --git a/drivers/media/rc/keymaps/Makefile b/drivers/media/rc/keymaps/Makefile
> index abf6079..661cd25 100644
> --- a/drivers/media/rc/keymaps/Makefile
> +++ b/drivers/media/rc/keymaps/Makefile
> @@ -51,7 +51,6 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
>  			rc-kworld-pc150u.o \
>  			rc-kworld-plus-tv-analog.o \
>  			rc-leadtek-y04g0051.o \
> -			rc-lirc.o \
>  			rc-lme2510.o \
>  			rc-manli.o \
>  			rc-medion-x10.o \
> diff --git a/drivers/media/rc/keymaps/rc-lirc.c b/drivers/media/rc/keymaps/rc-lirc.c
> deleted file mode 100644
> index fbf08fa..0000000
> --- a/drivers/media/rc/keymaps/rc-lirc.c
> +++ /dev/null
> @@ -1,42 +0,0 @@
> -/* rc-lirc.c - Empty dummy keytable, for use when its preferred to pass
> - * all raw IR data to the lirc userspace decoder.
> - *
> - * Copyright (c) 2010 by Jarod Wilson <jarod@redhat.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - */
> -
> -#include <media/rc-core.h>
> -#include <linux/module.h>
> -
> -static struct rc_map_table lirc[] = {
> -	{ },
> -};
> -
> -static struct rc_map_list lirc_map = {
> -	.map = {
> -		.scan    = lirc,
> -		.size    = ARRAY_SIZE(lirc),
> -		.rc_type = RC_TYPE_LIRC,
> -		.name    = RC_MAP_LIRC,
> -	}
> -};
> -
> -static int __init init_rc_map_lirc(void)
> -{
> -	return rc_map_register(&lirc_map);
> -}
> -
> -static void __exit exit_rc_map_lirc(void)
> -{
> -	rc_map_unregister(&lirc_map);
> -}
> -
> -module_init(init_rc_map_lirc)
> -module_exit(exit_rc_map_lirc)
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Jarod Wilson <jarod@redhat.com>");
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 128909c..e717dc9 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -797,7 +797,6 @@ static struct {
>  	{ RC_BIT_SANYO,		"sanyo"		},
>  	{ RC_BIT_SHARP,		"sharp"		},
>  	{ RC_BIT_MCE_KBD,	"mce_kbd"	},
> -	{ RC_BIT_LIRC,		"lirc"		},
>  	{ RC_BIT_XMP,		"xmp"		},
>  };
>  
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index 0e758ae..4834e78 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -295,7 +295,7 @@ static int st_rc_probe(struct platform_device *pdev)
>  	rdev->open = st_rc_open;
>  	rdev->close = st_rc_close;
>  	rdev->driver_name = IR_ST_NAME;
> -	rdev->map_name = RC_MAP_LIRC;
> +	rdev->map_name = RC_MAP_EMPTY;
>  	rdev->input_name = "ST Remote Control Receiver";
>  
>  	/* enable wake via this device */
> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> index e7a1514..dfca14b 100644
> --- a/include/media/rc-map.h
> +++ b/include/media/rc-map.h
> @@ -14,30 +14,28 @@
>  enum rc_type {
>  	RC_TYPE_UNKNOWN		= 0,	/* Protocol not known */
>  	RC_TYPE_OTHER		= 1,	/* Protocol known but proprietary */
> -	RC_TYPE_LIRC		= 2,	/* Pass raw IR to lirc userspace */
> -	RC_TYPE_RC5		= 3,	/* Philips RC5 protocol */
> -	RC_TYPE_RC5X		= 4,	/* Philips RC5x protocol */
> -	RC_TYPE_RC5_SZ		= 5,	/* StreamZap variant of RC5 */
> -	RC_TYPE_JVC		= 6,	/* JVC protocol */
> -	RC_TYPE_SONY12		= 7,	/* Sony 12 bit protocol */
> -	RC_TYPE_SONY15		= 8,	/* Sony 15 bit protocol */
> -	RC_TYPE_SONY20		= 9,	/* Sony 20 bit protocol */
> -	RC_TYPE_NEC		= 10,	/* NEC protocol */
> -	RC_TYPE_SANYO		= 11,	/* Sanyo protocol */
> -	RC_TYPE_MCE_KBD		= 12,	/* RC6-ish MCE keyboard/mouse */
> -	RC_TYPE_RC6_0		= 13,	/* Philips RC6-0-16 protocol */
> -	RC_TYPE_RC6_6A_20	= 14,	/* Philips RC6-6A-20 protocol */
> -	RC_TYPE_RC6_6A_24	= 15,	/* Philips RC6-6A-24 protocol */
> -	RC_TYPE_RC6_6A_32	= 16,	/* Philips RC6-6A-32 protocol */
> -	RC_TYPE_RC6_MCE		= 17,	/* MCE (Philips RC6-6A-32 subtype) protocol */
> -	RC_TYPE_SHARP		= 18,	/* Sharp protocol */
> -	RC_TYPE_XMP		= 19,	/* XMP protocol */
> +	RC_TYPE_RC5		= 2,	/* Philips RC5 protocol */
> +	RC_TYPE_RC5X		= 3,	/* Philips RC5x protocol */
> +	RC_TYPE_RC5_SZ		= 4,	/* StreamZap variant of RC5 */
> +	RC_TYPE_JVC		= 5,	/* JVC protocol */
> +	RC_TYPE_SONY12		= 6,	/* Sony 12 bit protocol */
> +	RC_TYPE_SONY15		= 7,	/* Sony 15 bit protocol */
> +	RC_TYPE_SONY20		= 8,	/* Sony 20 bit protocol */
> +	RC_TYPE_NEC		= 9,	/* NEC protocol */
> +	RC_TYPE_SANYO		= 10,	/* Sanyo protocol */
> +	RC_TYPE_MCE_KBD		= 11,	/* RC6-ish MCE keyboard/mouse */
> +	RC_TYPE_RC6_0		= 12,	/* Philips RC6-0-16 protocol */
> +	RC_TYPE_RC6_6A_20	= 13,	/* Philips RC6-6A-20 protocol */
> +	RC_TYPE_RC6_6A_24	= 14,	/* Philips RC6-6A-24 protocol */
> +	RC_TYPE_RC6_6A_32	= 15,	/* Philips RC6-6A-32 protocol */
> +	RC_TYPE_RC6_MCE		= 16,	/* MCE (Philips RC6-6A-32 subtype) protocol */
> +	RC_TYPE_SHARP		= 17,	/* Sharp protocol */
> +	RC_TYPE_XMP		= 18,	/* XMP protocol */
>  };
>  
>  #define RC_BIT_NONE		0
>  #define RC_BIT_UNKNOWN		(1 << RC_TYPE_UNKNOWN)
>  #define RC_BIT_OTHER		(1 << RC_TYPE_OTHER)
> -#define RC_BIT_LIRC		(1 << RC_TYPE_LIRC)
>  #define RC_BIT_RC5		(1 << RC_TYPE_RC5)
>  #define RC_BIT_RC5X		(1 << RC_TYPE_RC5X)
>  #define RC_BIT_RC5_SZ		(1 << RC_TYPE_RC5_SZ)
> @@ -56,9 +54,8 @@ enum rc_type {
>  #define RC_BIT_SHARP		(1 << RC_TYPE_SHARP)
>  #define RC_BIT_XMP		(1 << RC_TYPE_XMP)
>  
> -#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_LIRC | \
> -			 RC_BIT_RC5 | RC_BIT_RC5X | RC_BIT_RC5_SZ | \
> -			 RC_BIT_JVC | \
> +#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_RC5 | \
> +			 RC_BIT_RC5X | RC_BIT_RC5_SZ | RC_BIT_JVC | \
>  			 RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
>  			 RC_BIT_NEC | RC_BIT_SANYO | RC_BIT_MCE_KBD | \
>  			 RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
> @@ -160,7 +157,6 @@ void rc_map_init(void);
>  #define RC_MAP_KWORLD_PC150U             "rc-kworld-pc150u"
>  #define RC_MAP_KWORLD_PLUS_TV_ANALOG     "rc-kworld-plus-tv-analog"
>  #define RC_MAP_LEADTEK_Y04G0051          "rc-leadtek-y04g0051"
> -#define RC_MAP_LIRC                      "rc-lirc"
>  #define RC_MAP_LME2510                   "rc-lme2510"
>  #define RC_MAP_MANLI                     "rc-manli"
>  #define RC_MAP_MEDION_X10                "rc-medion-x10"

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[David Härdeman]

On Thu, May 14, 2015 at 01:51:23PM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 19 Mar 2015 21:50:15 +0000
>Sean Young <sean@mess.org> escreveu:
>
>> Since the lirc bridge is not a decoder we can remove its protocol. The
>> keymap existed only to select the protocol.
>
>This changes the userspace interface, as now it is possible to enable/disable
>LIRC handling from a given IR via /proc interface.

I still like the general idea though. If we expose the protocol in the
set/get keymap ioctls, then we need to expose the protocol enum to
userspace (in which point it will be set in stone)...removing lirc from
that list before we do that is a worthwhile cleanup IMHO (I have a
similar patch in my queue).

I think we should be able to at least not break userspace by still
accepting (and ignoring) commands to enable/disable lirc.

That lirc won't actually be disabled/enabled is (imho) a lesser
problem...is there any genuine use case for disabling lirc on a
per-device basis?

>
>> 
>> Signed-off-by: Sean Young <sean@mess.org>
>> ---
>>  drivers/media/rc/keymaps/Makefile  |  1 -
>>  drivers/media/rc/keymaps/rc-lirc.c | 42 --------------------------------------
>>  drivers/media/rc/rc-main.c         |  1 -
>>  drivers/media/rc/st_rc.c           |  2 +-
>>  include/media/rc-map.h             | 42 +++++++++++++++++---------------------
>>  5 files changed, 20 insertions(+), 68 deletions(-)
>>  delete mode 100644 drivers/media/rc/keymaps/rc-lirc.c
>> 
>> diff --git a/drivers/media/rc/keymaps/Makefile b/drivers/media/rc/keymaps/Makefile
>> index abf6079..661cd25 100644
>> --- a/drivers/media/rc/keymaps/Makefile
>> +++ b/drivers/media/rc/keymaps/Makefile
>> @@ -51,7 +51,6 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
>>  			rc-kworld-pc150u.o \
>>  			rc-kworld-plus-tv-analog.o \
>>  			rc-leadtek-y04g0051.o \
>> -			rc-lirc.o \
>>  			rc-lme2510.o \
>>  			rc-manli.o \
>>  			rc-medion-x10.o \
>> diff --git a/drivers/media/rc/keymaps/rc-lirc.c b/drivers/media/rc/keymaps/rc-lirc.c
>> deleted file mode 100644
>> index fbf08fa..0000000
>> --- a/drivers/media/rc/keymaps/rc-lirc.c
>> +++ /dev/null
>> @@ -1,42 +0,0 @@
>> -/* rc-lirc.c - Empty dummy keytable, for use when its preferred to pass
>> - * all raw IR data to the lirc userspace decoder.
>> - *
>> - * Copyright (c) 2010 by Jarod Wilson <jarod@redhat.com>
>> - *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published by
>> - * the Free Software Foundation; either version 2 of the License, or
>> - * (at your option) any later version.
>> - */
>> -
>> -#include <media/rc-core.h>
>> -#include <linux/module.h>
>> -
>> -static struct rc_map_table lirc[] = {
>> -	{ },
>> -};
>> -
>> -static struct rc_map_list lirc_map = {
>> -	.map = {
>> -		.scan    = lirc,
>> -		.size    = ARRAY_SIZE(lirc),
>> -		.rc_type = RC_TYPE_LIRC,
>> -		.name    = RC_MAP_LIRC,
>> -	}
>> -};
>> -
>> -static int __init init_rc_map_lirc(void)
>> -{
>> -	return rc_map_register(&lirc_map);
>> -}
>> -
>> -static void __exit exit_rc_map_lirc(void)
>> -{
>> -	rc_map_unregister(&lirc_map);
>> -}
>> -
>> -module_init(init_rc_map_lirc)
>> -module_exit(exit_rc_map_lirc)
>> -
>> -MODULE_LICENSE("GPL");
>> -MODULE_AUTHOR("Jarod Wilson <jarod@redhat.com>");
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index 128909c..e717dc9 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -797,7 +797,6 @@ static struct {
>>  	{ RC_BIT_SANYO,		"sanyo"		},
>>  	{ RC_BIT_SHARP,		"sharp"		},
>>  	{ RC_BIT_MCE_KBD,	"mce_kbd"	},
>> -	{ RC_BIT_LIRC,		"lirc"		},
>>  	{ RC_BIT_XMP,		"xmp"		},
>>  };
>>  
>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
>> index 0e758ae..4834e78 100644
>> --- a/drivers/media/rc/st_rc.c
>> +++ b/drivers/media/rc/st_rc.c
>> @@ -295,7 +295,7 @@ static int st_rc_probe(struct platform_device *pdev)
>>  	rdev->open = st_rc_open;
>>  	rdev->close = st_rc_close;
>>  	rdev->driver_name = IR_ST_NAME;
>> -	rdev->map_name = RC_MAP_LIRC;
>> +	rdev->map_name = RC_MAP_EMPTY;
>>  	rdev->input_name = "ST Remote Control Receiver";
>>  
>>  	/* enable wake via this device */
>> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
>> index e7a1514..dfca14b 100644
>> --- a/include/media/rc-map.h
>> +++ b/include/media/rc-map.h
>> @@ -14,30 +14,28 @@
>>  enum rc_type {
>>  	RC_TYPE_UNKNOWN		= 0,	/* Protocol not known */
>>  	RC_TYPE_OTHER		= 1,	/* Protocol known but proprietary */
>> -	RC_TYPE_LIRC		= 2,	/* Pass raw IR to lirc userspace */
>> -	RC_TYPE_RC5		= 3,	/* Philips RC5 protocol */
>> -	RC_TYPE_RC5X		= 4,	/* Philips RC5x protocol */
>> -	RC_TYPE_RC5_SZ		= 5,	/* StreamZap variant of RC5 */
>> -	RC_TYPE_JVC		= 6,	/* JVC protocol */
>> -	RC_TYPE_SONY12		= 7,	/* Sony 12 bit protocol */
>> -	RC_TYPE_SONY15		= 8,	/* Sony 15 bit protocol */
>> -	RC_TYPE_SONY20		= 9,	/* Sony 20 bit protocol */
>> -	RC_TYPE_NEC		= 10,	/* NEC protocol */
>> -	RC_TYPE_SANYO		= 11,	/* Sanyo protocol */
>> -	RC_TYPE_MCE_KBD		= 12,	/* RC6-ish MCE keyboard/mouse */
>> -	RC_TYPE_RC6_0		= 13,	/* Philips RC6-0-16 protocol */
>> -	RC_TYPE_RC6_6A_20	= 14,	/* Philips RC6-6A-20 protocol */
>> -	RC_TYPE_RC6_6A_24	= 15,	/* Philips RC6-6A-24 protocol */
>> -	RC_TYPE_RC6_6A_32	= 16,	/* Philips RC6-6A-32 protocol */
>> -	RC_TYPE_RC6_MCE		= 17,	/* MCE (Philips RC6-6A-32 subtype) protocol */
>> -	RC_TYPE_SHARP		= 18,	/* Sharp protocol */
>> -	RC_TYPE_XMP		= 19,	/* XMP protocol */
>> +	RC_TYPE_RC5		= 2,	/* Philips RC5 protocol */
>> +	RC_TYPE_RC5X		= 3,	/* Philips RC5x protocol */
>> +	RC_TYPE_RC5_SZ		= 4,	/* StreamZap variant of RC5 */
>> +	RC_TYPE_JVC		= 5,	/* JVC protocol */
>> +	RC_TYPE_SONY12		= 6,	/* Sony 12 bit protocol */
>> +	RC_TYPE_SONY15		= 7,	/* Sony 15 bit protocol */
>> +	RC_TYPE_SONY20		= 8,	/* Sony 20 bit protocol */
>> +	RC_TYPE_NEC		= 9,	/* NEC protocol */
>> +	RC_TYPE_SANYO		= 10,	/* Sanyo protocol */
>> +	RC_TYPE_MCE_KBD		= 11,	/* RC6-ish MCE keyboard/mouse */
>> +	RC_TYPE_RC6_0		= 12,	/* Philips RC6-0-16 protocol */
>> +	RC_TYPE_RC6_6A_20	= 13,	/* Philips RC6-6A-20 protocol */
>> +	RC_TYPE_RC6_6A_24	= 14,	/* Philips RC6-6A-24 protocol */
>> +	RC_TYPE_RC6_6A_32	= 15,	/* Philips RC6-6A-32 protocol */
>> +	RC_TYPE_RC6_MCE		= 16,	/* MCE (Philips RC6-6A-32 subtype) protocol */
>> +	RC_TYPE_SHARP		= 17,	/* Sharp protocol */
>> +	RC_TYPE_XMP		= 18,	/* XMP protocol */
>>  };
>>  
>>  #define RC_BIT_NONE		0
>>  #define RC_BIT_UNKNOWN		(1 << RC_TYPE_UNKNOWN)
>>  #define RC_BIT_OTHER		(1 << RC_TYPE_OTHER)
>> -#define RC_BIT_LIRC		(1 << RC_TYPE_LIRC)
>>  #define RC_BIT_RC5		(1 << RC_TYPE_RC5)
>>  #define RC_BIT_RC5X		(1 << RC_TYPE_RC5X)
>>  #define RC_BIT_RC5_SZ		(1 << RC_TYPE_RC5_SZ)
>> @@ -56,9 +54,8 @@ enum rc_type {
>>  #define RC_BIT_SHARP		(1 << RC_TYPE_SHARP)
>>  #define RC_BIT_XMP		(1 << RC_TYPE_XMP)
>>  
>> -#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_LIRC | \
>> -			 RC_BIT_RC5 | RC_BIT_RC5X | RC_BIT_RC5_SZ | \
>> -			 RC_BIT_JVC | \
>> +#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_RC5 | \
>> +			 RC_BIT_RC5X | RC_BIT_RC5_SZ | RC_BIT_JVC | \
>>  			 RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
>>  			 RC_BIT_NEC | RC_BIT_SANYO | RC_BIT_MCE_KBD | \
>>  			 RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
>> @@ -160,7 +157,6 @@ void rc_map_init(void);
>>  #define RC_MAP_KWORLD_PC150U             "rc-kworld-pc150u"
>>  #define RC_MAP_KWORLD_PLUS_TV_ANALOG     "rc-kworld-plus-tv-analog"
>>  #define RC_MAP_LEADTEK_Y04G0051          "rc-leadtek-y04g0051"
>> -#define RC_MAP_LIRC                      "rc-lirc"
>>  #define RC_MAP_LME2510                   "rc-lme2510"
>>  #define RC_MAP_MANLI                     "rc-manli"
>>  #define RC_MAP_MEDION_X10                "rc-medion-x10"
>

-- 
David Härdeman

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[Mauro Carvalho Chehab]

Em Tue, 19 May 2015 22:34:42 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On Thu, May 14, 2015 at 01:51:23PM -0300, Mauro Carvalho Chehab wrote:
> >Em Thu, 19 Mar 2015 21:50:15 +0000
> >Sean Young <sean@mess.org> escreveu:
> >
> >> Since the lirc bridge is not a decoder we can remove its protocol. The
> >> keymap existed only to select the protocol.
> >
> >This changes the userspace interface, as now it is possible to enable/disable
> >LIRC handling from a given IR via /proc interface.

I guess I meant to say: "as now it is not possible"

> I still like the general idea though. 

Yeah, LIRC is not actually a decoder, so it makes sense to have it
handled differently.

> If we expose the protocol in the
> set/get keymap ioctls, then we need to expose the protocol enum to
> userspace (in which point it will be set in stone)...removing lirc from
> that list before we do that is a worthwhile cleanup IMHO (I have a
> similar patch in my queue).
> 
> I think we should be able to at least not break userspace by still
> accepting (and ignoring) commands to enable/disable lirc.

Well, ignoring is not a good idea, as it still breaks userspace, but
on a more evil way. If one is using this feature, we'll be receiving
bug reports and fixes for it.

> 
> That lirc won't actually be disabled/enabled is (imho) a lesser
> problem...is there any genuine use case for disabling lirc on a
> per-device basis?

People do weird things sometimes. I won't doubt that someone would
be doing that.

In any case, keep supporting disabling LIRC is likely
simple, even if we don't map it internally as a protocol anymore.

> 
> >
> >> 
> >> Signed-off-by: Sean Young <sean@mess.org>
> >> ---
> >>  drivers/media/rc/keymaps/Makefile  |  1 -
> >>  drivers/media/rc/keymaps/rc-lirc.c | 42 --------------------------------------
> >>  drivers/media/rc/rc-main.c         |  1 -
> >>  drivers/media/rc/st_rc.c           |  2 +-
> >>  include/media/rc-map.h             | 42 +++++++++++++++++---------------------
> >>  5 files changed, 20 insertions(+), 68 deletions(-)
> >>  delete mode 100644 drivers/media/rc/keymaps/rc-lirc.c
> >> 
> >> diff --git a/drivers/media/rc/keymaps/Makefile b/drivers/media/rc/keymaps/Makefile
> >> index abf6079..661cd25 100644
> >> --- a/drivers/media/rc/keymaps/Makefile
> >> +++ b/drivers/media/rc/keymaps/Makefile
> >> @@ -51,7 +51,6 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
> >>  			rc-kworld-pc150u.o \
> >>  			rc-kworld-plus-tv-analog.o \
> >>  			rc-leadtek-y04g0051.o \
> >> -			rc-lirc.o \
> >>  			rc-lme2510.o \
> >>  			rc-manli.o \
> >>  			rc-medion-x10.o \
> >> diff --git a/drivers/media/rc/keymaps/rc-lirc.c b/drivers/media/rc/keymaps/rc-lirc.c
> >> deleted file mode 100644
> >> index fbf08fa..0000000
> >> --- a/drivers/media/rc/keymaps/rc-lirc.c
> >> +++ /dev/null
> >> @@ -1,42 +0,0 @@
> >> -/* rc-lirc.c - Empty dummy keytable, for use when its preferred to pass
> >> - * all raw IR data to the lirc userspace decoder.
> >> - *
> >> - * Copyright (c) 2010 by Jarod Wilson <jarod@redhat.com>
> >> - *
> >> - * This program is free software; you can redistribute it and/or modify
> >> - * it under the terms of the GNU General Public License as published by
> >> - * the Free Software Foundation; either version 2 of the License, or
> >> - * (at your option) any later version.
> >> - */
> >> -
> >> -#include <media/rc-core.h>
> >> -#include <linux/module.h>
> >> -
> >> -static struct rc_map_table lirc[] = {
> >> -	{ },
> >> -};
> >> -
> >> -static struct rc_map_list lirc_map = {
> >> -	.map = {
> >> -		.scan    = lirc,
> >> -		.size    = ARRAY_SIZE(lirc),
> >> -		.rc_type = RC_TYPE_LIRC,
> >> -		.name    = RC_MAP_LIRC,
> >> -	}
> >> -};
> >> -
> >> -static int __init init_rc_map_lirc(void)
> >> -{
> >> -	return rc_map_register(&lirc_map);
> >> -}
> >> -
> >> -static void __exit exit_rc_map_lirc(void)
> >> -{
> >> -	rc_map_unregister(&lirc_map);
> >> -}
> >> -
> >> -module_init(init_rc_map_lirc)
> >> -module_exit(exit_rc_map_lirc)
> >> -
> >> -MODULE_LICENSE("GPL");
> >> -MODULE_AUTHOR("Jarod Wilson <jarod@redhat.com>");
> >> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> >> index 128909c..e717dc9 100644
> >> --- a/drivers/media/rc/rc-main.c
> >> +++ b/drivers/media/rc/rc-main.c
> >> @@ -797,7 +797,6 @@ static struct {
> >>  	{ RC_BIT_SANYO,		"sanyo"		},
> >>  	{ RC_BIT_SHARP,		"sharp"		},
> >>  	{ RC_BIT_MCE_KBD,	"mce_kbd"	},
> >> -	{ RC_BIT_LIRC,		"lirc"		},
> >>  	{ RC_BIT_XMP,		"xmp"		},
> >>  };
> >>  
> >> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> >> index 0e758ae..4834e78 100644
> >> --- a/drivers/media/rc/st_rc.c
> >> +++ b/drivers/media/rc/st_rc.c
> >> @@ -295,7 +295,7 @@ static int st_rc_probe(struct platform_device *pdev)
> >>  	rdev->open = st_rc_open;
> >>  	rdev->close = st_rc_close;
> >>  	rdev->driver_name = IR_ST_NAME;
> >> -	rdev->map_name = RC_MAP_LIRC;
> >> +	rdev->map_name = RC_MAP_EMPTY;
> >>  	rdev->input_name = "ST Remote Control Receiver";
> >>  
> >>  	/* enable wake via this device */
> >> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> >> index e7a1514..dfca14b 100644
> >> --- a/include/media/rc-map.h
> >> +++ b/include/media/rc-map.h
> >> @@ -14,30 +14,28 @@
> >>  enum rc_type {
> >>  	RC_TYPE_UNKNOWN		= 0,	/* Protocol not known */
> >>  	RC_TYPE_OTHER		= 1,	/* Protocol known but proprietary */
> >> -	RC_TYPE_LIRC		= 2,	/* Pass raw IR to lirc userspace */
> >> -	RC_TYPE_RC5		= 3,	/* Philips RC5 protocol */
> >> -	RC_TYPE_RC5X		= 4,	/* Philips RC5x protocol */
> >> -	RC_TYPE_RC5_SZ		= 5,	/* StreamZap variant of RC5 */
> >> -	RC_TYPE_JVC		= 6,	/* JVC protocol */
> >> -	RC_TYPE_SONY12		= 7,	/* Sony 12 bit protocol */
> >> -	RC_TYPE_SONY15		= 8,	/* Sony 15 bit protocol */
> >> -	RC_TYPE_SONY20		= 9,	/* Sony 20 bit protocol */
> >> -	RC_TYPE_NEC		= 10,	/* NEC protocol */
> >> -	RC_TYPE_SANYO		= 11,	/* Sanyo protocol */
> >> -	RC_TYPE_MCE_KBD		= 12,	/* RC6-ish MCE keyboard/mouse */
> >> -	RC_TYPE_RC6_0		= 13,	/* Philips RC6-0-16 protocol */
> >> -	RC_TYPE_RC6_6A_20	= 14,	/* Philips RC6-6A-20 protocol */
> >> -	RC_TYPE_RC6_6A_24	= 15,	/* Philips RC6-6A-24 protocol */
> >> -	RC_TYPE_RC6_6A_32	= 16,	/* Philips RC6-6A-32 protocol */
> >> -	RC_TYPE_RC6_MCE		= 17,	/* MCE (Philips RC6-6A-32 subtype) protocol */
> >> -	RC_TYPE_SHARP		= 18,	/* Sharp protocol */
> >> -	RC_TYPE_XMP		= 19,	/* XMP protocol */
> >> +	RC_TYPE_RC5		= 2,	/* Philips RC5 protocol */
> >> +	RC_TYPE_RC5X		= 3,	/* Philips RC5x protocol */
> >> +	RC_TYPE_RC5_SZ		= 4,	/* StreamZap variant of RC5 */
> >> +	RC_TYPE_JVC		= 5,	/* JVC protocol */
> >> +	RC_TYPE_SONY12		= 6,	/* Sony 12 bit protocol */
> >> +	RC_TYPE_SONY15		= 7,	/* Sony 15 bit protocol */
> >> +	RC_TYPE_SONY20		= 8,	/* Sony 20 bit protocol */
> >> +	RC_TYPE_NEC		= 9,	/* NEC protocol */
> >> +	RC_TYPE_SANYO		= 10,	/* Sanyo protocol */
> >> +	RC_TYPE_MCE_KBD		= 11,	/* RC6-ish MCE keyboard/mouse */
> >> +	RC_TYPE_RC6_0		= 12,	/* Philips RC6-0-16 protocol */
> >> +	RC_TYPE_RC6_6A_20	= 13,	/* Philips RC6-6A-20 protocol */
> >> +	RC_TYPE_RC6_6A_24	= 14,	/* Philips RC6-6A-24 protocol */
> >> +	RC_TYPE_RC6_6A_32	= 15,	/* Philips RC6-6A-32 protocol */
> >> +	RC_TYPE_RC6_MCE		= 16,	/* MCE (Philips RC6-6A-32 subtype) protocol */
> >> +	RC_TYPE_SHARP		= 17,	/* Sharp protocol */
> >> +	RC_TYPE_XMP		= 18,	/* XMP protocol */
> >>  };
> >>  
> >>  #define RC_BIT_NONE		0
> >>  #define RC_BIT_UNKNOWN		(1 << RC_TYPE_UNKNOWN)
> >>  #define RC_BIT_OTHER		(1 << RC_TYPE_OTHER)
> >> -#define RC_BIT_LIRC		(1 << RC_TYPE_LIRC)
> >>  #define RC_BIT_RC5		(1 << RC_TYPE_RC5)
> >>  #define RC_BIT_RC5X		(1 << RC_TYPE_RC5X)
> >>  #define RC_BIT_RC5_SZ		(1 << RC_TYPE_RC5_SZ)
> >> @@ -56,9 +54,8 @@ enum rc_type {
> >>  #define RC_BIT_SHARP		(1 << RC_TYPE_SHARP)
> >>  #define RC_BIT_XMP		(1 << RC_TYPE_XMP)
> >>  
> >> -#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_LIRC | \
> >> -			 RC_BIT_RC5 | RC_BIT_RC5X | RC_BIT_RC5_SZ | \
> >> -			 RC_BIT_JVC | \
> >> +#define RC_BIT_ALL	(RC_BIT_UNKNOWN | RC_BIT_OTHER | RC_BIT_RC5 | \
> >> +			 RC_BIT_RC5X | RC_BIT_RC5_SZ | RC_BIT_JVC | \
> >>  			 RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
> >>  			 RC_BIT_NEC | RC_BIT_SANYO | RC_BIT_MCE_KBD | \
> >>  			 RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
> >> @@ -160,7 +157,6 @@ void rc_map_init(void);
> >>  #define RC_MAP_KWORLD_PC150U             "rc-kworld-pc150u"
> >>  #define RC_MAP_KWORLD_PLUS_TV_ANALOG     "rc-kworld-plus-tv-analog"
> >>  #define RC_MAP_LEADTEK_Y04G0051          "rc-leadtek-y04g0051"
> >> -#define RC_MAP_LIRC                      "rc-lirc"
> >>  #define RC_MAP_LME2510                   "rc-lme2510"
> >>  #define RC_MAP_MANLI                     "rc-manli"
> >>  #define RC_MAP_MEDION_X10                "rc-medion-x10"
> >
> 

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[David Härdeman]

On 2015-05-20 10:19, Mauro Carvalho Chehab wrote:
> Em Tue, 19 May 2015 22:34:42 +0200
> David Härdeman <david@hardeman.nu> escreveu:
> 
>> On Thu, May 14, 2015 at 01:51:23PM -0300, Mauro Carvalho Chehab wrote:
>> >Em Thu, 19 Mar 2015 21:50:15 +0000
>> >Sean Young <sean@mess.org> escreveu:
>> >
>> >> Since the lirc bridge is not a decoder we can remove its protocol. The
>> >> keymap existed only to select the protocol.
>> >
>> >This changes the userspace interface, as now it is possible to enable/disable
>> >LIRC handling from a given IR via /proc interface.
> 
> I guess I meant to say: "as now it is not possible"
> 
>> I still like the general idea though.
> 
> Yeah, LIRC is not actually a decoder, so it makes sense to have it
> handled differently.
> 
>> If we expose the protocol in the
>> set/get keymap ioctls, then we need to expose the protocol enum to
>> userspace (in which point it will be set in stone)...removing lirc 
>> from
>> that list before we do that is a worthwhile cleanup IMHO (I have a
>> similar patch in my queue).
>> 
>> I think we should be able to at least not break userspace by still
>> accepting (and ignoring) commands to enable/disable lirc.
> 
> Well, ignoring is not a good idea, as it still breaks userspace, but
> on a more evil way. If one is using this feature, we'll be receiving
> bug reports and fixes for it.

I disagree it's more "evil" (or at least I fail to see how it would be). 
Accepting but ignoring "lirc" means that the same commands as before 
will still be accepted (so pre-existing userspace scripts won't have to 
change which they would if we made "lirc" an invalid protocol to echo to 
the sysfs file).

And saying that the change will "break" userspace is still something of 
a misnomer. You'd basically expect userspace to open /sys/blabla, write 
"-lirc" (which would disable the lirc output but the device node is 
still in /dev), then later open /dev/lircX and be surprised that it's 
still receiving lirc events on the lirc device it just opened? I think 
that's a rather artificial scenario...

>> That lirc won't actually be disabled/enabled is (imho) a lesser
>> problem...is there any genuine use case for disabling lirc on a
>> per-device basis?
> 
> People do weird things sometimes. I won't doubt that someone would
> be doing that.
> 
> In any case, keep supporting disabling LIRC is likely
> simple, even if we don't map it internally as a protocol anymore.

I could write a different patch that removes the protocol enum but still 
allows lirc to be disabled/enabled. I doubt it'll be that simple though 
(ugly hack rather), and I still don't see the benefits of doing so (or 
downsides or "breakage" of not doing it).

Another option would be to commit the change a see if anyone screams (I 
very much doubt it).

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[Mauro Carvalho Chehab]

Em Wed, 20 May 2015 10:49:34 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On 2015-05-20 10:19, Mauro Carvalho Chehab wrote:
> > Em Tue, 19 May 2015 22:34:42 +0200
> > David Härdeman <david@hardeman.nu> escreveu:
> > 
> >> On Thu, May 14, 2015 at 01:51:23PM -0300, Mauro Carvalho Chehab wrote:
> >> >Em Thu, 19 Mar 2015 21:50:15 +0000
> >> >Sean Young <sean@mess.org> escreveu:
> >> >
> >> >> Since the lirc bridge is not a decoder we can remove its protocol. The
> >> >> keymap existed only to select the protocol.
> >> >
> >> >This changes the userspace interface, as now it is possible to enable/disable
> >> >LIRC handling from a given IR via /proc interface.
> > 
> > I guess I meant to say: "as now it is not possible"
> > 
> >> I still like the general idea though.
> > 
> > Yeah, LIRC is not actually a decoder, so it makes sense to have it
> > handled differently.
> > 
> >> If we expose the protocol in the
> >> set/get keymap ioctls, then we need to expose the protocol enum to
> >> userspace (in which point it will be set in stone)...removing lirc 
> >> from
> >> that list before we do that is a worthwhile cleanup IMHO (I have a
> >> similar patch in my queue).
> >> 
> >> I think we should be able to at least not break userspace by still
> >> accepting (and ignoring) commands to enable/disable lirc.
> > 
> > Well, ignoring is not a good idea, as it still breaks userspace, but
> > on a more evil way. If one is using this feature, we'll be receiving
> > bug reports and fixes for it.
> 
> I disagree it's more "evil" (or at least I fail to see how it would be). 

Because the Kernel would be lying to userspace. If one tells the Kernel to
disable something, it should do it, or otherwise return an error explaining
why disabling was not possible.

> Accepting but ignoring "lirc" means that the same commands as before 
> will still be accepted (so pre-existing userspace scripts won't have to 
> change which they would if we made "lirc" an invalid protocol to echo to 
> the sysfs file).

Yes, but, if someone is trying to disable lirc, it is doing for some
reason. The script won't fail, but his application will.

> And saying that the change will "break" userspace is still something of 
> a misnomer. You'd basically expect userspace to open /sys/blabla, write 
> "-lirc" (which would disable the lirc output but the device node is 
> still in /dev), then later open /dev/lircX and be surprised that it's 
> still receiving lirc events on the lirc device it just opened? I think 
> that's a rather artificial scenario...

Well, lircd is a daemon. I can easily an usecase where some application
would like to prevent it to be running because such application would
read the RC codes directly from input/dev.

I'm not arguing that this is the best way of doing that (nor I have
myself any such usecases), but if some app relies on such behavior, then
this is an userspace breakage.

> >> That lirc won't actually be disabled/enabled is (imho) a lesser
> >> problem...is there any genuine use case for disabling lirc on a
> >> per-device basis?
> > 
> > People do weird things sometimes. I won't doubt that someone would
> > be doing that.
> > 
> > In any case, keep supporting disabling LIRC is likely
> > simple, even if we don't map it internally as a protocol anymore.
> 
> I could write a different patch that removes the protocol enum but still 
> allows lirc to be disabled/enabled. I doubt it'll be that simple though 
> (ugly hack rather), and I still don't see the benefits of doing so (or 
> downsides or "breakage" of not doing it).
> 
> Another option would be to commit the change a see if anyone screams (I 
> very much doubt it).

It may take months for people to discover, as it will only reach userspace
after distros merge the patch on their Kernel. Not nice. We should avoid
doing things like that.

Regards,
Mauro

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[David Härdeman]

On 2015-05-20 11:01, Mauro Carvalho Chehab wrote:
> Em Wed, 20 May 2015 10:49:34 +0200
> David Härdeman <david@hardeman.nu> escreveu:
> 
>> On 2015-05-20 10:19, Mauro Carvalho Chehab wrote:
>> > Em Tue, 19 May 2015 22:34:42 +0200
>> > David Härdeman <david@hardeman.nu> escreveu:
>> >> I think we should be able to at least not break userspace by still
>> >> accepting (and ignoring) commands to enable/disable lirc.
>> >
>> > Well, ignoring is not a good idea, as it still breaks userspace, but
>> > on a more evil way. If one is using this feature, we'll be receiving
>> > bug reports and fixes for it.
>> 
>> I disagree it's more "evil" (or at least I fail to see how it would 
>> be).
> 
> Because the Kernel would be lying to userspace. If one tells the Kernel 
> to
> disable something, it should do it, or otherwise return an error 
> explaining
> why disabling was not possible.

Would really you be happier with a patch so that writing "-lirc" to the 
sysfs file returns an error?

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[David Härdeman]

On Wed, May 20, 2015 at 11:06:06AM +0200, David Härdeman wrote:
>On 2015-05-20 11:01, Mauro Carvalho Chehab wrote:
>>Em Wed, 20 May 2015 10:49:34 +0200
>>David Härdeman <david@hardeman.nu> escreveu:
>>
>>>On 2015-05-20 10:19, Mauro Carvalho Chehab wrote:
>>>> Em Tue, 19 May 2015 22:34:42 +0200
>>>> David Härdeman <david@hardeman.nu> escreveu:
>>>>> I think we should be able to at least not break userspace by still
>>>>> accepting (and ignoring) commands to enable/disable lirc.
>>>>
>>>> Well, ignoring is not a good idea, as it still breaks userspace, but
>>>> on a more evil way. If one is using this feature, we'll be receiving
>>>> bug reports and fixes for it.
>>>
>>>I disagree it's more "evil" (or at least I fail to see how it would be).
>>
>>Because the Kernel would be lying to userspace. If one tells the Kernel to
>>disable something, it should do it, or otherwise return an error explaining
>>why disabling was not possible.
>
>Would really you be happier with a patch so that writing "-lirc" to the sysfs
>file returns an error?

Actually that would be very weird in case userspace writes e.g. "rc5" to
the sysfs file (since that implies disabling lirc which would then
return an error as well).

So, that won't work. I still think just ignoring "+lirc" and "-lirc" is
the best solution...(and the usecase you suggested of disabling lirc so
that lircd won't get any events while an app reads only decoded
events...seems very far-fetched)...do you have any other suggestion?

-- 
David Härdeman

Re: [RFC PATCH 4/6] [media] rc: lirc is not a protocol or a keymap

[David Härdeman]

On Wed, May 20, 2015 at 09:16:32PM +0200, David Härdeman wrote:
>On Wed, May 20, 2015 at 11:06:06AM +0200, David Härdeman wrote:
>>On 2015-05-20 11:01, Mauro Carvalho Chehab wrote:
>>>Em Wed, 20 May 2015 10:49:34 +0200
>>>David Härdeman <david@hardeman.nu> escreveu:
>>>
>>>>On 2015-05-20 10:19, Mauro Carvalho Chehab wrote:
>>>>> Em Tue, 19 May 2015 22:34:42 +0200
>>>>> David Härdeman <david@hardeman.nu> escreveu:
>>>>>> I think we should be able to at least not break userspace by still
>>>>>> accepting (and ignoring) commands to enable/disable lirc.
>>>>>
>>>>> Well, ignoring is not a good idea, as it still breaks userspace, but
>>>>> on a more evil way. If one is using this feature, we'll be receiving
>>>>> bug reports and fixes for it.
>>>>
>>>>I disagree it's more "evil" (or at least I fail to see how it would be).
>>>
>>>Because the Kernel would be lying to userspace. If one tells the Kernel to
>>>disable something, it should do it, or otherwise return an error explaining
>>>why disabling was not possible.
>>
>>Would really you be happier with a patch so that writing "-lirc" to the sysfs
>>file returns an error?
>
>Actually that would be very weird in case userspace writes e.g. "rc5" to
>the sysfs file (since that implies disabling lirc which would then
>return an error as well).
>
>So, that won't work. I still think just ignoring "+lirc" and "-lirc" is
>the best solution...(and the usecase you suggested of disabling lirc so
>that lircd won't get any events while an app reads only decoded
>events...seems very far-fetched)...do you have any other suggestion?

I've done some more checking.

Not changing protocols even though user-space asked for it is actually
already part of the rc-core API.

struct rc_dev contains:
	int (*change_protocol)(struct rc_dev *dev, u64 *rc_type);

The reason rc_type is a pointer is that it is used to pass the requested
protocols to the change_protocol function and the function can then
update it to reflect what the actual result was.

Examples:
drivers/media/usb/em28xx/em28xx-input.c
drivers/media/rc/img-ir/img-ir-hw.c

Both of these drivers will pick *one* protocol to enable...so if you'd
write "+rc5 +nec" to their sysfs protocols file, it'd just enable one
protocol (and the file would read e.g. "[rc5] nec" afterwards).

With that in mind, userspace could never expect the list of enabled
protocols to match what it just wrote, even if no error was returned.

-- 
David Härdeman

[RFC PATCH 5/6] [media] lirc: pass IR scancodes to userspace via lirc bridge

[Sean Young]

The lirc interface only passes raw IR. Teach the lirc bridge how to pass
scancodes (along with their IR information) to userspace. This introduces
a new LIRC_MODE_SCANCODE mode where decoded IR is represented as two
u32. The first one signifies LIRC_MODE_SCANCODE, the IR protocol, repeat
and toggle bits, and the next u32 the scancode. This can be enabled with
LIRC_MODE_MODE2 at the same time so that raw IR and scancodes will all
be read.

By default LIRC_MODE_MODE2 is only enabled for raw IR devices so that
user space does not get confused by the new scancode messages. It can be
enabled if LIRC_MODE_SCANCODE is set using LIRC_SET_REC_MODE ioctl.

FIXME: The keycode is not passed via the bridge, but only via the input
interface. Maybe this should be changed.

With this change every rc device will have a lirc interface, including
those which only produce scancodes in which case LIRC_MODE_SCANCODE will
be enabled (else the lirc device will never produce anything).

Signed-off-by: Sean Young <sean@mess.org>
---
 .../DocBook/media/v4l/lirc_device_interface.xml    | 31 +++++++++-------
 drivers/media/rc/Kconfig                           |  4 +--
 drivers/media/rc/ir-lirc-codec.c                   | 42 +++++++++++++++++++++-
 drivers/media/rc/rc-core-priv.h                    |  4 +++
 drivers/media/rc/rc-main.c                         | 15 +++++---
 include/media/lirc.h                               |  8 +++++
 include/media/rc-core.h                            |  1 +
 7 files changed, 86 insertions(+), 19 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/lirc_device_interface.xml b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
index 25926bd..f92b5a5 100644
--- a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
+++ b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
@@ -6,10 +6,10 @@
 <title>Introduction</title>
 
 <para>The LIRC device interface is a bi-directional interface for
-transporting raw IR data between userspace and kernelspace. Fundamentally,
+transporting IR data between userspace and kernelspace. Fundamentally,
 it is just a chardev (/dev/lircX, for X = 0, 1, 2, ...), with a number
 of standard struct file_operations defined on it. With respect to
-transporting raw IR data to and fro, the essential fops are read, write
+transporting IR data to and fro, the essential fops are read, write
 and ioctl.</para>
 
 <para>Example dmesg output upon a driver registering w/LIRC:</para>
@@ -29,14 +29,19 @@ and ioctl.</para>
 <section id="lirc_read">
 <title>LIRC read fop</title>
 
-<para>The lircd userspace daemon reads raw IR data from the LIRC chardev. The
-exact format of the data depends on what modes a driver supports, and what
-mode has been selected. lircd obtains supported modes and sets the active mode
-via the ioctl interface, detailed at <xref linkend="lirc_ioctl"/>. The generally
-preferred mode is LIRC_MODE_MODE2, in which packets containing an int value
-describing an IR signal are read from the chardev.</para>
+<para>The data read from the chardev is IR. The format depends on the rec mode;
+this is either in LIRC_MODE_MODE2, LIRC_MODE_SCANCODE or both. In MODE2, data
+is read as 32 bit unsigned values. The highest 8 bits signifies the type:
+LIRC_MODE2_PULSE, LIRC_MODE2_SPACE, LIRC_MODE2_FREQUENCY, LIRC_MODE2_TIMEOUT.
+The lower 24 bits signify the value either in Hertz or nanoseconds.
+</para>
+<para>If LIRC_MODE_SCANCODE is enabled then the type in the highest 8 bits
+is LIRC_MODE2_SCANCODE. The 24 bit signifies a repeat, 23 bit toggle set and
+the lowest 8 bits is the rc protocol (see rc_type in rc-core.h). The next full
+unsigned int is the scancode; there is no type in the highest 8 bits.
+</para>
+<para>The mode can be set and get using <xref linkend="lirc_ioctl"/>. </para>
 
-<para>See also <ulink url="http://www.lirc.org/html/technical.html">http://www.lirc.org/html/technical.html</ulink> for more info.</para>
 </section>
 
 <section id="lirc_write">
@@ -82,10 +87,12 @@ on working with the default settings initially.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
-    <term>LIRC_GET_REC_MODE</term>
+    <term>LIRC_{G,S}ET_REC_MODE</term>
     <listitem>
-      <para>Get supported receive modes. Only LIRC_MODE_MODE2 and LIRC_MODE_LIRCCODE
-      are supported by lircd.</para>
+      <para>Get or set the receive mode. Devices that support raw IR will
+      support LIRC_MODE_MODE2; all devices support LIRC_MODE_SCANCODE. Note
+      that both modes can be enabled by ORing. That way, both raw IR and
+      decoded scancodes can be read simultaneously.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index efdd6f7..247c22c 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -13,7 +13,7 @@ config LIRC
 	---help---
 	   Enable this option to build the Linux Infrared Remote
 	   Control (LIRC) core device interface driver. The LIRC
-	   interface passes raw IR to and from userspace, where the
+	   interface passes IR to and from userspace, where the
 	   LIRC daemon handles protocol decoding for IR reception and
 	   encoding for IR transmitting (aka "blasting").
 
@@ -24,7 +24,7 @@ config IR_LIRC_CODEC
 	default y
 
 	---help---
-	   Enable this option to pass raw IR to and from userspace via
+	   Enable this option to pass IR to and from userspace via
 	   the LIRC interface.
 
 menuconfig RC_DECODERS
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 475f6af..594535e 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -36,6 +36,8 @@ int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 
 	if (!lirc || !lirc->rbuf)
 		return -EINVAL;
+	if (!(dev->rec_mode & LIRC_MODE_MODE2))
+		return 0;
 
 	/* Packet start */
 	if (ev.reset) {
@@ -101,6 +103,26 @@ int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 	return 0;
 }
 
+void ir_lirc_scancode(struct rc_dev *dev, enum rc_type proto, u32 scancode,
+						bool toggle, bool repeat)
+{
+	struct lirc_driver *lirc = dev->lirc;
+	int sample;
+
+	if (!lirc || !lirc->rbuf || !(dev->rec_mode & LIRC_MODE_SCANCODE))
+		return;
+
+	sample = LIRC_MODE2_SCANCODE | proto;
+	if (toggle)
+		sample |= LIRC_SCANCODE_TOGGLE;
+	if (repeat)
+		sample |= LIRC_SCANCODE_REPEAT;
+
+	lirc_buffer_write(lirc->rbuf, (unsigned char *) &sample);
+	lirc_buffer_write(lirc->rbuf, (unsigned char *) &scancode);
+	wake_up(&lirc->rbuf->wait_poll);
+}
+
 static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 				   size_t n, loff_t *ppos)
 {
@@ -206,6 +228,20 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 
 		return 0;
 
+	case LIRC_GET_REC_MODE:
+		val = dev->rec_mode;
+		break;
+
+	case LIRC_SET_REC_MODE:
+		if (val == 0 ||
+			val & ~(LIRC_MODE_MODE2 | LIRC_MODE_SCANCODE) ||
+			(!(dev->lirc->features & LIRC_CAN_REC_MODE2) &&
+					(val & LIRC_MODE_MODE2)))
+			return -EINVAL;
+
+		dev->rec_mode = val;
+		return 0;
+
 	/* TX settings */
 	case LIRC_SET_TRANSMITTER_MASK:
 		if (!dev->s_tx_mask)
@@ -346,7 +382,9 @@ int ir_lirc_register(struct rc_dev *dev)
 	if (rc)
 		goto rbuf_init_failed;
 
-	features = LIRC_CAN_REC_MODE2;
+	features = LIRC_CAN_REC_SCANCODE;
+	if (dev->driver_type == RC_DRIVER_IR_RAW)
+		features |= LIRC_CAN_REC_MODE2;
 	if (dev->tx_ir) {
 		features |= LIRC_CAN_SEND_PULSE;
 		if (dev->s_tx_mask)
@@ -374,6 +412,8 @@ int ir_lirc_register(struct rc_dev *dev)
 		 dev->driver_name);
 	drv->minor = -1;
 	drv->features = features;
+	dev->rec_mode = features & LIRC_CAN_REC_MODE2 ?
+					LIRC_MODE_MODE2 : LIRC_MODE2_SCANCODE;
 	drv->data = dev;
 	drv->rbuf = rbuf;
 	drv->set_use_inc = &ir_lirc_open;
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 732479d..f613306 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -223,11 +223,15 @@ static inline void load_xmp_decode(void) { }
 void ir_lirc_unregister(struct rc_dev *dev);
 int ir_lirc_register(struct rc_dev *dev);
 int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev);
+void ir_lirc_scancode(struct rc_dev *dev, enum rc_type proto, u32 scancode,
+						bool toggle, bool repeat);
 #else
 static inline void ir_lirc_unregister(struct rc_dev *dev) {}
 static inline int ir_lirc_register(struct rc_dev *dev) { return 0; }
 static inline int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
 								{ return 0; }
+static inline void ir_lirc_scancode(struct rc_dev *dev, enum rc_type proto,
+				u32 scancode, bool toggle, bool repeat) {}
 #endif
 
 #ifdef CONFIG_LIRC
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index e717dc9..483038b 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -606,6 +606,10 @@ void rc_repeat(struct rc_dev *dev)
 
 	spin_lock_irqsave(&dev->keylock, flags);
 
+	if (dev->lirc)
+		ir_lirc_scancode(dev, dev->last_protocol, dev->last_scancode,
+								false, true);
+
 	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
 	input_sync(dev->input_dev);
 
@@ -642,6 +646,9 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
 	if (new_event && dev->keypressed)
 		ir_do_keyup(dev, false);
 
+	if (dev->lirc)
+		ir_lirc_scancode(dev, protocol, scancode, toggle, false);
+
 	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
 
 	if (new_event && keycode != KEY_RESERVED) {
@@ -1416,12 +1423,12 @@ int rc_register_device(struct rc_dev *dev)
 		mutex_lock(&dev->lock);
 		if (rc < 0)
 			goto out_input;
-
-		rc = ir_lirc_register(dev);
-		if (rc < 0)
-			goto out_raw;
 	}
 
+	rc = ir_lirc_register(dev);
+	if (rc < 0)
+		goto out_raw;
+
 	if (dev->change_protocol) {
 		u64 rc_type = (1ll << rc_map->rc_type);
 		rc = dev->change_protocol(dev, &rc_type);
diff --git a/include/media/lirc.h b/include/media/lirc.h
index 7b845f8..a635fc9 100644
--- a/include/media/lirc.h
+++ b/include/media/lirc.h
@@ -16,6 +16,7 @@
 #define LIRC_MODE2_PULSE     0x01000000
 #define LIRC_MODE2_FREQUENCY 0x02000000
 #define LIRC_MODE2_TIMEOUT   0x03000000
+#define LIRC_MODE2_SCANCODE  0x04000000
 
 #define LIRC_VALUE_MASK      0x00FFFFFF
 #define LIRC_MODE2_MASK      0xFF000000
@@ -32,6 +33,11 @@
 #define LIRC_IS_PULSE(val) (LIRC_MODE2(val) == LIRC_MODE2_PULSE)
 #define LIRC_IS_FREQUENCY(val) (LIRC_MODE2(val) == LIRC_MODE2_FREQUENCY)
 #define LIRC_IS_TIMEOUT(val) (LIRC_MODE2(val) == LIRC_MODE2_TIMEOUT)
+#define LIRC_IS_SCANCODE(val) (LIRC_MODE2(val) == LIRC_MODE2_SCANCODE)
+
+#define LIRC_SCANCODE_TOGGLE		0x00800000
+#define LIRC_SCANCODE_REPEAT		0x00400000
+#define LIRC_SCANCODE_PROTOCOL_MASK	0x000000ff
 
 /* used heavily by lirc userspace */
 #define lirc_t int
@@ -46,6 +52,7 @@
 #define LIRC_MODE_RAW                  0x00000001
 #define LIRC_MODE_PULSE                0x00000002
 #define LIRC_MODE_MODE2                0x00000004
+#define LIRC_MODE_SCANCODE             0x00000008
 #define LIRC_MODE_LIRCCODE             0x00000010
 
 
@@ -64,6 +71,7 @@
 #define LIRC_CAN_REC_PULSE             LIRC_MODE2REC(LIRC_MODE_PULSE)
 #define LIRC_CAN_REC_MODE2             LIRC_MODE2REC(LIRC_MODE_MODE2)
 #define LIRC_CAN_REC_LIRCCODE          LIRC_MODE2REC(LIRC_MODE_LIRCCODE)
+#define LIRC_CAN_REC_SCANCODE          LIRC_MODE2REC(LIRC_MODE_SCANCODE)
 
 #define LIRC_CAN_REC_MASK              LIRC_MODE2REC(LIRC_CAN_SEND_MASK)
 
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index e3f217c..a8cef8c 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -163,6 +163,7 @@ struct rc_dev {
 	u64				gap_duration;
 	bool				gap;
 	bool				send_timeout_reports;
+	u32				rec_mode;
 	int				(*change_protocol)(struct rc_dev *dev, u64 *rc_type);
 	int				(*change_wakeup_protocol)(struct rc_dev *dev, u64 *rc_type);
 	int				(*open)(struct rc_dev *dev);
-- 
2.1.0

Re: [RFC PATCH 5/6] [media] lirc: pass IR scancodes to userspace via lirc bridge

[Mauro Carvalho Chehab]

Em Thu, 19 Mar 2015 21:50:16 +0000
Sean Young <sean@mess.org> escreveu:

> The lirc interface only passes raw IR. Teach the lirc bridge how to pass
> scancodes (along with their IR information) to userspace. This introduces
> a new LIRC_MODE_SCANCODE mode where decoded IR is represented as two
> u32. The first one signifies LIRC_MODE_SCANCODE, the IR protocol, repeat
> and toggle bits, and the next u32 the scancode. This can be enabled with
> LIRC_MODE_MODE2 at the same time so that raw IR and scancodes will all
> be read.
> 
> By default LIRC_MODE_MODE2 is only enabled for raw IR devices so that
> user space does not get confused by the new scancode messages. It can be
> enabled if LIRC_MODE_SCANCODE is set using LIRC_SET_REC_MODE ioctl.
> 
> FIXME: The keycode is not passed via the bridge, but only via the input
> interface. Maybe this should be changed.

What do you mean by "the bridge" in this context? The lirc devnode?

IMHO, if we enable LIRC to use LIRC_MODE_SCANCODE, the output via input
evdev should be suppressed, and only the LIRC interface should be doing
I/O. Yet, eventually it makes sense to provide a way to explicitly allow
or disable I/O via input/evdev interface when LIRC is in scanmode.

> 
> With this change every rc device will have a lirc interface, including
> those which only produce scancodes in which case LIRC_MODE_SCANCODE will
> be enabled (else the lirc device will never produce anything).
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  .../DocBook/media/v4l/lirc_device_interface.xml    | 31 +++++++++-------
>  drivers/media/rc/Kconfig                           |  4 +--
>  drivers/media/rc/ir-lirc-codec.c                   | 42 +++++++++++++++++++++-
>  drivers/media/rc/rc-core-priv.h                    |  4 +++
>  drivers/media/rc/rc-main.c                         | 15 +++++---
>  include/media/lirc.h                               |  8 +++++
>  include/media/rc-core.h                            |  1 +
>  7 files changed, 86 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/lirc_device_interface.xml b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> index 25926bd..f92b5a5 100644
> --- a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> +++ b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> @@ -6,10 +6,10 @@
>  <title>Introduction</title>
>  
>  <para>The LIRC device interface is a bi-directional interface for
> -transporting raw IR data between userspace and kernelspace. Fundamentally,
> +transporting IR data between userspace and kernelspace. Fundamentally,
>  it is just a chardev (/dev/lircX, for X = 0, 1, 2, ...), with a number
>  of standard struct file_operations defined on it. With respect to
> -transporting raw IR data to and fro, the essential fops are read, write
> +transporting IR data to and fro, the essential fops are read, write
>  and ioctl.</para>
>  
>  <para>Example dmesg output upon a driver registering w/LIRC:</para>
> @@ -29,14 +29,19 @@ and ioctl.</para>
>  <section id="lirc_read">
>  <title>LIRC read fop</title>
>  
> -<para>The lircd userspace daemon reads raw IR data from the LIRC chardev. The
> -exact format of the data depends on what modes a driver supports, and what
> -mode has been selected. lircd obtains supported modes and sets the active mode
> -via the ioctl interface, detailed at <xref linkend="lirc_ioctl"/>. The generally
> -preferred mode is LIRC_MODE_MODE2, in which packets containing an int value
> -describing an IR signal are read from the chardev.</para>
> +<para>The data read from the chardev is IR. The format depends on the rec mode;
> +this is either in LIRC_MODE_MODE2, LIRC_MODE_SCANCODE or both. In MODE2, data
> +is read as 32 bit unsigned values. The highest 8 bits signifies the type:
> +LIRC_MODE2_PULSE, LIRC_MODE2_SPACE, LIRC_MODE2_FREQUENCY, LIRC_MODE2_TIMEOUT.
> +The lower 24 bits signify the value either in Hertz or nanoseconds.
> +</para>
> +<para>If LIRC_MODE_SCANCODE is enabled then the type in the highest 8 bits
> +is LIRC_MODE2_SCANCODE. The 24 bit signifies a repeat, 23 bit toggle set and
> +the lowest 8 bits is the rc protocol (see rc_type in rc-core.h). The next full
> +unsigned int is the scancode; there is no type in the highest 8 bits.
> +</para>
> +<para>The mode can be set and get using <xref linkend="lirc_ioctl"/>. </para>
>  
> -<para>See also <ulink url="http://www.lirc.org/html/technical.html">http://www.lirc.org/html/technical.html</ulink> for more info.</para>
>  </section>
>  
>  <section id="lirc_write">
> @@ -82,10 +87,12 @@ on working with the default settings initially.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> -    <term>LIRC_GET_REC_MODE</term>
> +    <term>LIRC_{G,S}ET_REC_MODE</term>
>      <listitem>
> -      <para>Get supported receive modes. Only LIRC_MODE_MODE2 and LIRC_MODE_LIRCCODE
> -      are supported by lircd.</para>
> +      <para>Get or set the receive mode. Devices that support raw IR will
> +      support LIRC_MODE_MODE2; all devices support LIRC_MODE_SCANCODE. Note
> +      that both modes can be enabled by ORing. That way, both raw IR and
> +      decoded scancodes can be read simultaneously.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index efdd6f7..247c22c 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -13,7 +13,7 @@ config LIRC
>  	---help---
>  	   Enable this option to build the Linux Infrared Remote
>  	   Control (LIRC) core device interface driver. The LIRC
> -	   interface passes raw IR to and from userspace, where the
> +	   interface passes IR to and from userspace, where the
>  	   LIRC daemon handles protocol decoding for IR reception and
>  	   encoding for IR transmitting (aka "blasting").
>  
> @@ -24,7 +24,7 @@ config IR_LIRC_CODEC
>  	default y
>  
>  	---help---
> -	   Enable this option to pass raw IR to and from userspace via
> +	   Enable this option to pass IR to and from userspace via
>  	   the LIRC interface.
>  
>  menuconfig RC_DECODERS
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 475f6af..594535e 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -36,6 +36,8 @@ int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  
>  	if (!lirc || !lirc->rbuf)
>  		return -EINVAL;
> +	if (!(dev->rec_mode & LIRC_MODE_MODE2))
> +		return 0;
>  
>  	/* Packet start */
>  	if (ev.reset) {
> @@ -101,6 +103,26 @@ int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  	return 0;
>  }
>  
> +void ir_lirc_scancode(struct rc_dev *dev, enum rc_type proto, u32 scancode,
> +						bool toggle, bool repeat)
> +{
> +	struct lirc_driver *lirc = dev->lirc;
> +	int sample;
> +
> +	if (!lirc || !lirc->rbuf || !(dev->rec_mode & LIRC_MODE_SCANCODE))
> +		return;
> +
> +	sample = LIRC_MODE2_SCANCODE | proto;
> +	if (toggle)
> +		sample |= LIRC_SCANCODE_TOGGLE;
> +	if (repeat)
> +		sample |= LIRC_SCANCODE_REPEAT;
> +
> +	lirc_buffer_write(lirc->rbuf, (unsigned char *) &sample);
> +	lirc_buffer_write(lirc->rbuf, (unsigned char *) &scancode);
> +	wake_up(&lirc->rbuf->wait_poll);
> +}
> +
>  static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  				   size_t n, loff_t *ppos)
>  {
> @@ -206,6 +228,20 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  
>  		return 0;
>  
> +	case LIRC_GET_REC_MODE:
> +		val = dev->rec_mode;
> +		break;
> +
> +	case LIRC_SET_REC_MODE:
> +		if (val == 0 ||
> +			val & ~(LIRC_MODE_MODE2 | LIRC_MODE_SCANCODE) ||
> +			(!(dev->lirc->features & LIRC_CAN_REC_MODE2) &&
> +					(val & LIRC_MODE_MODE2)))
> +			return -EINVAL;
> +
> +		dev->rec_mode = val;
> +		return 0;
> +
>  	/* TX settings */
>  	case LIRC_SET_TRANSMITTER_MASK:
>  		if (!dev->s_tx_mask)
> @@ -346,7 +382,9 @@ int ir_lirc_register(struct rc_dev *dev)
>  	if (rc)
>  		goto rbuf_init_failed;
>  
> -	features = LIRC_CAN_REC_MODE2;
> +	features = LIRC_CAN_REC_SCANCODE;
> +	if (dev->driver_type == RC_DRIVER_IR_RAW)
> +		features |= LIRC_CAN_REC_MODE2;
>  	if (dev->tx_ir) {
>  		features |= LIRC_CAN_SEND_PULSE;
>  		if (dev->s_tx_mask)
> @@ -374,6 +412,8 @@ int ir_lirc_register(struct rc_dev *dev)
>  		 dev->driver_name);
>  	drv->minor = -1;
>  	drv->features = features;
> +	dev->rec_mode = features & LIRC_CAN_REC_MODE2 ?
> +					LIRC_MODE_MODE2 : LIRC_MODE2_SCANCODE;
>  	drv->data = dev;
>  	drv->rbuf = rbuf;
>  	drv->set_use_inc = &ir_lirc_open;
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 732479d..f613306 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -223,11 +223,15 @@ static inline void load_xmp_decode(void) { }
>  void ir_lirc_unregister(struct rc_dev *dev);
>  int ir_lirc_register(struct rc_dev *dev);
>  int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev);
> +void ir_lirc_scancode(struct rc_dev *dev, enum rc_type proto, u32 scancode,
> +						bool toggle, bool repeat);
>  #else
>  static inline void ir_lirc_unregister(struct rc_dev *dev) {}
>  static inline int ir_lirc_register(struct rc_dev *dev) { return 0; }
>  static inline int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  								{ return 0; }
> +static inline void ir_lirc_scancode(struct rc_dev *dev, enum rc_type proto,
> +				u32 scancode, bool toggle, bool repeat) {}
>  #endif
>  
>  #ifdef CONFIG_LIRC
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index e717dc9..483038b 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -606,6 +606,10 @@ void rc_repeat(struct rc_dev *dev)
>  
>  	spin_lock_irqsave(&dev->keylock, flags);
>  
> +	if (dev->lirc)
> +		ir_lirc_scancode(dev, dev->last_protocol, dev->last_scancode,
> +								false, true);
> +
>  	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
>  	input_sync(dev->input_dev);
>  
> @@ -642,6 +646,9 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
>  	if (new_event && dev->keypressed)
>  		ir_do_keyup(dev, false);
>  
> +	if (dev->lirc)
> +		ir_lirc_scancode(dev, protocol, scancode, toggle, false);
> +
>  	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
>  
>  	if (new_event && keycode != KEY_RESERVED) {
> @@ -1416,12 +1423,12 @@ int rc_register_device(struct rc_dev *dev)
>  		mutex_lock(&dev->lock);
>  		if (rc < 0)
>  			goto out_input;
> -
> -		rc = ir_lirc_register(dev);
> -		if (rc < 0)
> -			goto out_raw;
>  	}
>  
> +	rc = ir_lirc_register(dev);
> +	if (rc < 0)
> +		goto out_raw;
> +
>  	if (dev->change_protocol) {
>  		u64 rc_type = (1ll << rc_map->rc_type);
>  		rc = dev->change_protocol(dev, &rc_type);
> diff --git a/include/media/lirc.h b/include/media/lirc.h
> index 7b845f8..a635fc9 100644
> --- a/include/media/lirc.h
> +++ b/include/media/lirc.h
> @@ -16,6 +16,7 @@
>  #define LIRC_MODE2_PULSE     0x01000000
>  #define LIRC_MODE2_FREQUENCY 0x02000000
>  #define LIRC_MODE2_TIMEOUT   0x03000000
> +#define LIRC_MODE2_SCANCODE  0x04000000
>  
>  #define LIRC_VALUE_MASK      0x00FFFFFF
>  #define LIRC_MODE2_MASK      0xFF000000
> @@ -32,6 +33,11 @@
>  #define LIRC_IS_PULSE(val) (LIRC_MODE2(val) == LIRC_MODE2_PULSE)
>  #define LIRC_IS_FREQUENCY(val) (LIRC_MODE2(val) == LIRC_MODE2_FREQUENCY)
>  #define LIRC_IS_TIMEOUT(val) (LIRC_MODE2(val) == LIRC_MODE2_TIMEOUT)
> +#define LIRC_IS_SCANCODE(val) (LIRC_MODE2(val) == LIRC_MODE2_SCANCODE)
> +
> +#define LIRC_SCANCODE_TOGGLE		0x00800000
> +#define LIRC_SCANCODE_REPEAT		0x00400000
> +#define LIRC_SCANCODE_PROTOCOL_MASK	0x000000ff
>  
>  /* used heavily by lirc userspace */
>  #define lirc_t int
> @@ -46,6 +52,7 @@
>  #define LIRC_MODE_RAW                  0x00000001
>  #define LIRC_MODE_PULSE                0x00000002
>  #define LIRC_MODE_MODE2                0x00000004
> +#define LIRC_MODE_SCANCODE             0x00000008
>  #define LIRC_MODE_LIRCCODE             0x00000010
>  
>  
> @@ -64,6 +71,7 @@
>  #define LIRC_CAN_REC_PULSE             LIRC_MODE2REC(LIRC_MODE_PULSE)
>  #define LIRC_CAN_REC_MODE2             LIRC_MODE2REC(LIRC_MODE_MODE2)
>  #define LIRC_CAN_REC_LIRCCODE          LIRC_MODE2REC(LIRC_MODE_LIRCCODE)
> +#define LIRC_CAN_REC_SCANCODE          LIRC_MODE2REC(LIRC_MODE_SCANCODE)
>  
>  #define LIRC_CAN_REC_MASK              LIRC_MODE2REC(LIRC_CAN_SEND_MASK)
>  
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index e3f217c..a8cef8c 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -163,6 +163,7 @@ struct rc_dev {
>  	u64				gap_duration;
>  	bool				gap;
>  	bool				send_timeout_reports;
> +	u32				rec_mode;
>  	int				(*change_protocol)(struct rc_dev *dev, u64 *rc_type);
>  	int				(*change_wakeup_protocol)(struct rc_dev *dev, u64 *rc_type);
>  	int				(*open)(struct rc_dev *dev);

[RFC PATCH 6/6] [media] rc: teach lirc how to send scancodes

[Sean Young]

The send mode has to be switched to LIRC_MODE_SCANCODE and then you can
send one scancode with a write. The encoding is the same as for receiving
scancodes.

FIXME: Currently only the nec encoder can encode IR.
FIXME: The "decoders" should be renamed (codec?)

Signed-off-by: Sean Young <sean@mess.org>
---
 .../DocBook/media/v4l/lirc_device_interface.xml    | 39 +++++++++--------
 drivers/media/rc/ir-lirc-codec.c                   | 49 +++++++++++++++------
 drivers/media/rc/ir-nec-decoder.c                  | 50 ++++++++++++++++++++++
 drivers/media/rc/rc-core-priv.h                    |  1 +
 drivers/media/rc/rc-ir-raw.c                       | 19 ++++++++
 include/media/lirc.h                               |  1 +
 include/media/rc-core.h                            |  3 +-
 7 files changed, 132 insertions(+), 30 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/lirc_device_interface.xml b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
index f92b5a5..e7f8139 100644
--- a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
+++ b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
@@ -37,8 +37,8 @@ The lower 24 bits signify the value either in Hertz or nanoseconds.
 </para>
 <para>If LIRC_MODE_SCANCODE is enabled then the type in the highest 8 bits
 is LIRC_MODE2_SCANCODE. The 24 bit signifies a repeat, 23 bit toggle set and
-the lowest 8 bits is the rc protocol (see rc_type in rc-core.h). The next full
-unsigned int is the scancode; there is no type in the highest 8 bits.
+the lowest 8 bits is the rc protocol (see enum rc_type in rc-map.h). The next
+full unsigned int is the scancode; there is no type in the highest 8 bits.
 </para>
 <para>The mode can be set and get using <xref linkend="lirc_ioctl"/>. </para>
 
@@ -47,13 +47,24 @@ unsigned int is the scancode; there is no type in the highest 8 bits.
 <section id="lirc_write">
 <title>LIRC write fop</title>
 
-<para>The data written to the chardev is a pulse/space sequence of integer
-values. Pulses and spaces are only marked implicitly by their position. The
-data must start and end with a pulse, therefore, the data must always include
-an uneven number of samples. The write function must block until the data has
-been transmitted by the hardware. If more data is provided than the hardware
-can send, the driver returns EINVAL.</para>
+<para>
+This is for sending or "blasting" IR. The format depends on the send mode,
+this is either LIRC_MODE_PULSE or LIRC_MODE_SCANCODE. The mode can be set
+and get using <xref linkend="lirc_ioctl"/>. </para>
+</para>
+<para>In LIRC_MODE_PULSE, the data written to the chardev is a pulse/space
+sequence of integer values. Pulses and spaces are only marked implicitly by
+their position. The data must start and end with a pulse, therefore, the
+data must always include an uneven number of samples. The write function
+must block until the data has been transmitted by the hardware. If more data
+is provided than the hardware can send, the driver returns EINVAL.</para>
 
+<para>In LIRC_MODE_SCANCODE, two 32 bit unsigneds must be written. The
+first unsigned must have it highest 8 bits set to LIRC_MODE2_SCANCODE and
+the lowest 8 bit signify the rc protocol (see enum rc_type in rc-map.h).
+If the protocol supports repeats then that can be set using
+LIRC_SCANCODE_REPEAT and the same for LIRC_SCANCODE_TOGGLE. The next 32 bit
+unsigned is the scancode.
 </section>
 
 <section id="lirc_ioctl">
@@ -81,9 +92,10 @@ on working with the default settings initially.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
-    <term>LIRC_GET_SEND_MODE</term>
+    <term>LIRC_{G,S}ET_SEND_MODE</term>
     <listitem>
-      <para>Get supported transmit mode. Only LIRC_MODE_PULSE is supported by lircd.</para>
+      <para>Get or set the send mode. This can either be LIRC_MODE_PULSE or
+      LIRC_MODE_SCANCODE. </para>
     </listitem>
   </varlistentry>
   <varlistentry>
@@ -155,13 +167,6 @@ on working with the default settings initially.</para>
     </listitem>
   </varlistentry>
   <varlistentry>
-    <term>LIRC_SET_{SEND,REC}_MODE</term>
-    <listitem>
-      <para>Set send/receive mode. Largely obsolete for send, as only
-      LIRC_MODE_PULSE is supported.</para>
-    </listitem>
-  </varlistentry>
-  <varlistentry>
     <term>LIRC_SET_{SEND,REC}_CARRIER</term>
     <listitem>
       <para>Set send/receive carrier (in Hz).</para>
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 594535e..14d9b41 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -141,16 +141,39 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 	if (!dev || !dev->lirc)
 		return -EFAULT;
 
-	if (n < sizeof(unsigned) || n % sizeof(unsigned))
-		return -EINVAL;
+	if (dev->send_mode == LIRC_MODE_SCANCODE) {
+		unsigned c[2];
 
-	count = n / sizeof(unsigned);
-	if (count > LIRCBUF_SIZE || count % 2 == 0)
-		return -EINVAL;
+		if (n != sizeof(unsigned) * 2)
+			return -EINVAL;
+
+		ret = copy_from_user(c, buf, n);
+		if (ret)
+			return ret;
+
+		txbuf = kmalloc(GFP_KERNEL, sizeof(unsigned) * LIRCBUF_SIZE);
+		if (!txbuf)
+			return -ENOMEM;
 
-	txbuf = memdup_user(buf, n);
-	if (IS_ERR(txbuf))
-		return PTR_ERR(txbuf);
+		ret = ir_raw_encode(c[0] & LIRC_SCANCODE_PROTOCOL_MASK, c[1],
+				(c[0] & LIRC_SCANCODE_REPEAT) != 0,
+				(c[0] & LIRC_SCANCODE_TOGGLE) != 0,
+				txbuf, LIRCBUF_SIZE);
+		if (ret < 0)
+			goto out;
+		count = ret;
+	} else {
+		if (n < sizeof(unsigned) || n % sizeof(unsigned))
+			return -EINVAL;
+
+		count = n / sizeof(unsigned);
+		if (count > LIRCBUF_SIZE || count % 2 == 0)
+			return -EINVAL;
+
+		txbuf = memdup_user(buf, n);
+		if (IS_ERR(txbuf))
+			return PTR_ERR(txbuf);
+	}
 
 	if (!dev->tx_ir) {
 		ret = -ENOSYS;
@@ -173,7 +196,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 	for (duration = i = 0; i < ret; i++)
 		duration += txbuf[i];
 
-	ret *= sizeof(unsigned int);
+	ret = n;
 
 	/*
 	 * The lircd gap calculation expects the write function to
@@ -216,16 +239,17 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
 			return -ENOSYS;
 
-		val = LIRC_MODE_PULSE;
+		val = dev->send_mode;
 		break;
 
 	case LIRC_SET_SEND_MODE:
 		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
 			return -ENOSYS;
 
-		if (val != LIRC_MODE_PULSE)
+		if (val != LIRC_MODE_PULSE && val != LIRC_MODE_SCANCODE)
 			return -EINVAL;
 
+		dev->send_mode = val;
 		return 0;
 
 	case LIRC_GET_REC_MODE:
@@ -386,13 +410,14 @@ int ir_lirc_register(struct rc_dev *dev)
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		features |= LIRC_CAN_REC_MODE2;
 	if (dev->tx_ir) {
-		features |= LIRC_CAN_SEND_PULSE;
+		features |= LIRC_CAN_SEND_PULSE | LIRC_CAN_SEND_SCANCODE;
 		if (dev->s_tx_mask)
 			features |= LIRC_CAN_SET_TRANSMITTER_MASK;
 		if (dev->s_tx_carrier)
 			features |= LIRC_CAN_SET_SEND_CARRIER;
 		if (dev->s_tx_duty_cycle)
 			features |= LIRC_CAN_SET_SEND_DUTY_CYCLE;
+		dev->send_mode = LIRC_MODE_PULSE;
 	}
 
 	if (dev->s_rx_carrier_range)
diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 7b81fec..1232084 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -200,9 +200,59 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 	return -EINVAL;
 }
 
+static int ir_nec_encode(enum rc_type type, u32 scancode, bool repeat,
+				bool toggle, unsigned *buf, unsigned count)
+{
+	unsigned i = 0, bits;
+
+	if (type != RC_TYPE_NEC)
+		return -EINVAL;
+
+	if (count < 3)
+		return -ENOSPC;
+
+	if (repeat) {
+		buf[i++] = NEC_HEADER_PULSE;
+		buf[i++] = NEC_REPEAT_SPACE;
+		buf[i++] = STATE_TRAILER_PULSE;
+		return 3;
+	}
+
+	if (count < 3 + NEC_NBITS * 2)
+		return -ENOSPC;
+
+	if (scancode < 0x10000) { /* normal NEC */
+		u32 cmd, addr;
+
+		addr = (scancode >> 8) & 0xff;
+		cmd = scancode & 0xff;
+
+		scancode = addr << 24 | (~addr & 0xff) << 16 |
+						cmd << 8 | (~cmd & 0xff);
+	} else if (scancode < 0x1000000) { /* extended NEC */
+		u32 cmd = scancode & 0xff;
+
+		scancode = (scancode & 0xffff00) << 8 |
+						(cmd << 8) | (~cmd & 0xff);
+	}
+
+	buf[i++] = NEC_HEADER_PULSE;
+	buf[i++] = NEC_HEADER_SPACE;
+	for (bits = 0; bits < NEC_NBITS; bits++) {
+		buf[i++] = NEC_BIT_PULSE;
+		buf[i++] = (scancode & 0x80000000) ?
+					NEC_BIT_1_SPACE : NEC_BIT_0_SPACE;
+		scancode <<= 1;
+	}
+	buf[i++] = STATE_TRAILER_PULSE;
+
+	return i;
+}
+
 static struct ir_raw_handler nec_handler = {
 	.protocols	= RC_BIT_NEC,
 	.decode		= ir_nec_decode,
+	.encode		= ir_nec_encode,
 };
 
 static int __init ir_nec_decode_init(void)
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index f613306..82d9132 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -25,6 +25,7 @@ struct ir_raw_handler {
 
 	u64 protocols; /* which are handled by this handler */
 	int (*decode)(struct rc_dev *dev, struct ir_raw_event event);
+	int (*encode)(enum rc_type protocol, u32 scancode, bool repeat, bool toggle, unsigned *buf, unsigned size);
 
 	/* These two should only be used by the mce kbd decoder */
 	int (*raw_register)(struct rc_dev *dev);
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index d298be7..d4e2144 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -351,6 +351,25 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
 }
 EXPORT_SYMBOL(ir_raw_handler_unregister);
 
+int ir_raw_encode(enum rc_type type, u32 scancode, bool repeat, bool toggle,
+					unsigned *buf, unsigned size)
+{
+	struct ir_raw_handler *handler;
+	u64 protocol = 1ull << type;
+	int ret = -ENOSYS;
+
+	mutex_lock(&ir_raw_handler_lock);
+	list_for_each_entry(handler, &ir_raw_handler_list, list) {
+		if (handler->protocols & protocol && handler->encode) {
+			ret = handler->encode(type, scancode, repeat, toggle,
+								buf, size);
+			break;
+		}
+	}
+	mutex_unlock(&ir_raw_handler_lock);
+	return ret;
+}
+
 void ir_raw_init(void)
 {
 	/* Load the decoder modules */
diff --git a/include/media/lirc.h b/include/media/lirc.h
index a635fc9..3807ded 100644
--- a/include/media/lirc.h
+++ b/include/media/lirc.h
@@ -60,6 +60,7 @@
 #define LIRC_CAN_SEND_PULSE            LIRC_MODE2SEND(LIRC_MODE_PULSE)
 #define LIRC_CAN_SEND_MODE2            LIRC_MODE2SEND(LIRC_MODE_MODE2)
 #define LIRC_CAN_SEND_LIRCCODE         LIRC_MODE2SEND(LIRC_MODE_LIRCCODE)
+#define LIRC_CAN_SEND_SCANCODE         LIRC_MODE2SEND(LIRC_MODE_SCANCODE)
 
 #define LIRC_CAN_SEND_MASK             0x0000003f
 
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index a8cef8c..601a5ba 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -163,7 +163,7 @@ struct rc_dev {
 	u64				gap_duration;
 	bool				gap;
 	bool				send_timeout_reports;
-	u32				rec_mode;
+	u32				rec_mode, send_mode;
 	int				(*change_protocol)(struct rc_dev *dev, u64 *rc_type);
 	int				(*change_wakeup_protocol)(struct rc_dev *dev, u64 *rc_type);
 	int				(*open)(struct rc_dev *dev);
@@ -258,6 +258,7 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type);
 int ir_raw_event_store_with_filter(struct rc_dev *dev,
 				struct ir_raw_event *ev);
 void ir_raw_event_set_idle(struct rc_dev *dev, bool idle);
+int ir_raw_encode(enum rc_type type, u32 scancode, bool repeat, bool toggle, unsigned *buf, unsigned size);
 
 static inline void ir_raw_event_reset(struct rc_dev *dev)
 {
-- 
2.1.0

Re: [RFC PATCH 6/6] [media] rc: teach lirc how to send scancodes

[Mauro Carvalho Chehab]

Em Thu, 19 Mar 2015 21:50:17 +0000
Sean Young <sean@mess.org> escreveu:

> The send mode has to be switched to LIRC_MODE_SCANCODE and then you can
> send one scancode with a write. The encoding is the same as for receiving
> scancodes.
> 
> FIXME: Currently only the nec encoder can encode IR.
> FIXME: The "decoders" should be renamed (codec?)

The FIXMEs should of course be addressed to merge this one ;)

The patch's idea makes sense to me, but I'll let to review it on a next
spin of this patch series.

Re: [RFC PATCH 6/6] [media] rc: teach lirc how to send scancodes

[David Härdeman]

On 2015-03-19 22:50, Sean Young wrote:
> The send mode has to be switched to LIRC_MODE_SCANCODE and then you can
> send one scancode with a write. The encoding is the same as for 
> receiving
> scancodes.

Why do the encoding in-kernel when it can be done in userspace?

I'd understand if it was hardware that accepted a scancode as input, but 
that doesn't seem to be the case?

> FIXME: Currently only the nec encoder can encode IR.
> FIXME: The "decoders" should be renamed (codec?)
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  .../DocBook/media/v4l/lirc_device_interface.xml    | 39 
> +++++++++--------
>  drivers/media/rc/ir-lirc-codec.c                   | 49 
> +++++++++++++++------
>  drivers/media/rc/ir-nec-decoder.c                  | 50 
> ++++++++++++++++++++++
>  drivers/media/rc/rc-core-priv.h                    |  1 +
>  drivers/media/rc/rc-ir-raw.c                       | 19 ++++++++
>  include/media/lirc.h                               |  1 +
>  include/media/rc-core.h                            |  3 +-
>  7 files changed, 132 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> index f92b5a5..e7f8139 100644
> --- a/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> +++ b/Documentation/DocBook/media/v4l/lirc_device_interface.xml
> @@ -37,8 +37,8 @@ The lower 24 bits signify the value either in Hertz
> or nanoseconds.
>  </para>
>  <para>If LIRC_MODE_SCANCODE is enabled then the type in the highest 8 
> bits
>  is LIRC_MODE2_SCANCODE. The 24 bit signifies a repeat, 23 bit toggle 
> set and
> -the lowest 8 bits is the rc protocol (see rc_type in rc-core.h). The 
> next full
> -unsigned int is the scancode; there is no type in the highest 8 bits.
> +the lowest 8 bits is the rc protocol (see enum rc_type in rc-map.h). 
> The next
> +full unsigned int is the scancode; there is no type in the highest 8 
> bits.
>  </para>
>  <para>The mode can be set and get using <xref linkend="lirc_ioctl"/>. 
> </para>
> 
> @@ -47,13 +47,24 @@ unsigned int is the scancode; there is no type in
> the highest 8 bits.
>  <section id="lirc_write">
>  <title>LIRC write fop</title>
> 
> -<para>The data written to the chardev is a pulse/space sequence of 
> integer
> -values. Pulses and spaces are only marked implicitly by their 
> position. The
> -data must start and end with a pulse, therefore, the data must always 
> include
> -an uneven number of samples. The write function must block until the 
> data has
> -been transmitted by the hardware. If more data is provided than the 
> hardware
> -can send, the driver returns EINVAL.</para>
> +<para>
> +This is for sending or "blasting" IR. The format depends on the send 
> mode,
> +this is either LIRC_MODE_PULSE or LIRC_MODE_SCANCODE. The mode can be 
> set
> +and get using <xref linkend="lirc_ioctl"/>. </para>
> +</para>
> +<para>In LIRC_MODE_PULSE, the data written to the chardev is a 
> pulse/space
> +sequence of integer values. Pulses and spaces are only marked 
> implicitly by
> +their position. The data must start and end with a pulse, therefore, 
> the
> +data must always include an uneven number of samples. The write 
> function
> +must block until the data has been transmitted by the hardware. If 
> more data
> +is provided than the hardware can send, the driver returns 
> EINVAL.</para>
> 
> +<para>In LIRC_MODE_SCANCODE, two 32 bit unsigneds must be written. The
> +first unsigned must have it highest 8 bits set to LIRC_MODE2_SCANCODE 
> and
> +the lowest 8 bit signify the rc protocol (see enum rc_type in 
> rc-map.h).
> +If the protocol supports repeats then that can be set using
> +LIRC_SCANCODE_REPEAT and the same for LIRC_SCANCODE_TOGGLE. The next 
> 32 bit
> +unsigned is the scancode.
>  </section>
> 
>  <section id="lirc_ioctl">
> @@ -81,9 +92,10 @@ on working with the default settings 
> initially.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> -    <term>LIRC_GET_SEND_MODE</term>
> +    <term>LIRC_{G,S}ET_SEND_MODE</term>
>      <listitem>
> -      <para>Get supported transmit mode. Only LIRC_MODE_PULSE is
> supported by lircd.</para>
> +      <para>Get or set the send mode. This can either be 
> LIRC_MODE_PULSE or
> +      LIRC_MODE_SCANCODE. </para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> @@ -155,13 +167,6 @@ on working with the default settings 
> initially.</para>
>      </listitem>
>    </varlistentry>
>    <varlistentry>
> -    <term>LIRC_SET_{SEND,REC}_MODE</term>
> -    <listitem>
> -      <para>Set send/receive mode. Largely obsolete for send, as only
> -      LIRC_MODE_PULSE is supported.</para>
> -    </listitem>
> -  </varlistentry>
> -  <varlistentry>
>      <term>LIRC_SET_{SEND,REC}_CARRIER</term>
>      <listitem>
>        <para>Set send/receive carrier (in Hz).</para>
> diff --git a/drivers/media/rc/ir-lirc-codec.c 
> b/drivers/media/rc/ir-lirc-codec.c
> index 594535e..14d9b41 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -141,16 +141,39 @@ static ssize_t ir_lirc_transmit_ir(struct file
> *file, const char __user *buf,
>  	if (!dev || !dev->lirc)
>  		return -EFAULT;
> 
> -	if (n < sizeof(unsigned) || n % sizeof(unsigned))
> -		return -EINVAL;
> +	if (dev->send_mode == LIRC_MODE_SCANCODE) {
> +		unsigned c[2];
> 
> -	count = n / sizeof(unsigned);
> -	if (count > LIRCBUF_SIZE || count % 2 == 0)
> -		return -EINVAL;
> +		if (n != sizeof(unsigned) * 2)
> +			return -EINVAL;
> +
> +		ret = copy_from_user(c, buf, n);
> +		if (ret)
> +			return ret;
> +
> +		txbuf = kmalloc(GFP_KERNEL, sizeof(unsigned) * LIRCBUF_SIZE);
> +		if (!txbuf)
> +			return -ENOMEM;
> 
> -	txbuf = memdup_user(buf, n);
> -	if (IS_ERR(txbuf))
> -		return PTR_ERR(txbuf);
> +		ret = ir_raw_encode(c[0] & LIRC_SCANCODE_PROTOCOL_MASK, c[1],
> +				(c[0] & LIRC_SCANCODE_REPEAT) != 0,
> +				(c[0] & LIRC_SCANCODE_TOGGLE) != 0,
> +				txbuf, LIRCBUF_SIZE);
> +		if (ret < 0)
> +			goto out;
> +		count = ret;
> +	} else {
> +		if (n < sizeof(unsigned) || n % sizeof(unsigned))
> +			return -EINVAL;
> +
> +		count = n / sizeof(unsigned);
> +		if (count > LIRCBUF_SIZE || count % 2 == 0)
> +			return -EINVAL;
> +
> +		txbuf = memdup_user(buf, n);
> +		if (IS_ERR(txbuf))
> +			return PTR_ERR(txbuf);
> +	}
> 
>  	if (!dev->tx_ir) {
>  		ret = -ENOSYS;
> @@ -173,7 +196,7 @@ static ssize_t ir_lirc_transmit_ir(struct file
> *file, const char __user *buf,
>  	for (duration = i = 0; i < ret; i++)
>  		duration += txbuf[i];
> 
> -	ret *= sizeof(unsigned int);
> +	ret = n;
> 
>  	/*
>  	 * The lircd gap calculation expects the write function to
> @@ -216,16 +239,17 @@ static long ir_lirc_ioctl(struct file *filep,
> unsigned int cmd,
>  		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
>  			return -ENOSYS;
> 
> -		val = LIRC_MODE_PULSE;
> +		val = dev->send_mode;
>  		break;
> 
>  	case LIRC_SET_SEND_MODE:
>  		if (!(dev->lirc->features & LIRC_CAN_SEND_MASK))
>  			return -ENOSYS;
> 
> -		if (val != LIRC_MODE_PULSE)
> +		if (val != LIRC_MODE_PULSE && val != LIRC_MODE_SCANCODE)
>  			return -EINVAL;
> 
> +		dev->send_mode = val;
>  		return 0;
> 
>  	case LIRC_GET_REC_MODE:
> @@ -386,13 +410,14 @@ int ir_lirc_register(struct rc_dev *dev)
>  	if (dev->driver_type == RC_DRIVER_IR_RAW)
>  		features |= LIRC_CAN_REC_MODE2;
>  	if (dev->tx_ir) {
> -		features |= LIRC_CAN_SEND_PULSE;
> +		features |= LIRC_CAN_SEND_PULSE | LIRC_CAN_SEND_SCANCODE;
>  		if (dev->s_tx_mask)
>  			features |= LIRC_CAN_SET_TRANSMITTER_MASK;
>  		if (dev->s_tx_carrier)
>  			features |= LIRC_CAN_SET_SEND_CARRIER;
>  		if (dev->s_tx_duty_cycle)
>  			features |= LIRC_CAN_SET_SEND_DUTY_CYCLE;
> +		dev->send_mode = LIRC_MODE_PULSE;
>  	}
> 
>  	if (dev->s_rx_carrier_range)
> diff --git a/drivers/media/rc/ir-nec-decoder.c
> b/drivers/media/rc/ir-nec-decoder.c
> index 7b81fec..1232084 100644
> --- a/drivers/media/rc/ir-nec-decoder.c
> +++ b/drivers/media/rc/ir-nec-decoder.c
> @@ -200,9 +200,59 @@ static int ir_nec_decode(struct rc_dev *dev,
> struct ir_raw_event ev)
>  	return -EINVAL;
>  }
> 
> +static int ir_nec_encode(enum rc_type type, u32 scancode, bool repeat,
> +				bool toggle, unsigned *buf, unsigned count)
> +{
> +	unsigned i = 0, bits;
> +
> +	if (type != RC_TYPE_NEC)
> +		return -EINVAL;
> +
> +	if (count < 3)
> +		return -ENOSPC;
> +
> +	if (repeat) {
> +		buf[i++] = NEC_HEADER_PULSE;
> +		buf[i++] = NEC_REPEAT_SPACE;
> +		buf[i++] = STATE_TRAILER_PULSE;
> +		return 3;
> +	}
> +
> +	if (count < 3 + NEC_NBITS * 2)
> +		return -ENOSPC;
> +
> +	if (scancode < 0x10000) { /* normal NEC */
> +		u32 cmd, addr;
> +
> +		addr = (scancode >> 8) & 0xff;
> +		cmd = scancode & 0xff;
> +
> +		scancode = addr << 24 | (~addr & 0xff) << 16 |
> +						cmd << 8 | (~cmd & 0xff);
> +	} else if (scancode < 0x1000000) { /* extended NEC */
> +		u32 cmd = scancode & 0xff;
> +
> +		scancode = (scancode & 0xffff00) << 8 |
> +						(cmd << 8) | (~cmd & 0xff);
> +	}
> +
> +	buf[i++] = NEC_HEADER_PULSE;
> +	buf[i++] = NEC_HEADER_SPACE;
> +	for (bits = 0; bits < NEC_NBITS; bits++) {
> +		buf[i++] = NEC_BIT_PULSE;
> +		buf[i++] = (scancode & 0x80000000) ?
> +					NEC_BIT_1_SPACE : NEC_BIT_0_SPACE;
> +		scancode <<= 1;
> +	}
> +	buf[i++] = STATE_TRAILER_PULSE;
> +
> +	return i;
> +}
> +
>  static struct ir_raw_handler nec_handler = {
>  	.protocols	= RC_BIT_NEC,
>  	.decode		= ir_nec_decode,
> +	.encode		= ir_nec_encode,
>  };
> 
>  static int __init ir_nec_decode_init(void)
> diff --git a/drivers/media/rc/rc-core-priv.h 
> b/drivers/media/rc/rc-core-priv.h
> index f613306..82d9132 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -25,6 +25,7 @@ struct ir_raw_handler {
> 
>  	u64 protocols; /* which are handled by this handler */
>  	int (*decode)(struct rc_dev *dev, struct ir_raw_event event);
> +	int (*encode)(enum rc_type protocol, u32 scancode, bool repeat, bool
> toggle, unsigned *buf, unsigned size);
> 
>  	/* These two should only be used by the mce kbd decoder */
>  	int (*raw_register)(struct rc_dev *dev);
> diff --git a/drivers/media/rc/rc-ir-raw.c 
> b/drivers/media/rc/rc-ir-raw.c
> index d298be7..d4e2144 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -351,6 +351,25 @@ void ir_raw_handler_unregister(struct
> ir_raw_handler *ir_raw_handler)
>  }
>  EXPORT_SYMBOL(ir_raw_handler_unregister);
> 
> +int ir_raw_encode(enum rc_type type, u32 scancode, bool repeat, bool 
> toggle,
> +					unsigned *buf, unsigned size)
> +{
> +	struct ir_raw_handler *handler;
> +	u64 protocol = 1ull << type;
> +	int ret = -ENOSYS;
> +
> +	mutex_lock(&ir_raw_handler_lock);
> +	list_for_each_entry(handler, &ir_raw_handler_list, list) {
> +		if (handler->protocols & protocol && handler->encode) {
> +			ret = handler->encode(type, scancode, repeat, toggle,
> +								buf, size);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&ir_raw_handler_lock);
> +	return ret;
> +}
> +
>  void ir_raw_init(void)
>  {
>  	/* Load the decoder modules */
> diff --git a/include/media/lirc.h b/include/media/lirc.h
> index a635fc9..3807ded 100644
> --- a/include/media/lirc.h
> +++ b/include/media/lirc.h
> @@ -60,6 +60,7 @@
>  #define LIRC_CAN_SEND_PULSE            LIRC_MODE2SEND(LIRC_MODE_PULSE)
>  #define LIRC_CAN_SEND_MODE2            LIRC_MODE2SEND(LIRC_MODE_MODE2)
>  #define LIRC_CAN_SEND_LIRCCODE         
> LIRC_MODE2SEND(LIRC_MODE_LIRCCODE)
> +#define LIRC_CAN_SEND_SCANCODE         
> LIRC_MODE2SEND(LIRC_MODE_SCANCODE)
> 
>  #define LIRC_CAN_SEND_MASK             0x0000003f
> 
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index a8cef8c..601a5ba 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -163,7 +163,7 @@ struct rc_dev {
>  	u64				gap_duration;
>  	bool				gap;
>  	bool				send_timeout_reports;
> -	u32				rec_mode;
> +	u32				rec_mode, send_mode;
>  	int				(*change_protocol)(struct rc_dev *dev, u64 *rc_type);
>  	int				(*change_wakeup_protocol)(struct rc_dev *dev, u64 *rc_type);
>  	int				(*open)(struct rc_dev *dev);
> @@ -258,6 +258,7 @@ int ir_raw_event_store_edge(struct rc_dev *dev,
> enum raw_event_type type);
>  int ir_raw_event_store_with_filter(struct rc_dev *dev,
>  				struct ir_raw_event *ev);
>  void ir_raw_event_set_idle(struct rc_dev *dev, bool idle);
> +int ir_raw_encode(enum rc_type type, u32 scancode, bool repeat, bool
> toggle, unsigned *buf, unsigned size);
> 
>  static inline void ir_raw_event_reset(struct rc_dev *dev)
>  {

Re: [RFC PATCH 6/6] [media] rc: teach lirc how to send scancodes

[Mauro Carvalho Chehab]

Em Wed, 20 May 2015 10:53:59 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On 2015-03-19 22:50, Sean Young wrote:
> > The send mode has to be switched to LIRC_MODE_SCANCODE and then you can
> > send one scancode with a write. The encoding is the same as for 
> > receiving
> > scancodes.
> 
> Why do the encoding in-kernel when it can be done in userspace?
> 
> I'd understand if it was hardware that accepted a scancode as input, but 
> that doesn't seem to be the case?

IMO, that makes the interface clearer. Also, the encoding code is needed
anyway, as it is needed to setup the wake up keycode on some hardware.
So, we already added encoder capabilities at some decoders:

0d830b2d1295 [media] rc: rc-core: Add support for encode_wakeup drivers
cf257e288ad3 [media] rc: ir-rc6-decoder: Add encode capability
a0466f15b465 [media] rc: ir-rc5-decoder: Add encode capability
1d971d927efa [media] rc: rc-ir-raw: Add Manchester encoder (phase encoder) helper
9869da5bacc5 [media] rc: rc-ir-raw: Add scancode encoder callback

> 
> > FIXME: Currently only the nec encoder can encode IR.

We actually need to be sure that the NEC encoder is doing the same
way as the RC5/RC6 encoders.

Regards,
Mauro

Re: [RFC PATCH 6/6] [media] rc: teach lirc how to send scancodes

[David Härdeman]

On 2015-05-20 11:08, Mauro Carvalho Chehab wrote:
> Em Wed, 20 May 2015 10:53:59 +0200
> David Härdeman <david@hardeman.nu> escreveu:
> 
>> On 2015-03-19 22:50, Sean Young wrote:
>> > The send mode has to be switched to LIRC_MODE_SCANCODE and then you can
>> > send one scancode with a write. The encoding is the same as for
>> > receiving
>> > scancodes.
>> 
>> Why do the encoding in-kernel when it can be done in userspace?
>> 
>> I'd understand if it was hardware that accepted a scancode as input, 
>> but
>> that doesn't seem to be the case?
> 
> IMO, that makes the interface clearer. Also, the encoding code is 
> needed
> anyway, as it is needed to setup the wake up keycode on some hardware.
> So, we already added encoder capabilities at some decoders:

I know.

But with the proposed API userspace would have to first try to send a 
scancode + protocol, then see what the error code was, and if it 
indicated that the kernel couldn't encode the scancode, userspace would 
anyway have to encode it itself and then try again with raw events?

I think that's a bad API (to put it mildly).

There's simply no need to encode the scancodes in kernel (even if the 
code happens to be present for a random and fluctuating set of 
protocols) and any well-written userspace would have to include code to 
encode to any protocol it wants to support (since it can't rely on the 
kernel supporting any given protocol)....what does that buy you except a 
hairy API and added complexity?

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[David Härdeman]

On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
>This patch series tries to fix the lirc interface and extend it so it can
>be used to send/recv scancodes in addition to raw IR:
>
> - Make it possible to receive scancodes from hardware that generates 
>   scancodes (with rc protocol information)
> - Make it possible to receive decoded scancodes from raw IR, from the 
>   in-kernel decoders (not mce mouse/keyboard). Now you can take any
>   remote, run ir-rec and you will get both the raw IR and the decoded
>   scancodes plus rc protocol information.
> - Make it possible to send scancodes; in kernel-encoding of IR
> - Make it possible to send scancodes for hardware that can only do that
>   (so that lirc_zilog can be moved out of staging, not completed yet)
> - Possibly the lirc interface can be used for cec?
>
>This requires a little refactoring.
> - All rc-core devices will have a lirc device associated with them
> - The rc-core <-> lirc bridge no longer is a "decoder", this never made 
>   sense and caused confusion

IIRC, it was written that way to make the lirc module optional.

>This requires new API for rc-core lirc devices.
> - New feature LIRC_CAN_REC_SCANCODE and LIRC_CAN_SEND_SCANCODE
> - REC_MODE and SEND_MODE do not enable LIRC_MODE_SCANCODE by default since 
>   this would confuse existing userspace code
> - For each scancode we need: 
>   - rc protocol (RC_TYPE_*) 
>   - toggle and repeat bit for some protocols
>   - 32 bit scancode

I haven't looked at the patches in detail, but I think exposing the
scancodes to userspace requires some careful consideration.

First of all, scancode length should be dynamic, there are protocols
(NEC48 and, at least theoretically, RC6) that have scancodes > 32 bits.

Second, if we expose protocol type (which we should, not doing so is
throwing away valuable information) we should tackle the NEC scancode
question. I've already explained my firm conviction that always
reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
is of the opinion that NEC16/24/32 should be essentially different
protocols.

Third, we should still have a way to represent the protocol in the
keymap as well.

And on a much more general level...I still think we should start from
scratch with a rc specific chardev with it's own API that is generic
enough to cover both scancode / raw ir / future / other protocols (not
that such an undertaking would preclude adding stuff to the lirc API of
course).

Re,
David

PS
Thanks for your continued RC efforts Sean!

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[Sean Young]

On Mon, Mar 30, 2015 at 11:18:19PM +0200, David Härdeman wrote:
> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
> >This patch series tries to fix the lirc interface and extend it so it can
> >be used to send/recv scancodes in addition to raw IR:
> >
> > - Make it possible to receive scancodes from hardware that generates 
> >   scancodes (with rc protocol information)
> > - Make it possible to receive decoded scancodes from raw IR, from the 
> >   in-kernel decoders (not mce mouse/keyboard). Now you can take any
> >   remote, run ir-rec and you will get both the raw IR and the decoded
> >   scancodes plus rc protocol information.
> > - Make it possible to send scancodes; in kernel-encoding of IR
> > - Make it possible to send scancodes for hardware that can only do that
> >   (so that lirc_zilog can be moved out of staging, not completed yet)
> > - Possibly the lirc interface can be used for cec?
> >
> >This requires a little refactoring.
> > - All rc-core devices will have a lirc device associated with them
> > - The rc-core <-> lirc bridge no longer is a "decoder", this never made 
> >   sense and caused confusion
> 
> IIRC, it was written that way to make the lirc module optional.

It does make the lirc module optional, but it does not make sense that
lirc is a decoder, when it handles other things like transmit and displaying
raw IR from userspace. An unsuspecting user will be surprised to learn
that he/she has to "echo +lirc > protocols" in order to lircd/mode2 to
work (this has come up on linux-media a couple of times).

More importantly it makes receiving decoded IR (from the other real
decoders) a real mess. That's why I chose this solution.

> >This requires new API for rc-core lirc devices.
> > - New feature LIRC_CAN_REC_SCANCODE and LIRC_CAN_SEND_SCANCODE
> > - REC_MODE and SEND_MODE do not enable LIRC_MODE_SCANCODE by default since 
> >   this would confuse existing userspace code
> > - For each scancode we need: 
> >   - rc protocol (RC_TYPE_*) 
> >   - toggle and repeat bit for some protocols
> >   - 32 bit scancode
> 
> I haven't looked at the patches in detail, but I think exposing the
> scancodes to userspace requires some careful consideration.
> 
> First of all, scancode length should be dynamic, there are protocols
> (NEC48 and, at least theoretically, RC6) that have scancodes > 32 bits.

Ok, interesting. We should allow for that.

> Second, if we expose protocol type (which we should, not doing so is
> throwing away valuable information) we should tackle the NEC scancode
> question. I've already explained my firm conviction that always
> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
> is of the opinion that NEC16/24/32 should be essentially different
> protocols.

In the patch set, I had to write a nec encoder called ir_nec_encode(). 
Since it does not know which type of nec it is, it has to guess based on
the width of the scancode. I thought it was pretty hideous. Your solution
of using 32 bit whatever the nec variant would be a strange API as well;
it would be much nicer if the nec variant could be specified for transmit.

> Third, we should still have a way to represent the protocol in the
> keymap as well.

Agreed; this work is separate from keymap handling though.

> And on a much more general level...I still think we should start from
> scratch with a rc specific chardev with it's own API that is generic
> enough to cover both scancode / raw ir / future / other protocols (not
> that such an undertaking would preclude adding stuff to the lirc API of
> course).

I hope my patches show that that adding scancodes with protocol information
is not difficult and it fits in with the general lirc design. I have
toyed with adding a new chardev but I don't know what it adds when the
lirc interface suffices. It just ends up looking like lirc with an new 
name and new ioctls.

The only thing it adds is nanosecond precision (rather than microseconds)
and being able to send IR signals ad infinitum if the hardware supports
it (not many). Neither of these are needed for real world IR use cases.

A new interface does not add anything useful, so why would anyone use it
rather than the existing lirc interface?

Thanks

Sean

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[David Härdeman]

On Tue, Mar 31, 2015 at 12:08:37AM +0100, Sean Young wrote:
>On Mon, Mar 30, 2015 at 11:18:19PM +0200, David Härdeman wrote:
>> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
>> >This patch series tries to fix the lirc interface and extend it so it can
>> >be used to send/recv scancodes in addition to raw IR:
>> >
>> > - Make it possible to receive scancodes from hardware that generates 
>> >   scancodes (with rc protocol information)
>> > - Make it possible to receive decoded scancodes from raw IR, from the 
>> >   in-kernel decoders (not mce mouse/keyboard). Now you can take any
>> >   remote, run ir-rec and you will get both the raw IR and the decoded
>> >   scancodes plus rc protocol information.
>> > - Make it possible to send scancodes; in kernel-encoding of IR
>> > - Make it possible to send scancodes for hardware that can only do that
>> >   (so that lirc_zilog can be moved out of staging, not completed yet)
>> > - Possibly the lirc interface can be used for cec?
>> >
>> >This requires a little refactoring.
>> > - All rc-core devices will have a lirc device associated with them
>> > - The rc-core <-> lirc bridge no longer is a "decoder", this never made 
>> >   sense and caused confusion
>> 
>> IIRC, it was written that way to make the lirc module optional.
>
>It does make the lirc module optional, but it does not make sense that
>lirc is a decoder, when it handles other things like transmit and displaying
>raw IR from userspace. An unsuspecting user will be surprised to learn
>that he/she has to "echo +lirc > protocols" in order to lircd/mode2 to
>work (this has come up on linux-media a couple of times).

Yes...I agree that it's suboptimal.

>More importantly it makes receiving decoded IR (from the other real
>decoders) a real mess. That's why I chose this solution.

rc-core was supposed (at least in theory) to support other remote
control schemes as well (RF...maybe CEC...etc), so I still think making
a lirc device optional for non-IR hardware makes sense.

>> >This requires new API for rc-core lirc devices.
>> > - New feature LIRC_CAN_REC_SCANCODE and LIRC_CAN_SEND_SCANCODE
>> > - REC_MODE and SEND_MODE do not enable LIRC_MODE_SCANCODE by default since 
>> >   this would confuse existing userspace code
>> > - For each scancode we need: 
>> >   - rc protocol (RC_TYPE_*) 
>> >   - toggle and repeat bit for some protocols
>> >   - 32 bit scancode
>> 
>> I haven't looked at the patches in detail, but I think exposing the
>> scancodes to userspace requires some careful consideration.
>> 
>> First of all, scancode length should be dynamic, there are protocols
>> (NEC48 and, at least theoretically, RC6) that have scancodes > 32 bits.
>
>Ok, interesting. We should allow for that.
>
>> Second, if we expose protocol type (which we should, not doing so is
>> throwing away valuable information) we should tackle the NEC scancode
>> question. I've already explained my firm conviction that always
>> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
>> is of the opinion that NEC16/24/32 should be essentially different
>> protocols.
>
>In the patch set, I had to write a nec encoder called ir_nec_encode(). 
>Since it does not know which type of nec it is, it has to guess based on
>the width of the scancode. I thought it was pretty hideous. Your solution
>of using 32 bit whatever the nec variant would be a strange API as well;

Why is it strange? The different NEC variants send/receive 32 bits "on
the wire". We are talking about the communication between userspace and
the kernel and I don't think there's anything weird about saying "these
bits have been received/should be sent" without any further
(unnecessary) parsing being done by the kernel before/after. Adding
separate NEC16/NEC24/NEC32 "protocols" (where the same 32 bits have been
parsed and mashed by the kernel) adds complexity to the kernel while
providing no benefits whatsoever.

>it would be much nicer if the nec variant could be specified for transmit.
>
>> Third, we should still have a way to represent the protocol in the
>> keymap as well.
>
>Agreed; this work is separate from keymap handling though.

Yes. Absolutely. But it's another aspect where the protocol numbers will
be exposed to userspace.

>> And on a much more general level...I still think we should start from
>> scratch with a rc specific chardev with it's own API that is generic
>> enough to cover both scancode / raw ir / future / other protocols (not
>> that such an undertaking would preclude adding stuff to the lirc API of
>> course).
>
>I hope my patches show that that adding scancodes with protocol information
>is not difficult and it fits in with the general lirc design. I have
>toyed with adding a new chardev but I don't know what it adds when the
>lirc interface suffices. It just ends up looking like lirc with an new 
>name and new ioctls.

The chardev I suggested does add a few things. Versioning. Explicit
support for different kinds of RC hardware (raw IR, scancode IR,
others), and it gives the opportunity to clean up the lirc ioctls
(they're pretty messy...a lot of them don't seem to ever have been used
while other functionality is missing...like the ability to set all TX/RX
parameters in one go).

>The only thing it adds is nanosecond precision (rather than microseconds)
>and being able to send IR signals ad infinitum if the hardware supports
>it (not many). Neither of these are needed for real world IR use cases.
>
>A new interface does not add anything useful, so why would anyone use it
>rather than the existing lirc interface?

I'll expand a bit more in a separate email....but I don't think my
proposal is really in the way of extending the lirc device as you
propose...it's rather something to keep in mind..

Regards,
David

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[Mauro Carvalho Chehab]

Em Mon, 30 Mar 2015 23:18:19 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
> >This patch series tries to fix the lirc interface and extend it so it can
> >be used to send/recv scancodes in addition to raw IR:

Haven't look at the series yet. I just arrived from ELC/Linux Media conf.

> >
> > - Make it possible to receive scancodes from hardware that generates 
> >   scancodes (with rc protocol information)
> > - Make it possible to receive decoded scancodes from raw IR, from the 
> >   in-kernel decoders (not mce mouse/keyboard). Now you can take any
> >   remote, run ir-rec and you will get both the raw IR and the decoded
> >   scancodes plus rc protocol information.
> > - Make it possible to send scancodes; in kernel-encoding of IR
> > - Make it possible to send scancodes for hardware that can only do that
> >   (so that lirc_zilog can be moved out of staging, not completed yet)
> > - Possibly the lirc interface can be used for cec?
> >
> >This requires a little refactoring.
> > - All rc-core devices will have a lirc device associated with them
> > - The rc-core <-> lirc bridge no longer is a "decoder", this never made 
> >   sense and caused confusion
> 
> IIRC, it was written that way to make the lirc module optional.

Yes, the idea is that LIRC is an option for most devices. For devices
with TX, however, it would make sense to always have an association, if
compiled with LIRC support (otherwise, TX would be disabled).

Maybe we could track this with an additional config option (CONFIG_IR_TX?).

> >This requires new API for rc-core lirc devices.
> > - New feature LIRC_CAN_REC_SCANCODE and LIRC_CAN_SEND_SCANCODE
> > - REC_MODE and SEND_MODE do not enable LIRC_MODE_SCANCODE by default since 
> >   this would confuse existing userspace code
> > - For each scancode we need: 
> >   - rc protocol (RC_TYPE_*) 
> >   - toggle and repeat bit for some protocols
> >   - 32 bit scancode
> 
> I haven't looked at the patches in detail, but I think exposing the
> scancodes to userspace requires some careful consideration.
> 
> First of all, scancode length should be dynamic, there are protocols
> (NEC48 and, at least theoretically, RC6) that have scancodes > 32 bits.

Good point.
 
> Second, if we expose protocol type (which we should, not doing so is
> throwing away valuable information) we should tackle the NEC scancode
> question. I've already explained my firm conviction that always
> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
> is of the opinion that NEC16/24/32 should be essentially different
> protocols.

Changing NEC would break userspace, as existing tables won't work.
So, no matter what I think, changing it won't happen as we're not
allowed to break userspace.

(and yes, I think NEC16 is *the* NEC protocol; the other are just
variants made by some vendors to fill their needs)

> Third, we should still have a way to represent the protocol in the
> keymap as well.

Not sure about that, but this is a different matter. 

> And on a much more general level...I still think we should start from
> scratch with a rc specific chardev with it's own API that is generic
> enough to cover both scancode / raw ir / future / other protocols (not
> that such an undertaking would preclude adding stuff to the lirc API of
> course).

Starting from scratch sounds a bad idea. We don't do that on Linux,
except when we really screwed everything very badly. Also, the input
developers already denied adding a separate chardev with its own API
when we started discussing about the remote controller core.

Adding a new chardev would make things very confusing, as we'll need
to keep reporting data on both new and old chardev. We have this already
for LIRC, but with different interfaces, so, no big issue. Also, LIRC
can be dynamically disabled at runtime. So, it seems that this is the
best approach, IMO.

> 
> Re,
> David
> 
> PS
> Thanks for your continued RC efforts Sean!
> 


-- 

Cheers,
Mauro

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[David Härdeman]

On Tue, Mar 31, 2015 at 08:47:16PM -0300, Mauro Carvalho Chehab wrote:
>Em Mon, 30 Mar 2015 23:18:19 +0200
>David Härdeman <david@hardeman.nu> escreveu:
>> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
>> Second, if we expose protocol type (which we should, not doing so is
>> throwing away valuable information) we should tackle the NEC scancode
>> question. I've already explained my firm conviction that always
>> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
>> is of the opinion that NEC16/24/32 should be essentially different
>> protocols.
>
>Changing NEC would break userspace, as existing tables won't work.
>So, no matter what I think, changing it won't happen as we're not
>allowed to break userspace.

I have no idea what breakage you're talking about. Sean's patches would
introduce new API, so they can't break anything. My patch series also
introduced a new API for setting/getting keytable entries (with
heuristics for the old ways to convert NEC scancodes on the fly) so it
should (hopefully) not break anything.

>(and yes, I think NEC16 is *the* NEC protocol; the other are just
>variants made by some vendors to fill their needs)

We are talking about the protocol used to communicate what has been
received/should be sent between userspace and the kernel. Simply passing
the 32 bits that have been sent/received is the simplest, most
straightfoward way to go.

>> Third, we should still have a way to represent the protocol in the
>> keymap as well.
>
>Not sure about that, but this is a different matter. 

Yes, it's a different matter. And what is there to be unsure about? Not
having the protocol as part of the keymap means throwing away
information...

>> And on a much more general level...I still think we should start from
>> scratch with a rc specific chardev with it's own API that is generic
>> enough to cover both scancode / raw ir / future / other protocols (not
>> that such an undertaking would preclude adding stuff to the lirc API of
>> course).
>
>Starting from scratch sounds a bad idea. We don't do that on Linux,
>except when we really screwed everything very badly.

LIRC...IR specific....rc-core....not IR specific...and the lirc IOCTL
API is pretty badly screwed. Have you had a closer look at it?

I'm not saying we should throw away the lirc module/device, it'll have
to stay around for a long time. But we should design a v2. If you've
looked at my patches the idea is basically:

RC device is plugged in, /dev/rc/rc0 is created by udev

Applications wishing to muck about with RC devices do:

	int fd;
	int ver;
	int type;

	fd = open("/dev/rc/rc0")

	ver = ioctl(fd, RCIOCGVERSION);
	if (ver != 1)
		warn("New RC API version");

	type = ioctl(fd, RCIOCGVERSION);
	switch (type) {
	case RC_DRIVER_SCANCODE:
		debug("Scancode hardware");
		break;
	case RC_DRIVER_IR_RAW:
		debug("Raw IR hardware");
		break;
	default:
		debug("Unknown hardware type");
		break;
	}

And then they can do further operations depending on the type of the
device. For example, for raw IR devices you can read() raw IR
pulse/space timings or (if the hardware supports TX) write() raw IR
timings.

Other examples of ioctls are (all four work using structs with all the
relevant parameters):

* RCIOCSIRRX
	set all RX parameters in one go (and return the result since
	the exact values requested might not be supported)
* RCIOCSIRTX
	same as RCIOCSIRRX but for TX
* RCIOCGIRRX
	get all RX parameters
* RCIOCGIRTX
	get all TX parameters

These ioctls only work with RC_DRIVER_IR_RAW hardware. Others can be
defined for other kinds of hardware.

Then there's one more thing, and that's multiple keytables per rc
device. Each keytable has one associated input device (so there's a
1-to-N mapping between rc devices and input devices). Userspace can
create/destroy additional keytables and add/remove scancode<->keycode
mappings per keytable. The idea is that you'd be able to e.g. define one
keytable per physical remote control (the thing you hold in your hand,
not the receiver/transmitter), and each would get its own input device.
Those input devices can then be used by different applications (so you
could have that old VCR remote control the PVR software while the TV
remote controls your Kodi frontend). An idea I borrowed from Jon Smirl
(who posted a similar proof-of-concept based on debugfs back in the
days).

I hope that makes things a bit clearer...?

>Also, the input
>developers already denied adding a separate chardev with its own API
>when we started discussing about the remote controller core.

Care to provide links? I think you're talking about something else...

My patches are not about reimplementing the input subsystem, it is
basically to define a replacement for the lirc dev which is IR agnostic.
input chardevs would still exist in parallel with the "rc-core" dev
instead of the "lirc" dev (like today).

>Adding a new chardev would make things very confusing, as we'll need
>to keep reporting data on both new and old chardev.

I fail to see the confusion. My patches already handled both the "old"
(i.e. lirc) dev and the new dev. And userspace will be written to use
one of the two interfaces...

>We have this already
>for LIRC, but with different interfaces, so, no big issue. Also, LIRC
>can be dynamically disabled at runtime. So, it seems that this is the
>best approach, IMO.

The whole point of having one core rc-core interface is of course
completely orthogonal to whether lirc can be disabled...


-- 
David Härdeman

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[Mauro Carvalho Chehab]

Em Thu, 02 Apr 2015 00:19:41 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On Tue, Mar 31, 2015 at 08:47:16PM -0300, Mauro Carvalho Chehab wrote:
> >Em Mon, 30 Mar 2015 23:18:19 +0200
> >David Härdeman <david@hardeman.nu> escreveu:
> >> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
> >> Second, if we expose protocol type (which we should, not doing so is
> >> throwing away valuable information) we should tackle the NEC scancode
> >> question. I've already explained my firm conviction that always
> >> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
> >> is of the opinion that NEC16/24/32 should be essentially different
> >> protocols.
> >
> >Changing NEC would break userspace, as existing tables won't work.
> >So, no matter what I think, changing it won't happen as we're not
> >allowed to break userspace.
> 
> I have no idea what breakage you're talking about. Sean's patches would
> introduce new API, so they can't break anything. 

Sure, but changing RX would break, and using 32 bits just for TX,
while keeping 16/24/32 for RX would be too messy.

> My patch series also
> introduced a new API for setting/getting keytable entries (with
> heuristics for the old ways to convert NEC scancodes on the fly) so it
> should (hopefully) not break anything.

I sent a review of your patch series a long time ago. Didn't receive
any answer to my review from you yet. Yet, let's not mix the subjects.
If you want to discuss that, please reply to the old thread and submit
your work on small chunks, after the approach is agreed.

> >(and yes, I think NEC16 is *the* NEC protocol; the other are just
> >variants made by some vendors to fill their needs)
> 
> We are talking about the protocol used to communicate what has been
> received/should be sent between userspace and the kernel. Simply passing
> the 32 bits that have been sent/received is the simplest, most
> straightfoward way to go.

Yes, it would be simpler. That doesn't mean that it is technically
correct. Yet, you could argue that passing 48 bits would be even
simpler, due to NEC/48 (or 64 bits, if one would ever propose a
nec-64 variant).

Ok, NEC/16/24/32 always send 32 bits at the same way, while other
"longer" variants would actually change the payload size, while
16/24/32 is just a change of the bytes meaning at the payload.
Yet, they're different.

> >> Third, we should still have a way to represent the protocol in the
> >> keymap as well.
> >
> >Not sure about that, but this is a different matter. 
> 
> Yes, it's a different matter. And what is there to be unsure about? Not
> having the protocol as part of the keymap means throwing away
> information...

The internal representation at kernelspace can always be changed.

If you're instead referring to some specific problem with the userspace
to kernelspace TX API, then please point to the specific patch for the
actual implementation, instead of discussing it in an abstract way.

> >> And on a much more general level...I still think we should start from
> >> scratch with a rc specific chardev with it's own API that is generic
> >> enough to cover both scancode / raw ir / future / other protocols (not
> >> that such an undertaking would preclude adding stuff to the lirc API of
> >> course).
> >
> >Starting from scratch sounds a bad idea. We don't do that on Linux,
> >except when we really screwed everything very badly.
> 
> LIRC...IR specific....rc-core....not IR specific...and the lirc IOCTL
> API is pretty badly screwed. Have you had a closer look at it?
> 
> I'm not saying we should throw away the lirc module/device, it'll have
> to stay around for a long time. But we should design a v2. If you've
> looked at my patches the idea is basically:
> 
> RC device is plugged in, /dev/rc/rc0 is created by udev
> 
> Applications wishing to muck about with RC devices do:
> 
> 	int fd;
> 	int ver;
> 	int type;
> 
> 	fd = open("/dev/rc/rc0")
> 
> 	ver = ioctl(fd, RCIOCGVERSION);
> 	if (ver != 1)
> 		warn("New RC API version");
> 
> 	type = ioctl(fd, RCIOCGVERSION);
> 	switch (type) {
> 	case RC_DRIVER_SCANCODE:
> 		debug("Scancode hardware");
> 		break;
> 	case RC_DRIVER_IR_RAW:
> 		debug("Raw IR hardware");
> 		break;
> 	default:
> 		debug("Unknown hardware type");
> 		break;
> 	}
> 
> And then they can do further operations depending on the type of the
> device. For example, for raw IR devices you can read() raw IR
> pulse/space timings or (if the hardware supports TX) write() raw IR
> timings.
> 
> Other examples of ioctls are (all four work using structs with all the
> relevant parameters):
> 
> * RCIOCSIRRX
> 	set all RX parameters in one go (and return the result since
> 	the exact values requested might not be supported)
> * RCIOCSIRTX
> 	same as RCIOCSIRRX but for TX
> * RCIOCGIRRX
> 	get all RX parameters
> * RCIOCGIRTX
> 	get all TX parameters
> 
> These ioctls only work with RC_DRIVER_IR_RAW hardware. Others can be
> defined for other kinds of hardware.
> 
> Then there's one more thing, and that's multiple keytables per rc
> device. Each keytable has one associated input device (so there's a
> 1-to-N mapping between rc devices and input devices). Userspace can
> create/destroy additional keytables and add/remove scancode<->keycode
> mappings per keytable. The idea is that you'd be able to e.g. define one
> keytable per physical remote control (the thing you hold in your hand,
> not the receiver/transmitter), and each would get its own input device.
> Those input devices can then be used by different applications (so you
> could have that old VCR remote control the PVR software while the TV
> remote controls your Kodi frontend). An idea I borrowed from Jon Smirl
> (who posted a similar proof-of-concept based on debugfs back in the
> days).
> 
> I hope that makes things a bit clearer...?

The Jon Smirl's proposal were nacked because it was re-defining a new
input subsystem. The input maintainers complained, and they're right.
We should not create an independent input core, but, instead, use the
already existing one.

That's basically the reason why LIRC support was added as a separate
interface: we didn't want to mess up with the input layer.

Besides that, I don't see any gain on adding an IR-specific input layer.
Doing that would require not only kernel work, but someone would also need
to patch X11, Wayland, MIR, VNC, xrdp ... in order to make them to support
such new input layer. Too much work, too less benefit.

If all you want is to add the protocol type, it should be easy to add a new
input event type to reflect it. Right now, we have already events for both
keycode and scancode. Adding support for protocol type should be an easy
addition there.

> >Also, the input
> >developers already denied adding a separate chardev with its own API
> >when we started discussing about the remote controller core.
> 
> Care to provide links? I think you're talking about something else...

All those discussions happened back on 2009.

This is one of the threads:
	http://www.gossamer-threads.com/lists/linux/kernel/972130#971761

There were actually 3 or 4 different threads.
> 
> My patches are not about reimplementing the input subsystem, it is
> basically to define a replacement for the lirc dev which is IR agnostic.
> input chardevs would still exist in parallel with the "rc-core" dev
> instead of the "lirc" dev (like today).

As I said before, let's discuss this on the top of my review to your
patchset, not hijacking Sean's submission.

> >Adding a new chardev would make things very confusing, as we'll need
> >to keep reporting data on both new and old chardev.
> 
> I fail to see the confusion. My patches already handled both the "old"
> (i.e. lirc) dev and the new dev. And userspace will be written to use
> one of the two interfaces...

Actually 3 interfaces, if your proposal is to keep the input evdev.
So, yeah, is it messy to have 3 different device nodes to get the reports
for the same hardware events.

LIRC interface is meant to be used for "raw" access to the device, just
like other input devices do. So, not much confusion (at least before
Sean's patch):
	- raw data: lirc devnode
	- handled data (scancode and keycode): input/evdev devnode

> >We have this already
> >for LIRC, but with different interfaces, so, no big issue. Also, LIRC
> >can be dynamically disabled at runtime. So, it seems that this is the
> >best approach, IMO.
> 
> The whole point of having one core rc-core interface is of course
> completely orthogonal to whether lirc can be disabled...

Yeah, it should always be possible to disable raw data support.

Regards,
Mauro

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[David Härdeman]

On Wed, Apr 01, 2015 at 08:10:16PM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 02 Apr 2015 00:19:41 +0200
>David Härdeman <david@hardeman.nu> escreveu:
>
>> On Tue, Mar 31, 2015 at 08:47:16PM -0300, Mauro Carvalho Chehab wrote:
>> >Em Mon, 30 Mar 2015 23:18:19 +0200
>> >David Härdeman <david@hardeman.nu> escreveu:
>> >> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
>> >> Second, if we expose protocol type (which we should, not doing so is
>> >> throwing away valuable information) we should tackle the NEC scancode
>> >> question. I've already explained my firm conviction that always
>> >> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
>> >> is of the opinion that NEC16/24/32 should be essentially different
>> >> protocols.
>> >
>> >Changing NEC would break userspace, as existing tables won't work.
>> >So, no matter what I think, changing it won't happen as we're not
>> >allowed to break userspace.
>> 
>> I have no idea what breakage you're talking about. Sean's patches would
>> introduce new API, so they can't break anything. 
>
>Sure, but changing RX would break, and using 32 bits just for TX,
>while keeping 16/24/32 for RX would be too messy.

Sorry, I still don't follow...why and how would RX break?

-- 
David Härdeman

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[David Härdeman]

On Wed, Apr 01, 2015 at 08:10:16PM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 02 Apr 2015 00:19:41 +0200
>David Härdeman <david@hardeman.nu> escreveu:
>> On Tue, Mar 31, 2015 at 08:47:16PM -0300, Mauro Carvalho Chehab wrote:
>> >Em Mon, 30 Mar 2015 23:18:19 +0200
>> >David Härdeman <david@hardeman.nu> escreveu:
>> >> On Thu, Mar 19, 2015 at 09:50:11PM +0000, Sean Young wrote:
>> >> Second, if we expose protocol type (which we should, not doing so is
>> >> throwing away valuable information) we should tackle the NEC scancode
>> >> question. I've already explained my firm conviction that always
>> >> reporting NEC as a 32 bit scancode is the only sane thing to do. Mauro
>> >> is of the opinion that NEC16/24/32 should be essentially different
>> >> protocols.
>> >
>> >Changing NEC would break userspace, as existing tables won't work.
>> >So, no matter what I think, changing it won't happen as we're not
>> >allowed to break userspace.
>> 
>> I have no idea what breakage you're talking about. Sean's patches would
>> introduce new API, so they can't break anything. 
>
>Sure, but changing RX would break, and using 32 bits just for TX,
>while keeping 16/24/32 for RX would be too messy.

Well...why would RX "break"?

I'm still not sure what you're referring to when you mention RX, the
only thing I can think of is keytables...

>> My patch series also
>> introduced a new API for setting/getting keytable entries (with
>> heuristics for the old ways to convert NEC scancodes on the fly) so it
>> should (hopefully) not break anything.
>
>I sent a review of your patch series a long time ago. Didn't receive
>any answer to my review from you yet. Yet, let's not mix the subjects.
>If you want to discuss that, please reply to the old thread and submit
>your work on small chunks, after the approach is agreed.

The old thread suggested I should start an RFC round for the new rc
device. The new API for setting/getting keytable entries with explicit
protocol information is orthogonal to that. I'll re-post those two
patches shortly so we have something tangible to discuss.

>> >(and yes, I think NEC16 is *the* NEC protocol; the other are just
>> >variants made by some vendors to fill their needs)

Oh, and I forgot to add. I've used NEC branded remote controls which
were not using NEC16 (IIRC, they used NEC24).

>> We are talking about the protocol used to communicate what has been
>> received/should be sent between userspace and the kernel. Simply passing
>> the 32 bits that have been sent/received is the simplest, most
>> straightfoward way to go.
>
>Yes, it would be simpler. That doesn't mean that it is technically
>correct. Yet, you could argue that passing 48 bits would be even
>simpler, due to NEC/48 (or 64 bits, if one would ever propose a
>nec-64 variant).

That's a strawman...a different transmission length calls for a separate
protocol id, otherwise you don't know what has been received (was it
0x0004, 0x000004, 0x00000004, etc...)

>Ok, NEC/16/24/32 always send 32 bits at the same way, while other
>"longer" variants would actually change the payload size, while
>16/24/32 is just a change of the bytes meaning at the payload.
>Yet, they're different.

In much the same way as private or public IP addresses are "different",
yet any sane API just reports the 32 bit address and lets the userspace
application worry about how it should be presented.

>> >> Third, we should still have a way to represent the protocol in the
>> >> keymap as well.
>> >
>> >Not sure about that, but this is a different matter. 
>> 
>> Yes, it's a different matter. And what is there to be unsure about? Not
>> having the protocol as part of the keymap means throwing away
>> information...
>
>The internal representation at kernelspace can always be changed.
>
>If you're instead referring to some specific problem with the userspace
>to kernelspace TX API, then please point to the specific patch for the
>actual implementation, instead of discussing it in an abstract way.

I'll post two patches. Let's discuss them.


-- 
David Härdeman

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[Sean Young]

On Thu, Apr 02, 2015 at 12:19:41AM +0200, David Härdeman wrote:
> On Tue, Mar 31, 2015 at 08:47:16PM -0300, Mauro Carvalho Chehab wrote:
> >Em Mon, 30 Mar 2015 23:18:19 +0200
> >David Härdeman <david@hardeman.nu> escreveu:
> >> And on a much more general level...I still think we should start from
> >> scratch with a rc specific chardev with it's own API that is generic
> >> enough to cover both scancode / raw ir / future / other protocols (not
> >> that such an undertaking would preclude adding stuff to the lirc API of
> >> course).
> >
> >Starting from scratch sounds a bad idea. We don't do that on Linux,
> >except when we really screwed everything very badly.
> 
> LIRC...IR specific....rc-core....not IR specific...and the lirc IOCTL
> API is pretty badly screwed. Have you had a closer look at it?

LIRC is IR specific, yes. If something else comes along we can think
about something new, but not before that.

> I'm not saying we should throw away the lirc module/device, it'll have
> to stay around for a long time. But we should design a v2.

What is wrong with lirc that requires a redesign?

> If you've looked at my patches the idea is basically:
> 
> RC device is plugged in, /dev/rc/rc0 is created by udev
> 
> Applications wishing to muck about with RC devices do:
> 
> 	int fd;
> 	int ver;
> 	int type;
> 
> 	fd = open("/dev/rc/rc0")
> 
> 	ver = ioctl(fd, RCIOCGVERSION);
> 	if (ver != 1)
> 		warn("New RC API version");
> 
> 	type = ioctl(fd, RCIOCGVERSION);

lirc uses feature bits. As we know from filesystems features are much
better than versioning. I'm sure there are other examples.

> 	switch (type) {
> 	case RC_DRIVER_SCANCODE:
> 		debug("Scancode hardware");
> 		break;
> 	case RC_DRIVER_IR_RAW:
> 		debug("Raw IR hardware");
> 		break;
> 	default:
> 		debug("Unknown hardware type");
> 		break;
> 	}

lirc_zilog can send both raw IR and scancodes (although I don't know how
to do raw IR yet). So with lirc you would do:

	unsigned features;

	ioctl(fd, LIRC_GET_FEATURES, &features);

	if (features & LIRC_CAN_SEND_MODE2) 
		// can send raw IR

	if (features & LIRC_CAN_SEND_SCANCODE) // needs my patches
		// can send scancodes


> And then they can do further operations depending on the type of the
> device. For example, for raw IR devices you can read() raw IR
> pulse/space timings or (if the hardware supports TX) write() raw IR
> timings.
> 
> Other examples of ioctls are (all four work using structs with all the
> relevant parameters):
> 
> * RCIOCSIRRX
> 	set all RX parameters in one go (and return the result since
> 	the exact values requested might not be supported)

Putting everything in one big struct isn't really future-proof, and it
doesn't tell you which parts are supported by the hardware.

> * RCIOCSIRTX
> 	same as RCIOCSIRRX but for TX

That would have to be a different struct for RX and TX.

RX is different from TX. You want to set a carrier range for RX, and 
a specific carrier for TX. You want a duty cycle for TX but not for RX,
carrier reports for RX but that makes no sense for TX.

> * RCIOCGIRRX
> 	get all RX parameters
> * RCIOCGIRTX
> 	get all TX parameters
> 
> These ioctls only work with RC_DRIVER_IR_RAW hardware. Others can be
> defined for other kinds of hardware.

The cec patches going round at the moment create their own character 
devices. rc-core is only a side note in that system; IR and CEC are
so widely different it doesn't really make sense to share character
devices.

> Then there's one more thing, and that's multiple keytables per rc
> device. Each keytable has one associated input device (so there's a
> 1-to-N mapping between rc devices and input devices). Userspace can
> create/destroy additional keytables and add/remove scancode<->keycode
> mappings per keytable. The idea is that you'd be able to e.g. define one
> keytable per physical remote control (the thing you hold in your hand,
> not the receiver/transmitter), and each would get its own input device.
> Those input devices can then be used by different applications (so you
> could have that old VCR remote control the PVR software while the TV
> remote controls your Kodi frontend). An idea I borrowed from Jon Smirl
> (who posted a similar proof-of-concept based on debugfs back in the
> days).

That would be very nice thing to have, but that is a separate from this.


Sean

Re: [RFC PATCH 0/6] Send and receive decoded IR using lirc interface

[David Härdeman]

On Fri, Apr 03, 2015 at 11:11:30AM +0100, Sean Young wrote:
>On Thu, Apr 02, 2015 at 12:19:41AM +0200, David Härdeman wrote:
>> On Tue, Mar 31, 2015 at 08:47:16PM -0300, Mauro Carvalho Chehab wrote:
>> >Em Mon, 30 Mar 2015 23:18:19 +0200
>> >David Härdeman <david@hardeman.nu> escreveu:
>> >> And on a much more general level...I still think we should start from
>> >> scratch with a rc specific chardev with it's own API that is generic
>> >> enough to cover both scancode / raw ir / future / other protocols (not
>> >> that such an undertaking would preclude adding stuff to the lirc API of
>> >> course).
>> >
>> >Starting from scratch sounds a bad idea. We don't do that on Linux,
>> >except when we really screwed everything very badly.
>> 
>> LIRC...IR specific....rc-core....not IR specific...and the lirc IOCTL
>> API is pretty badly screwed. Have you had a closer look at it?
>
>LIRC is IR specific, yes. If something else comes along we can think
>about something new, but not before that.

Yeah, I'm not saying we should throw out lirc now or not improve on
it...not at all.

>> I'm not saying we should throw away the lirc module/device, it'll have
>> to stay around for a long time. But we should design a v2.
>
>What is wrong with lirc that requires a redesign?

I'll defer that discussion to a separate thread. It's really not related
to your patches (sorry).

>> If you've looked at my patches the idea is basically:
>> 
>> RC device is plugged in, /dev/rc/rc0 is created by udev
>> 
>> Applications wishing to muck about with RC devices do:
>> 
>> 	int fd;
>> 	int ver;
>> 	int type;
>> 
>> 	fd = open("/dev/rc/rc0")
>> 
>> 	ver = ioctl(fd, RCIOCGVERSION);
>> 	if (ver != 1)
>> 		warn("New RC API version");
>> 
>> 	type = ioctl(fd, RCIOCGVERSION);
>
>lirc uses feature bits. As we know from filesystems features are much
>better than versioning. I'm sure there are other examples.

My implementation supports feature bits (flags).

>> 	switch (type) {
>> 	case RC_DRIVER_SCANCODE:
>> 		debug("Scancode hardware");
>> 		break;
>> 	case RC_DRIVER_IR_RAW:
>> 		debug("Raw IR hardware");
>> 		break;
>> 	default:
>> 		debug("Unknown hardware type");
>> 		break;
>> 	}
>
>lirc_zilog can send both raw IR and scancodes (although I don't know how
>to do raw IR yet). So with lirc you would do:
>
>	unsigned features;
>
>	ioctl(fd, LIRC_GET_FEATURES, &features);
>
>	if (features & LIRC_CAN_SEND_MODE2) 
>		// can send raw IR
>
>	if (features & LIRC_CAN_SEND_SCANCODE) // needs my patches
>		// can send scancodes

That's something to keep in mind, thanks.

>> And then they can do further operations depending on the type of the
>> device. For example, for raw IR devices you can read() raw IR
>> pulse/space timings or (if the hardware supports TX) write() raw IR
>> timings.
>> 
>> Other examples of ioctls are (all four work using structs with all the
>> relevant parameters):
>> 
>> * RCIOCSIRRX
>> 	set all RX parameters in one go (and return the result since
>> 	the exact values requested might not be supported)
>
>Putting everything in one big struct isn't really future-proof, and it
>doesn't tell you which parts are supported by the hardware.

It's reasonably "future proof" for three reasons:

1) Microsoft has a CIR API...with RX/TX structs for setting the
parameters, that will in itself tend to make sure that hardware sticks
to what's supported by that API

2) If you look at the evolution of the LIRC API, it's pretty clear that
completely new hardware capabilities aren't really being developed at a
brisk pace. The capabilities and parameters that need to be set have
remained fairly static for a long time.

3) My proposed structs include explicit support for flags as well as
reserved struct members (which help future extensibility) and new
ioctls could be added as a fallback.

>> * RCIOCSIRTX
>> 	same as RCIOCSIRRX but for TX
>
>That would have to be a different struct for RX and TX.
>
>RX is different from TX. You want to set a carrier range for RX, and 
>a specific carrier for TX. You want a duty cycle for TX but not for RX,
>carrier reports for RX but that makes no sense for TX.

It is two different structs in the implementation that I've posted

>
>> * RCIOCGIRRX
>> 	get all RX parameters
>> * RCIOCGIRTX
>> 	get all TX parameters
>> 
>> These ioctls only work with RC_DRIVER_IR_RAW hardware. Others can be
>> defined for other kinds of hardware.
>
>The cec patches going round at the moment create their own character 
>devices. rc-core is only a side note in that system; IR and CEC are
>so widely different it doesn't really make sense to share character
>devices.

Maybe so. I haven't looked at the proposed CEC API in detail. But IIRC
CEC has IR passthrough capabilities so it's not obvious that they'll be
completely independent...?

>> Then there's one more thing, and that's multiple keytables per rc
>> device. Each keytable has one associated input device (so there's a
>> 1-to-N mapping between rc devices and input devices). Userspace can
>> create/destroy additional keytables and add/remove scancode<->keycode
>> mappings per keytable. The idea is that you'd be able to e.g. define one
>> keytable per physical remote control (the thing you hold in your hand,
>> not the receiver/transmitter), and each would get its own input device.
>> Those input devices can then be used by different applications (so you
>> could have that old VCR remote control the PVR software while the TV
>> remote controls your Kodi frontend). An idea I borrowed from Jon Smirl
>> (who posted a similar proof-of-concept based on debugfs back in the
>> days).
>
>That would be very nice thing to have, but that is a separate from this.

Agreed. And I kind of hijacked your thread. Sorry about that. I'm not
opposed to hacking on/improving the lirc interface, and I'm not opposed
to your patches. The takeaway from what I've said basically boils down to:

1) Dynamic scancode length
2) Need to sort out/agree on/standardize scancode representation before
exposing it further to userspace
3) Keeping lirc as separate from rc-core as possible (e.g. not littering
rc-core with lirc specifics) still makes sense

-- 
David Härdeman