[PATCH v3 0/7] rc: Add IR encode based wakeup filtering

[PATCH v3 0/7] rc: Add IR encode based wakeup filtering

[Antti Seppälä]

Hello.

I'd like to revive efforts and discussion on adding IR encoding support to
rc-core.

Here is an updated patchset which adds support for modulating RC-5 scancodes
into raw IR events which can be written to hardware such as nuvoton-cir to
perform system wakeup with an IR pulse.

The previous patchset was posted by James Hogan roughly a year ago. I've
now updated the patchset to apply cleanly against latest linux-media tree.

For simplicity I've also dropped support for nec scancode encoding as the
nec format turned out to be ambiquous between variants. I'm hoping the nec
scancodes can be re-engineered and support for them added back later once the
initial encoding groundwork is in place. The remaining RC-5(X,SZ) support
should work in an unambiquous manner. I've also added an encoder for RC-6(A)
protocols as the encoding is very similar to RC-5.

Changes since v2:
 - Ported to apply against latest media-tree
 - Fixed new checkpatch.pl warnings
 - Dropped nec scancode support for now
 - Merged separate encoders for rc-5 and rc5-sz into single encoder
 - Added rc-6 encoder

Cc: James Hogan <james@albanarts.com>
Cc: David Härdeman <david@hardeman.nu>

Antti Seppälä (3):
  rc: rc-ir-raw: Add Manchester encoder (phase encoder) helper
  rc: ir-rc6-decoder: Add encode capability
  rc: nuvoton-cir: Add support for writing wakeup samples via sysfs
    filter callback

James Hogan (4):
  rc: rc-ir-raw: Add scancode encoder callback
  rc: ir-rc5-decoder: Add encode capability
  rc: rc-core: Add support for encode_wakeup drivers
  rc: rc-loopback: Add loopback of filter scancodes

 drivers/media/rc/ir-rc5-decoder.c | 116 +++++++++++++++++++++++++++++++
 drivers/media/rc/ir-rc6-decoder.c | 121 +++++++++++++++++++++++++++++++++
 drivers/media/rc/nuvoton-cir.c    | 127 ++++++++++++++++++++++++++++++++++
 drivers/media/rc/nuvoton-cir.h    |   1 +
 drivers/media/rc/rc-core-priv.h   |  36 ++++++++++
 drivers/media/rc/rc-ir-raw.c      | 139 ++++++++++++++++++++++++++++++++++++++
 drivers/media/rc/rc-loopback.c    |  36 ++++++++++
 drivers/media/rc/rc-main.c        |   7 +-
 include/media/rc-core.h           |   7 ++
 9 files changed, 589 insertions(+), 1 deletion(-)

-- 
2.0.5

[PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

From: James Hogan <james@albanarts.com>

Add a callback to raw ir handlers for encoding and modulating a scancode
to a set of raw events. This could be used for transmit, or for
converting a wakeup scancode filter to a form that is more suitable for
raw hardware wake up filters.

Signed-off-by: James Hogan <james@albanarts.com>
Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Cc: David Härdeman <david@hardeman.nu>
---

Notes:
    Changes in v3:
     - Ported to apply against latest media-tree
    
    Changes in v2:
     - Alter encode API to return -ENOBUFS when there isn't enough buffer
       space. When this occurs all buffer contents must have been written
       with the partial encoding of the scancode. This is to allow drivers
       such as nuvoton-cir to provide a shorter buffer and still get a
       useful partial encoding for the wakeup pattern.

 drivers/media/rc/rc-core-priv.h |  2 ++
 drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
 include/media/rc-core.h         |  3 +++
 3 files changed, 42 insertions(+)

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index b68d4f76..122c25f 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
+		      struct ir_raw_event *events, unsigned int max);
 
 	/* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
 	return 0;
 }
 
+/**
+ * ir_raw_encode_scancode() - Encode a scancode as raw events
+ *
+ * @protocols:		permitted protocols
+ * @scancode:		scancode filter describing a single scancode
+ * @events:		array of raw events to write into
+ * @max:		max number of raw events
+ *
+ * Attempts to encode the scancode as raw events.
+ *
+ * Returns:	The number of events written.
+ *		-ENOBUFS if there isn't enough space in the array to fit the
+ *		encoding. In this case all @max events will have been written.
+ *		-EINVAL if the scancode is ambiguous or invalid, or if no
+ *		compatible encoder was found.
+ */
+int ir_raw_encode_scancode(u64 protocols,
+			   const struct rc_scancode_filter *scancode,
+			   struct ir_raw_event *events, unsigned int max)
+{
+	struct ir_raw_handler *handler;
+	int ret = -EINVAL;
+
+	mutex_lock(&ir_raw_handler_lock);
+	list_for_each_entry(handler, &ir_raw_handler_list, list) {
+		if (handler->protocols & protocols && handler->encode) {
+			ret = handler->encode(protocols, scancode, events, max);
+			if (ret >= 0 || ret == -ENOBUFS)
+				break;
+		}
+	}
+	mutex_unlock(&ir_raw_handler_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(ir_raw_encode_scancode);
+
 /*
  * Used to (un)register raw event clients
  */
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 2c7fbca..5703c08 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -250,6 +250,9 @@ 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_scancode(u64 protocols,
+			   const struct rc_scancode_filter *scancode,
+			   struct ir_raw_event *events, unsigned int max);
 
 static inline void ir_raw_event_reset(struct rc_dev *dev)
 {
-- 
2.0.5

[PATCH v3 2/7] rc: rc-ir-raw: Add Manchester encoder (phase encoder) helper

[Antti Seppälä]

Adding a simple Manchester encoder to rc-core.
Manchester coding is used by at least RC-5 and RC-6 protocols and their
variants.

Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Signed-off-by: James Hogan <james@albanarts.com>
Cc: David Härdeman <david@hardeman.nu>
---

Notes:
    Changes in v3:
     - Ported to apply against latest media-tree
     - Enhanced to support rc-6 encoding
     - Checkpatch.pl fixes
    
    Changes in v2 (James Hogan):
     - Alter encode API to return -ENOBUFS when there isn't enough buffer
       space. When this occurs all buffer contents must have been written
       with the partial encoding of the scancode. This is to allow drivers
       such as nuvoton-cir to provide a shorter buffer and still get a
       useful partial encoding for the wakeup pattern.
     - Add kerneldoc comment.
     - Add individual buffer full checks, in order to support -ENOBUFS
       properly.
     - Make i unsigned to theoretically support all 32bits of data.
     - Increment *ev at end so caller can calculate correct number of
       events (during the loop *ev points to the last written event to allow
       it to be extended in length).
     - Make start/leader pulse optional, continuing from (*ev)[-1] if
       disabled. This helps support rc-5x which has a space in the middle of
       the bits.

 drivers/media/rc/rc-core-priv.h | 33 ++++++++++++++++
 drivers/media/rc/rc-ir-raw.c    | 85 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 122c25f..5266ecc7 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -152,6 +152,39 @@ static inline bool is_timing_event(struct ir_raw_event ev)
 #define TO_US(duration)			DIV_ROUND_CLOSEST((duration), 1000)
 #define TO_STR(is_pulse)		((is_pulse) ? "pulse" : "space")
 
+/* functions for IR encoders */
+
+static inline void init_ir_raw_event_duration(struct ir_raw_event *ev,
+					      unsigned int pulse,
+					      u32 duration)
+{
+	init_ir_raw_event(ev);
+	ev->duration = duration;
+	ev->pulse = pulse;
+}
+
+/**
+ * struct ir_raw_timings_manchester - Manchester coding timings
+ * @leader:		duration of leader pulse (if any) 0 if continuing
+ *			existing signal (see @pulse_space_start)
+ * @pulse_space_start:	1 for starting with pulse (0 for starting with space)
+ * @clock:		duration of each pulse/space in ns
+ * @invert:		if set clock logic is inverted
+ *			(0 = space + pulse, 1 = pulse + space)
+ * @trailer_space:	duration of trailer space in ns
+ */
+struct ir_raw_timings_manchester {
+	unsigned int leader;
+	unsigned int pulse_space_start:1;
+	unsigned int clock;
+	unsigned int invert:1;
+	unsigned int trailer_space;
+};
+
+int ir_raw_gen_manchester(struct ir_raw_event **ev, unsigned int max,
+			  const struct ir_raw_timings_manchester *timings,
+			  unsigned int n, unsigned int data);
+
 /*
  * Routines from rc-raw.c to be used internally and by decoders
  */
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index dd47fe5..6c9580e 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -247,6 +247,91 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
 }
 
 /**
+ * ir_raw_gen_manchester() - Encode data with Manchester (bi-phase) modulation.
+ * @ev:		Pointer to pointer to next free event. *@ev is incremented for
+ *		each raw event filled.
+ * @max:	Maximum number of raw events to fill.
+ * @timings:	Manchester modulation timings.
+ * @n:		Number of bits of data.
+ * @data:	Data bits to encode.
+ *
+ * Encodes the @n least significant bits of @data using Manchester (bi-phase)
+ * modulation with the timing characteristics described by @timings, writing up
+ * to @max raw IR events using the *@ev pointer.
+ *
+ * Returns:	0 on success.
+ *		-ENOBUFS if there isn't enough space in the array to fit the
+ *		full encoded data. In this case all @max events will have been
+ *		written.
+ */
+int ir_raw_gen_manchester(struct ir_raw_event **ev, unsigned int max,
+			  const struct ir_raw_timings_manchester *timings,
+			  unsigned int n, unsigned int data)
+{
+	bool need_pulse;
+	unsigned int i;
+	int ret = -ENOBUFS;
+
+	i = 1 << (n - 1);
+
+	if (timings->leader) {
+		if (!max--)
+			return ret;
+		if (timings->pulse_space_start) {
+			init_ir_raw_event_duration((*ev)++, 1, timings->leader);
+
+			if (!max--)
+				return ret;
+			init_ir_raw_event_duration((*ev), 0, timings->leader);
+		} else {
+			init_ir_raw_event_duration((*ev), 1, timings->leader);
+		}
+		i >>= 1;
+	} else {
+		/* continue existing signal */
+		--(*ev);
+	}
+	/* from here on *ev will point to the last event rather than the next */
+
+	while (n && i > 0) {
+		need_pulse = !(data & i);
+		if (timings->invert)
+			need_pulse = !need_pulse;
+		if (need_pulse == !!(*ev)->pulse) {
+			(*ev)->duration += timings->clock;
+		} else {
+			if (!max--)
+				goto nobufs;
+			init_ir_raw_event_duration(++(*ev), need_pulse,
+						   timings->clock);
+		}
+
+		if (!max--)
+			goto nobufs;
+		init_ir_raw_event_duration(++(*ev), !need_pulse,
+					   timings->clock);
+		i >>= 1;
+	}
+
+	if (timings->trailer_space) {
+		if (!(*ev)->pulse)
+			(*ev)->duration += timings->trailer_space;
+		else if (!max--)
+			goto nobufs;
+		else
+			init_ir_raw_event_duration(++(*ev), 0,
+						   timings->trailer_space);
+	}
+
+	ret = 0;
+nobufs:
+	/* point to the next event rather than last event before returning */
+	++(*ev);
+	return ret;
+}
+EXPORT_SYMBOL(ir_raw_gen_manchester);
+
+/**
  * ir_raw_encode_scancode() - Encode a scancode as raw events
  *
  * @protocols:		permitted protocols
-- 
2.0.5

[PATCH v3 3/7] rc: ir-rc5-decoder: Add encode capability

[Antti Seppälä]

From: James Hogan <james@albanarts.com>

Add the capability to encode RC-5, RC-5X and RC-5-SZ scancodes as raw
events. The protocol is chosen based on the specified protocol mask,
and whether all the required bits are set in the scancode mask, and
none of the unused bits are set in the scancode data. For example a
scancode filter with bit 16 set in both data and mask is unambiguously
RC-5X.

The Manchester modulation helper is used, and for RC-5X it is used twice
with two sets of timings, the first with a short trailer space for the
space in the middle, and the second with no leader so that it can
continue the space.

The encoding in RC-5-SZ first inserts a pulse and then simply utilizes
the generic Manchester encoder available in rc-core.

Signed-off-by: James Hogan <james@albanarts.com>
Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Cc: David Härdeman <david@hardeman.nu>
---

Notes:
    Changes in v3:
     - Ported to apply with latest media-tree
     - Merged with an encoder for rc-5-sz variant
    
    I've mostly reverse engineered RC-5X from the decoder, but it seems to
    work in loopback. Here's some debug output:
    
    RC-5X:
    evbug: Event. Dev: input0, Type: 4, Code: 4, Value: 65793
    evbug: Event. Dev: input0, Type: 0, Code: 0, Value: 0
     _   __   _   _   _   _    _      _   _   _   _   _    __   _   _   _   _    _
    | | |  | | | | | | | | |  | |    | | | | | | | | | |  |  | | | | | | | | |  | |
              |
    | |_|  |_| |_| |_| |_| |__| |____| |_| |_| |_| |_| |__|  |_| |_| |_| |_| |__| |__________|
          1                  1      3                    1  1                  1            8
     8 8  7 8 8 8 8 8 8 8 8  7 8    5 8 8 8 8 8 8 8 8 8  7  7 8 8 8 8 8 8 8 8  7 8          8
     8 8  7 8 8 8 8 8 8 8 8  7 8    5 8 8 8 8 8 8 8 8 8  7  7 8 8 8 8 8 8 8 8  7 8          8
     9 9  8 9 9 9 9 9 9 9 9  8 9    6 9 9 9 9 9 9 9 9 9  8  8 9 9 9 9 9 9 9 9  8 9          9
       1  0   0   0   0   0  1      X 0   0   0   0   0  1  0   0   0   0   0  1            E rc-5
    
    RC-5:
    evbug: Event. Dev: input0, Type: 4, Code: 4, Value: 257
    evbug: Event. Dev: input0, Type: 0, Code: 0, Value: 0
     _   __   _   _   _   _    __   _   _   _   _    _
    | | |  | | | | | | | | |  |  | | | | | | | | |  | |          |
    | |_|  |_| |_| |_| |_| |__|  |_| |_| |_| |_| |__| |__________|
          1                  1  1                  1            8
     8 8  7 8 8 8 8 8 8 8 8  7  7 8 8 8 8 8 8 8 8  7 8          8
     8 8  7 8 8 8 8 8 8 8 8  7  7 8 8 8 8 8 8 8 8  7 8          8
     9 9  8 9 9 9 9 9 9 9 9  8  8 9 9 9 9 9 9 9 9  8 9          9
       1  0   0   0   0   0  1  0   0   0   0   0  1            E rc-5

 drivers/media/rc/ir-rc5-decoder.c | 116 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/media/rc/ir-rc5-decoder.c b/drivers/media/rc/ir-rc5-decoder.c
index 84fa6e9..8939ebd 100644
--- a/drivers/media/rc/ir-rc5-decoder.c
+++ b/drivers/media/rc/ir-rc5-decoder.c
@@ -184,9 +184,125 @@ out:
 	return -EINVAL;
 }
 
+static struct ir_raw_timings_manchester ir_rc5_timings = {
+	.leader			= RC5_UNIT,
+	.pulse_space_start	= 0,
+	.clock			= RC5_UNIT,
+	.trailer_space		= RC5_UNIT * 10,
+};
+
+static struct ir_raw_timings_manchester ir_rc5x_timings[2] = {
+	{
+		.leader			= RC5_UNIT,
+		.pulse_space_start	= 0,
+		.clock			= RC5_UNIT,
+		.trailer_space		= RC5X_SPACE,
+	},
+	{
+		.clock			= RC5_UNIT,
+		.trailer_space		= RC5_UNIT * 10,
+	},
+};
+
+static struct ir_raw_timings_manchester ir_rc5_sz_timings = {
+	.leader				= RC5_UNIT,
+	.pulse_space_start		= 0,
+	.clock				= RC5_UNIT,
+	.trailer_space			= RC5_UNIT * 10,
+};
+
+static int ir_rc5_validate_filter(const struct rc_scancode_filter *scancode,
+				  unsigned int important_bits)
+{
+	/* all important bits of scancode should be set in mask */
+	if (~scancode->mask & important_bits)
+		return -EINVAL;
+	/* extra bits in mask should be zero in data */
+	if (scancode->mask & scancode->data & ~important_bits)
+		return -EINVAL;
+	return 0;
+}
+
+/**
+ * ir_rc5_encode() - Encode a scancode as a stream of raw events
+ *
+ * @protocols:	allowed protocols
+ * @scancode:	scancode filter describing scancode (helps distinguish between
+ *		protocol subtypes when scancode is ambiguous)
+ * @events:	array of raw ir events to write into
+ * @max:	maximum size of @events
+ *
+ * Returns:	The number of events written.
+ *		-ENOBUFS if there isn't enough space in the array to fit the
+ *		encoding. In this case all @max events will have been written.
+ *		-EINVAL if the scancode is ambiguous or invalid.
+ */
+static int ir_rc5_encode(u64 protocols,
+			 const struct rc_scancode_filter *scancode,
+			 struct ir_raw_event *events, unsigned int max)
+{
+	int ret;
+	struct ir_raw_event *e = events;
+	unsigned int data, xdata, command, commandx, system;
+
+	/* Detect protocol and convert scancode to raw data */
+	if (protocols & RC_BIT_RC5 &&
+	    !ir_rc5_validate_filter(scancode, 0x1f7f)) {
+		/* decode scancode */
+		command  = (scancode->data & 0x003f) >> 0;
+		commandx = (scancode->data & 0x0040) >> 6;
+		system   = (scancode->data & 0x1f00) >> 8;
+		/* encode data */
+		data = !commandx << 12 | system << 6 | command;
+
+		/* Modulate the data */
+		ret = ir_raw_gen_manchester(&e, max, &ir_rc5_timings, RC5_NBITS,
+					    data);
+		if (ret < 0)
+			return ret;
+	} else if (protocols & RC_BIT_RC5X &&
+		   !ir_rc5_validate_filter(scancode, 0x1f7f3f)) {
+		/* decode scancode */
+		xdata    = (scancode->data & 0x00003f) >> 0;
+		command  = (scancode->data & 0x003f00) >> 8;
+		commandx = (scancode->data & 0x004000) >> 14;
+		system   = (scancode->data & 0x1f0000) >> 16;
+		/* commandx and system overlap, bits must match when encoded */
+		if (commandx == (system & 0x1))
+			return -EINVAL;
+		/* encode data */
+		data = 1 << 18 | system << 12 | command << 6 | xdata;
+
+		/* Modulate the data */
+		ret = ir_raw_gen_manchester(&e, max, &ir_rc5x_timings[0],
+					CHECK_RC5X_NBITS,
+					data >> (RC5X_NBITS-CHECK_RC5X_NBITS));
+		if (ret < 0)
+			return ret;
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					&ir_rc5x_timings[1],
+					RC5X_NBITS - CHECK_RC5X_NBITS,
+					data);
+		if (ret < 0)
+			return ret;
+	} else if (protocols & RC_BIT_RC5_SZ &&
+		   !ir_rc5_validate_filter(scancode, 0x2fff)) {
+		/* RC5-SZ scancode is raw enough for Manchester as it is */
+		ret = ir_raw_gen_manchester(&e, max, &ir_rc5_sz_timings,
+					RC5_SZ_NBITS, scancode->data & 0x2fff);
+		if (ret < 0)
+			return ret;
+	} else {
+		return -EINVAL;
+	}
+
+	return e - events;
+}
+
 static struct ir_raw_handler rc5_handler = {
 	.protocols	= RC_BIT_RC5 | RC_BIT_RC5X | RC_BIT_RC5_SZ,
 	.decode		= ir_rc5_decode,
+	.encode		= ir_rc5_encode,
 };
 
 static int __init ir_rc5_decode_init(void)
-- 
2.0.5

[PATCH v3 4/7] rc: ir-rc6-decoder: Add encode capability

[Antti Seppälä]

Add the capability to encode RC-6 and RC-6A scancodes as raw events.
The protocol is chosen based on the specified protocol mask, and
whether all the required bits are set in the scancode mask, and none of
the unused bits are set in the scancode data.

The Manchester modulation helper is used several times with various
timings so that RC-6 header preamble, the header, header trailing bit
and the data itself can be modulated correctly.

Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Cc: James Hogan <james@albanarts.com>
Cc: David Härdeman <david@hardeman.nu>
---

Notes:
    Changes in v3:
     - New patch

 drivers/media/rc/ir-rc6-decoder.c | 122 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/media/rc/ir-rc6-decoder.c b/drivers/media/rc/ir-rc6-decoder.c
index d16bc67..f9c70ba 100644
--- a/drivers/media/rc/ir-rc6-decoder.c
+++ b/drivers/media/rc/ir-rc6-decoder.c
@@ -291,11 +291,133 @@ out:
 	return -EINVAL;
 }
 
+static struct ir_raw_timings_manchester ir_rc6_timings[4] = {
+	{
+		.leader			= RC6_PREFIX_PULSE,
+		.pulse_space_start	= 0,
+		.clock			= RC6_UNIT,
+		.invert			= 1,
+		.trailer_space		= RC6_PREFIX_SPACE,
+	},
+	{
+		.clock			= RC6_UNIT,
+		.invert			= 1,
+	},
+	{
+		.clock			= RC6_UNIT * 2,
+		.invert			= 1,
+	},
+	{
+		.clock			= RC6_UNIT,
+		.invert			= 1,
+		.trailer_space		= RC6_SUFFIX_SPACE,
+	},
+};
+
+static int ir_rc6_validate_filter(const struct rc_scancode_filter *scancode,
+				  unsigned int important_bits)
+{
+	/* all important bits of scancode should be set in mask */
+	if (~scancode->mask & important_bits)
+		return -EINVAL;
+	/* extra bits in mask should be zero in data */
+	if (scancode->mask & scancode->data & ~important_bits)
+		return -EINVAL;
+	return 0;
+}
+
+/**
+ * ir_rc6_encode() - Encode a scancode as a stream of raw events
+ *
+ * @protocols:	allowed protocols
+ * @scancode:	scancode filter describing scancode (helps distinguish between
+ *		protocol subtypes when scancode is ambiguous)
+ * @events:	array of raw ir events to write into
+ * @max:	maximum size of @events
+ *
+ * Returns:	The number of events written.
+ *		-ENOBUFS if there isn't enough space in the array to fit the
+ *		encoding. In this case all @max events will have been written.
+ *		-EINVAL if the scancode is ambiguous or invalid.
+ */
+static int ir_rc6_encode(u64 protocols,
+			 const struct rc_scancode_filter *scancode,
+			 struct ir_raw_event *events, unsigned int max)
+{
+	int ret;
+	struct ir_raw_event *e = events;
+
+	if (protocols & RC_BIT_RC6_0 &&
+	    !ir_rc6_validate_filter(scancode, 0xffff)) {
+
+		/* Modulate the preamble */
+		ret = ir_raw_gen_manchester(&e, max, &ir_rc6_timings[0], 0, 0);
+		if (ret < 0)
+			return ret;
+
+		/* Modulate the header (Start Bit & Mode-0) */
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					    &ir_rc6_timings[1],
+					    RC6_HEADER_NBITS, (1 << 3));
+		if (ret < 0)
+			return ret;
+
+		/* Modulate Trailer Bit */
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					    &ir_rc6_timings[2], 1, 0);
+		if (ret < 0)
+			return ret;
+
+		/* Modulate rest of the data */
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					    &ir_rc6_timings[3], RC6_0_NBITS,
+					    scancode->data);
+		if (ret < 0)
+			return ret;
+
+	} else if (protocols & (RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
+				RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE) &&
+		   !ir_rc6_validate_filter(scancode, 0x8fffffff)) {
+
+		/* Modulate the preamble */
+		ret = ir_raw_gen_manchester(&e, max, &ir_rc6_timings[0], 0, 0);
+		if (ret < 0)
+			return ret;
+
+		/* Modulate the header (Start Bit & Header-version 6 */
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					    &ir_rc6_timings[1],
+					    RC6_HEADER_NBITS, (1 << 3 | 6));
+		if (ret < 0)
+			return ret;
+
+		/* Modulate Trailer Bit */
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					    &ir_rc6_timings[2], 1, 0);
+		if (ret < 0)
+			return ret;
+
+		/* Modulate rest of the data */
+		ret = ir_raw_gen_manchester(&e, max - (e - events),
+					    &ir_rc6_timings[3],
+					    fls(scancode->mask),
+					    scancode->data);
+		if (ret < 0)
+			return ret;
+
+	} else {
+		return -EINVAL;
+	}
+
+	return e - events;
+}
+
 static struct ir_raw_handler rc6_handler = {
 	.protocols	= RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 |
 			  RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32 |
 			  RC_BIT_RC6_MCE,
 	.decode		= ir_rc6_decode,
+	.encode		= ir_rc6_encode,
 };
 
 static int __init ir_rc6_decode_init(void)
-- 
2.0.5

[PATCH v3 5/7] rc: rc-core: Add support for encode_wakeup drivers

[Antti Seppälä]

From: James Hogan <james@albanarts.com>

Add support in rc-core for drivers which implement the wakeup scancode
filter by encoding the scancode using the raw IR encoders. This is by
way of rc_dev::encode_wakeup which should be set to true to make the
allowed wakeup protocols the same as the set of raw IR encoders.

As well as updating the sysfs interface to know which wakeup protocols
are allowed for encode_wakeup drivers, also ensure that the IR
decoders/encoders are loaded when an encode_wakeup driver is registered.

Signed-off-by: James Hogan <james@albanarts.com>
Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Cc: David Härdeman <david@hardeman.nu>
---

Notes:
    Changes in v3:
     - Ported to apply against latest media-tree
    
    Changes in v2:
     - New patch

 drivers/media/rc/rc-core-priv.h |  1 +
 drivers/media/rc/rc-ir-raw.c    | 17 +++++++++++++++++
 drivers/media/rc/rc-main.c      |  7 ++++++-
 include/media/rc-core.h         |  3 +++
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 5266ecc7..4b994aa 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -189,6 +189,7 @@ int ir_raw_gen_manchester(struct ir_raw_event **ev, unsigned int max,
  * Routines from rc-raw.c to be used internally and by decoders
  */
 u64 ir_raw_get_allowed_protocols(void);
+u64 ir_raw_get_encode_protocols(void);
 int ir_raw_event_register(struct rc_dev *dev);
 void ir_raw_event_unregister(struct rc_dev *dev);
 int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 6c9580e..b9e4645 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -30,6 +30,7 @@ static LIST_HEAD(ir_raw_client_list);
 static DEFINE_MUTEX(ir_raw_handler_lock);
 static LIST_HEAD(ir_raw_handler_list);
 static u64 available_protocols;
+static u64 encode_protocols;
 
 static int ir_raw_event_thread(void *data)
 {
@@ -240,6 +241,18 @@ ir_raw_get_allowed_protocols(void)
 	return protocols;
 }
 
+/* used internally by the sysfs interface */
+u64
+ir_raw_get_encode_protocols(void)
+{
+	u64 protocols;
+
+	mutex_lock(&ir_raw_handler_lock);
+	protocols = encode_protocols;
+	mutex_unlock(&ir_raw_handler_lock);
+	return protocols;
+}
+
 static int change_protocol(struct rc_dev *dev, u64 *rc_type)
 {
 	/* the caller will update dev->enabled_protocols */
@@ -450,6 +463,8 @@ int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler)
 		list_for_each_entry(raw, &ir_raw_client_list, list)
 			ir_raw_handler->raw_register(raw->dev);
 	available_protocols |= ir_raw_handler->protocols;
+	if (ir_raw_handler->encode)
+		encode_protocols |= ir_raw_handler->protocols;
 	mutex_unlock(&ir_raw_handler_lock);
 
 	return 0;
@@ -466,6 +481,8 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
 		list_for_each_entry(raw, &ir_raw_client_list, list)
 			ir_raw_handler->raw_unregister(raw->dev);
 	available_protocols &= ~ir_raw_handler->protocols;
+	if (ir_raw_handler->encode)
+		encode_protocols &= ~ir_raw_handler->protocols;
 	mutex_unlock(&ir_raw_handler_lock);
 }
 EXPORT_SYMBOL(ir_raw_handler_unregister);
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index f8c5e47..cb42655 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -865,6 +865,8 @@ static ssize_t show_protocols(struct device *device,
 	} else {
 		enabled = dev->enabled_wakeup_protocols;
 		allowed = dev->allowed_wakeup_protocols;
+		if (dev->encode_wakeup && !allowed)
+			allowed = ir_raw_get_encode_protocols();
 	}
 
 	mutex_unlock(&dev->lock);
@@ -1406,13 +1408,16 @@ int rc_register_device(struct rc_dev *dev)
 		path ? path : "N/A");
 	kfree(path);
 
-	if (dev->driver_type == RC_DRIVER_IR_RAW) {
+	if (dev->driver_type == RC_DRIVER_IR_RAW || dev->encode_wakeup) {
 		/* Load raw decoders, if they aren't already */
 		if (!raw_init) {
 			IR_dprintk(1, "Loading raw decoders\n");
 			ir_raw_init();
 			raw_init = true;
 		}
+	}
+
+	if (dev->driver_type == RC_DRIVER_IR_RAW) {
 		/* calls ir_register_device so unlock mutex here*/
 		mutex_unlock(&dev->lock);
 		rc = ir_raw_event_register(dev);
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 5703c08..9ae433c 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -74,6 +74,8 @@ enum rc_filter_type {
  * @input_dev: the input child device used to communicate events to userspace
  * @driver_type: specifies if protocol decoding is done in hardware or software
  * @idle: used to keep track of RX state
+ * @encode_wakeup: wakeup filtering uses IR encode API, therefore the allowed
+ *	wakeup protocols is the set of all raw encoders
  * @allowed_protocols: bitmask with the supported RC_BIT_* protocols
  * @enabled_protocols: bitmask with the enabled RC_BIT_* protocols
  * @allowed_wakeup_protocols: bitmask with the supported RC_BIT_* wakeup protocols
@@ -134,6 +136,7 @@ struct rc_dev {
 	struct input_dev		*input_dev;
 	enum rc_driver_type		driver_type;
 	bool				idle;
+	bool				encode_wakeup;
 	u64				allowed_protocols;
 	u64				enabled_protocols;
 	u64				allowed_wakeup_protocols;
-- 
2.0.5

[PATCH v3 6/7] rc: rc-loopback: Add loopback of filter scancodes

[Antti Seppälä]

From: James Hogan <james@albanarts.com>

Add the s_wakeup_filter callback to the rc-loopback driver, which instead of
setting the filter just feeds the scancode back through the input device
so that it can be verified.

Signed-off-by: James Hogan <james@albanarts.com>
Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Cc: David Härdeman <david@hardeman.nu>
---

Notes:
    Changes in v3:
     - Ported to apply against latest media-tree
     - Checkpatch.pl fixes
    
    Changes in v2:
     - Move img-ir-raw test code to rc-loopback.
     - Handle new encode API, specifically -ENOBUFS when the buffer isn't
       long enough.
     - Set encode_wakeup so that the set of allowed wakeup protocols matches
       the set of raw IR encoders.

 drivers/media/rc/rc-loopback.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/media/rc/rc-loopback.c b/drivers/media/rc/rc-loopback.c
index 63dace8..d8bdf63 100644
--- a/drivers/media/rc/rc-loopback.c
+++ b/drivers/media/rc/rc-loopback.c
@@ -26,6 +26,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 #include <media/rc-core.h>
 
 #define DRIVER_NAME	"rc-loopback"
@@ -176,6 +177,39 @@ static int loop_set_carrier_report(struct rc_dev *dev, int enable)
 	return 0;
 }
 
+static int loop_set_wakeup_filter(struct rc_dev *dev,
+				  struct rc_scancode_filter *sc_filter)
+{
+	static const unsigned int max = 512;
+	struct ir_raw_event *raw;
+	int ret;
+	int i;
+
+	/* fine to disable filter */
+	if (!sc_filter->mask)
+		return 0;
+
+	/* encode the specified filter and loop it back */
+	raw = kmalloc_array(max, sizeof(*raw), GFP_KERNEL);
+	ret = ir_raw_encode_scancode(dev->enabled_wakeup_protocols, sc_filter,
+				     raw, max);
+	/* still loop back the partial raw IR even if it's incomplete */
+	if (ret == -ENOBUFS)
+		ret = max;
+	if (ret >= 0) {
+		/* do the loopback */
+		for (i = 0; i < ret; ++i)
+			ir_raw_event_store(dev, &raw[i]);
+		ir_raw_event_handle(dev);
+
+		ret = 0;
+	}
+
+	kfree(raw);
+
+	return ret;
+}
+
 static int __init loop_init(void)
 {
 	struct rc_dev *rc;
@@ -195,6 +229,7 @@ static int __init loop_init(void)
 	rc->map_name		= RC_MAP_EMPTY;
 	rc->priv		= &loopdev;
 	rc->driver_type		= RC_DRIVER_IR_RAW;
+	rc->encode_wakeup	= true;
 	rc->allowed_protocols	= RC_BIT_ALL;
 	rc->timeout		= 100 * 1000 * 1000; /* 100 ms */
 	rc->min_timeout		= 1;
@@ -209,6 +244,7 @@ static int __init loop_init(void)
 	rc->s_idle		= loop_set_idle;
 	rc->s_learning_mode	= loop_set_learning_mode;
 	rc->s_carrier_report	= loop_set_carrier_report;
+	rc->s_wakeup_filter	= loop_set_wakeup_filter;
 
 	loopdev.txmask		= RXMASK_REGULAR;
 	loopdev.txcarrier	= 36000;
-- 
2.0.5

[PATCH v3 7/7] rc: nuvoton-cir: Add support for writing wakeup samples via sysfs filter callback

[Antti Seppälä]

Nuvoton-cir utilizes the encoding capabilities of rc-core to convert
scancodes from user space to pulse/space format understood by the
underlying hardware.

Converted samples are then written to the wakeup fifo along with other
necessary configuration to enable wake up functionality.

Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Signed-off-by: James Hogan <james@albanarts.com>
Cc: Jarod Wilson <jarod@redhat.com>
---

Notes:
    Changes in v3:
     - Ported to apply against latest media-tree
     - Checkpatch.pl fixes
    
    Changes in v2 (James Hogan):
     - Handle new -ENOBUFS when IR encoding buffer isn't long enough and
       reduce buffer size to the size of the wake fifo so that time isn't
       wasted processing encoded IR events that will be discarded. Also only
       discard the last event if the encoded data is complete.
     - Change reference to rc_dev::enabled_protocols to
       enabled_protocols[type] since it has been converted to an array.
     - Fix IR encoding buffer loop condition to be i < ret rather than i <=
       ret. The return value of ir_raw_encode_scancode is the number of
       events rather than the last event.
     - Set encode_wakeup so that the set of allowed wakeup protocols matches
       the set of raw IR encoders.

 drivers/media/rc/nuvoton-cir.c | 127 +++++++++++++++++++++++++++++++++++++++++
 drivers/media/rc/nuvoton-cir.h |   1 +
 include/media/rc-core.h        |   1 +
 3 files changed, 129 insertions(+)

diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 9c2c863..6391d70 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -526,6 +526,130 @@ static int nvt_set_tx_carrier(struct rc_dev *dev, u32 carrier)
 	return 0;
 }
 
+static int nvt_write_wakeup_codes(struct rc_dev *dev,
+				  const u8 *wakeup_sample_buf, int count)
+{
+	int i = 0;
+	u8 reg, reg_learn_mode;
+	unsigned long flags;
+	struct nvt_dev *nvt = dev->priv;
+
+	nvt_dbg_wake("writing wakeup samples");
+
+	reg = nvt_cir_wake_reg_read(nvt, CIR_WAKE_IRCON);
+	reg_learn_mode = reg & ~CIR_WAKE_IRCON_MODE0;
+	reg_learn_mode |= CIR_WAKE_IRCON_MODE1;
+
+	/* Lock the learn area to prevent racing with wake-isr */
+	spin_lock_irqsave(&nvt->nvt_lock, flags);
+
+	/* Enable fifo writes */
+	nvt_cir_wake_reg_write(nvt, reg_learn_mode, CIR_WAKE_IRCON);
+
+	/* Clear cir wake rx fifo */
+	nvt_clear_cir_wake_fifo(nvt);
+
+	if (count > WAKE_FIFO_LEN) {
+		nvt_dbg_wake("HW FIFO too small for all wake samples");
+		count = WAKE_FIFO_LEN;
+	}
+
+	if (count)
+		pr_info("Wake samples (%d) =", count);
+	else
+		pr_info("Wake sample fifo cleared");
+
+	/* Write wake samples to fifo */
+	for (i = 0; i < count; i++) {
+		pr_cont(" %02x", wakeup_sample_buf[i]);
+		nvt_cir_wake_reg_write(nvt, wakeup_sample_buf[i],
+				       CIR_WAKE_WR_FIFO_DATA);
+	}
+	pr_cont("\n");
+
+	/* Switch cir to wakeup mode and disable fifo writing */
+	nvt_cir_wake_reg_write(nvt, reg, CIR_WAKE_IRCON);
+
+	/* Set number of bytes needed for wake */
+	nvt_cir_wake_reg_write(nvt, count ? count :
+			       CIR_WAKE_FIFO_CMP_BYTES,
+			       CIR_WAKE_FIFO_CMP_DEEP);
+
+	spin_unlock_irqrestore(&nvt->nvt_lock, flags);
+
+	return 0;
+}
+
+static int nvt_ir_raw_set_wakeup_filter(struct rc_dev *dev,
+					struct rc_scancode_filter *sc_filter)
+{
+	u8 *reg_buf;
+	u8 buf_val;
+	int i, ret, count;
+	unsigned int val;
+	struct ir_raw_event *raw;
+	bool complete;
+
+	/* Require both mask and data to be set before actually committing */
+	if (!sc_filter->mask || !sc_filter->data)
+		return 0;
+
+	raw = kmalloc_array(WAKE_FIFO_LEN, sizeof(*raw), GFP_KERNEL);
+	if (!raw)
+		return -ENOMEM;
+
+	ret = ir_raw_encode_scancode(dev->enabled_wakeup_protocols, sc_filter,
+				     raw, WAKE_FIFO_LEN);
+	complete = (ret != -ENOBUFS);
+	if (!complete)
+		ret = WAKE_FIFO_LEN;
+	else if (ret < 0)
+		goto out_raw;
+
+	reg_buf = kmalloc_array(WAKE_FIFO_LEN, sizeof(*reg_buf), GFP_KERNEL);
+	if (!reg_buf) {
+		ret = -ENOMEM;
+		goto out_raw;
+	}
+
+	/* Inspect the ir samples */
+	for (i = 0, count = 0; i < ret && count < WAKE_FIFO_LEN; ++i) {
+		val = NS_TO_US((raw[i]).duration) / SAMPLE_PERIOD;
+
+		/* Split too large values into several smaller ones */
+		while (val > 0 && count < WAKE_FIFO_LEN) {
+
+			/* Skip last value for better comparison tolerance */
+			if (complete && i == ret - 1 && val < BUF_LEN_MASK)
+				break;
+
+			/* Clamp values to BUF_LEN_MASK at most */
+			buf_val = (val > BUF_LEN_MASK) ? BUF_LEN_MASK : val;
+
+			reg_buf[count] = buf_val;
+			val -= buf_val;
+			if ((raw[i]).pulse)
+				reg_buf[count] |= BUF_PULSE_BIT;
+			count++;
+		}
+	}
+
+	ret = nvt_write_wakeup_codes(dev, reg_buf, count);
+
+	kfree(reg_buf);
+out_raw:
+	kfree(raw);
+
+	return ret;
+}
+
+/* Dummy implementation. nuvoton is agnostic to the protocol used */
+static int nvt_ir_raw_change_wakeup_protocol(struct rc_dev *dev,
+					     u64 *rc_type)
+{
+	return 0;
+}
+
 /*
  * nvt_tx_ir
  *
@@ -1043,11 +1167,14 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* Set up the rc device */
 	rdev->priv = nvt;
 	rdev->driver_type = RC_DRIVER_IR_RAW;
+	rdev->encode_wakeup = true;
 	rdev->allowed_protocols = RC_BIT_ALL;
 	rdev->open = nvt_open;
 	rdev->close = nvt_close;
 	rdev->tx_ir = nvt_tx_ir;
 	rdev->s_tx_carrier = nvt_set_tx_carrier;
+	rdev->s_wakeup_filter = nvt_ir_raw_set_wakeup_filter;
+	rdev->change_wakeup_protocol = nvt_ir_raw_change_wakeup_protocol;
 	rdev->input_name = "Nuvoton w836x7hg Infrared Remote Transceiver";
 	rdev->input_phys = "nuvoton/cir0";
 	rdev->input_id.bustype = BUS_HOST;
diff --git a/drivers/media/rc/nuvoton-cir.h b/drivers/media/rc/nuvoton-cir.h
index e1cf23c..9d0e161 100644
--- a/drivers/media/rc/nuvoton-cir.h
+++ b/drivers/media/rc/nuvoton-cir.h
@@ -63,6 +63,7 @@ static int debug;
  */
 #define TX_BUF_LEN 256
 #define RX_BUF_LEN 32
+#define WAKE_FIFO_LEN 67
 
 struct nvt_dev {
 	struct pnp_dev *pdev;
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 9ae433c..f1cb9da 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -246,6 +246,7 @@ static inline void init_ir_raw_event(struct ir_raw_event *ev)
 #define US_TO_NS(usec)		((usec) * 1000)
 #define MS_TO_US(msec)		((msec) * 1000)
 #define MS_TO_NS(msec)		((msec) * 1000 * 1000)
+#define NS_TO_US(nsec)		DIV_ROUND_UP(nsec, 1000L)
 
 void ir_raw_event_handle(struct rc_dev *dev);
 int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev);
-- 
2.0.5

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>From: James Hogan <james@albanarts.com>
>
>Add a callback to raw ir handlers for encoding and modulating a scancode
>to a set of raw events. This could be used for transmit, or for
>converting a wakeup scancode filter to a form that is more suitable for
>raw hardware wake up filters.
>
>Signed-off-by: James Hogan <james@albanarts.com>
>Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>Cc: David Härdeman <david@hardeman.nu>
>---
>
>Notes:
>    Changes in v3:
>     - Ported to apply against latest media-tree
>    
>    Changes in v2:
>     - Alter encode API to return -ENOBUFS when there isn't enough buffer
>       space. When this occurs all buffer contents must have been written
>       with the partial encoding of the scancode. This is to allow drivers
>       such as nuvoton-cir to provide a shorter buffer and still get a
>       useful partial encoding for the wakeup pattern.
>
> drivers/media/rc/rc-core-priv.h |  2 ++
> drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
> include/media/rc-core.h         |  3 +++
> 3 files changed, 42 insertions(+)
>
>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
>index b68d4f76..122c25f 100644
>--- a/drivers/media/rc/rc-core-priv.h
>+++ b/drivers/media/rc/rc-core-priv.h
>@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
>+		      struct ir_raw_event *events, unsigned int max);
> 
> 	/* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>--- a/drivers/media/rc/rc-ir-raw.c
>+++ b/drivers/media/rc/rc-ir-raw.c
>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
> 	return 0;
> }
> 
>+/**
>+ * ir_raw_encode_scancode() - Encode a scancode as raw events
>+ *
>+ * @protocols:		permitted protocols
>+ * @scancode:		scancode filter describing a single scancode
>+ * @events:		array of raw events to write into
>+ * @max:		max number of raw events
>+ *
>+ * Attempts to encode the scancode as raw events.
>+ *
>+ * Returns:	The number of events written.
>+ *		-ENOBUFS if there isn't enough space in the array to fit the
>+ *		encoding. In this case all @max events will have been written.
>+ *		-EINVAL if the scancode is ambiguous or invalid, or if no
>+ *		compatible encoder was found.
>+ */
>+int ir_raw_encode_scancode(u64 protocols,

Why a bitmask of protocols and not a single protocol enum? What's the
use case for encoding a given scancode according to one out of a number
of protocols (and not even knowing which one)??

>+			   const struct rc_scancode_filter *scancode,
>+			   struct ir_raw_event *events, unsigned int max)
>+{
>+	struct ir_raw_handler *handler;
>+	int ret = -EINVAL;
>+
>+	mutex_lock(&ir_raw_handler_lock);
>+	list_for_each_entry(handler, &ir_raw_handler_list, list) {
>+		if (handler->protocols & protocols && handler->encode) {
>+			ret = handler->encode(protocols, scancode, events, max);
>+			if (ret >= 0 || ret == -ENOBUFS)
>+				break;
>+		}
>+	}
>+	mutex_unlock(&ir_raw_handler_lock);
>+
>+	return ret;
>+}
>+EXPORT_SYMBOL(ir_raw_encode_scancode);
>+
> /*
>  * Used to (un)register raw event clients
>  */
>diff --git a/include/media/rc-core.h b/include/media/rc-core.h
>index 2c7fbca..5703c08 100644
>--- a/include/media/rc-core.h
>+++ b/include/media/rc-core.h
>@@ -250,6 +250,9 @@ 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_scancode(u64 protocols,
>+			   const struct rc_scancode_filter *scancode,
>+			   struct ir_raw_event *events, unsigned int max);
> 
> static inline void ir_raw_event_reset(struct rc_dev *dev)
> {
>-- 
>2.0.5
>

-- 
David Härdeman

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>From: James Hogan <james@albanarts.com>
>>
>>Add a callback to raw ir handlers for encoding and modulating a scancode
>>to a set of raw events. This could be used for transmit, or for
>>converting a wakeup scancode filter to a form that is more suitable for
>>raw hardware wake up filters.
>>
>>Signed-off-by: James Hogan <james@albanarts.com>
>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>Cc: David Härdeman <david@hardeman.nu>
>>---
>>
>>Notes:
>>    Changes in v3:
>>     - Ported to apply against latest media-tree
>>
>>    Changes in v2:
>>     - Alter encode API to return -ENOBUFS when there isn't enough buffer
>>       space. When this occurs all buffer contents must have been written
>>       with the partial encoding of the scancode. This is to allow drivers
>>       such as nuvoton-cir to provide a shorter buffer and still get a
>>       useful partial encoding for the wakeup pattern.
>>
>> drivers/media/rc/rc-core-priv.h |  2 ++
>> drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
>> include/media/rc-core.h         |  3 +++
>> 3 files changed, 42 insertions(+)
>>
>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
>>index b68d4f76..122c25f 100644
>>--- a/drivers/media/rc/rc-core-priv.h
>>+++ b/drivers/media/rc/rc-core-priv.h
>>@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
>>+                    struct ir_raw_event *events, unsigned int max);
>>
>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>--- a/drivers/media/rc/rc-ir-raw.c
>>+++ b/drivers/media/rc/rc-ir-raw.c
>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
>>       return 0;
>> }
>>
>>+/**
>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events
>>+ *
>>+ * @protocols:                permitted protocols
>>+ * @scancode:         scancode filter describing a single scancode
>>+ * @events:           array of raw events to write into
>>+ * @max:              max number of raw events
>>+ *
>>+ * Attempts to encode the scancode as raw events.
>>+ *
>>+ * Returns:   The number of events written.
>>+ *            -ENOBUFS if there isn't enough space in the array to fit the
>>+ *            encoding. In this case all @max events will have been written.
>>+ *            -EINVAL if the scancode is ambiguous or invalid, or if no
>>+ *            compatible encoder was found.
>>+ */
>>+int ir_raw_encode_scancode(u64 protocols,
>
> Why a bitmask of protocols and not a single protocol enum? What's the
> use case for encoding a given scancode according to one out of a number
> of protocols (and not even knowing which one)??
>

I think bitmask was used simply for consistency reasons. Most of the
rc-core handles protocols in a bitmask (u64 protocols or some variant
of it). Especially in the decoders the dev->enabled_protocols is used
to mean "decode any of these protocols but I don't care which one" and
the encoders were written to follow that logic.

>From ir driver point of view it was also kind of nice to use the u64
enabled_wakeup_protocols from struct rc_dev which already exists and
is manipulated with the same sysfs code as the enabled_protocols
bitmask.

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote:
>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>>From: James Hogan <james@albanarts.com>
>>>
>>>Add a callback to raw ir handlers for encoding and modulating a scancode
>>>to a set of raw events. This could be used for transmit, or for
>>>converting a wakeup scancode filter to a form that is more suitable for
>>>raw hardware wake up filters.
>>>
>>>Signed-off-by: James Hogan <james@albanarts.com>
>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>>Cc: David Härdeman <david@hardeman.nu>
>>>---
>>>
>>>Notes:
>>>    Changes in v3:
>>>     - Ported to apply against latest media-tree
>>>
>>>    Changes in v2:
>>>     - Alter encode API to return -ENOBUFS when there isn't enough buffer
>>>       space. When this occurs all buffer contents must have been written
>>>       with the partial encoding of the scancode. This is to allow drivers
>>>       such as nuvoton-cir to provide a shorter buffer and still get a
>>>       useful partial encoding for the wakeup pattern.
>>>
>>> drivers/media/rc/rc-core-priv.h |  2 ++
>>> drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
>>> include/media/rc-core.h         |  3 +++
>>> 3 files changed, 42 insertions(+)
>>>
>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
>>>index b68d4f76..122c25f 100644
>>>--- a/drivers/media/rc/rc-core-priv.h
>>>+++ b/drivers/media/rc/rc-core-priv.h
>>>@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
>>>+                    struct ir_raw_event *events, unsigned int max);
>>>
>>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>>--- a/drivers/media/rc/rc-ir-raw.c
>>>+++ b/drivers/media/rc/rc-ir-raw.c
>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
>>>       return 0;
>>> }
>>>
>>>+/**
>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events
>>>+ *
>>>+ * @protocols:                permitted protocols
>>>+ * @scancode:         scancode filter describing a single scancode
>>>+ * @events:           array of raw events to write into
>>>+ * @max:              max number of raw events
>>>+ *
>>>+ * Attempts to encode the scancode as raw events.
>>>+ *
>>>+ * Returns:   The number of events written.
>>>+ *            -ENOBUFS if there isn't enough space in the array to fit the
>>>+ *            encoding. In this case all @max events will have been written.
>>>+ *            -EINVAL if the scancode is ambiguous or invalid, or if no
>>>+ *            compatible encoder was found.
>>>+ */
>>>+int ir_raw_encode_scancode(u64 protocols,
>>
>> Why a bitmask of protocols and not a single protocol enum? What's the
>> use case for encoding a given scancode according to one out of a number
>> of protocols (and not even knowing which one)??
>>
>
>I think bitmask was used simply for consistency reasons. Most of the
>rc-core handles protocols in a bitmask (u64 protocols or some variant
>of it).

Yes, all the parts where multiple protocols make sense use a bitmap of
protocols. If there's any part which uses a bitmap where only one
protocol makes sense that'd be a bug...

>Especially in the decoders the dev->enabled_protocols is used
>to mean "decode any of these protocols but I don't care which one" and
>the encoders were written to follow that logic.

The fact that you might want to be able to receive and decode more than
one protocol has no bearing on encoding when you're supposed to know
what it is you want to encode....

>From ir driver point of view it was also kind of nice to use the u64
>enabled_wakeup_protocols from struct rc_dev which already exists and
>is manipulated with the same sysfs code as the enabled_protocols
>bitmask.

But it makes no sense? "here's a scancode...I have no idea what it means
since only knowing the protocol allows you to make any sense of the
scancode...but please encode it to something...anything...."

-- 
David Härdeman

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote:
> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote:
>>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>>>From: James Hogan <james@albanarts.com>
>>>>
>>>>Add a callback to raw ir handlers for encoding and modulating a scancode
>>>>to a set of raw events. This could be used for transmit, or for
>>>>converting a wakeup scancode filter to a form that is more suitable for
>>>>raw hardware wake up filters.
>>>>
>>>>Signed-off-by: James Hogan <james@albanarts.com>
>>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>>>Cc: David Härdeman <david@hardeman.nu>
>>>>---
>>>>
>>>>Notes:
>>>>    Changes in v3:
>>>>     - Ported to apply against latest media-tree
>>>>
>>>>    Changes in v2:
>>>>     - Alter encode API to return -ENOBUFS when there isn't enough buffer
>>>>       space. When this occurs all buffer contents must have been written
>>>>       with the partial encoding of the scancode. This is to allow drivers
>>>>       such as nuvoton-cir to provide a shorter buffer and still get a
>>>>       useful partial encoding for the wakeup pattern.
>>>>
>>>> drivers/media/rc/rc-core-priv.h |  2 ++
>>>> drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
>>>> include/media/rc-core.h         |  3 +++
>>>> 3 files changed, 42 insertions(+)
>>>>
>>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
>>>>index b68d4f76..122c25f 100644
>>>>--- a/drivers/media/rc/rc-core-priv.h
>>>>+++ b/drivers/media/rc/rc-core-priv.h
>>>>@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
>>>>+                    struct ir_raw_event *events, unsigned int max);
>>>>
>>>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>>>--- a/drivers/media/rc/rc-ir-raw.c
>>>>+++ b/drivers/media/rc/rc-ir-raw.c
>>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
>>>>       return 0;
>>>> }
>>>>
>>>>+/**
>>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events
>>>>+ *
>>>>+ * @protocols:                permitted protocols
>>>>+ * @scancode:         scancode filter describing a single scancode
>>>>+ * @events:           array of raw events to write into
>>>>+ * @max:              max number of raw events
>>>>+ *
>>>>+ * Attempts to encode the scancode as raw events.
>>>>+ *
>>>>+ * Returns:   The number of events written.
>>>>+ *            -ENOBUFS if there isn't enough space in the array to fit the
>>>>+ *            encoding. In this case all @max events will have been written.
>>>>+ *            -EINVAL if the scancode is ambiguous or invalid, or if no
>>>>+ *            compatible encoder was found.
>>>>+ */
>>>>+int ir_raw_encode_scancode(u64 protocols,
>>>
>>> Why a bitmask of protocols and not a single protocol enum? What's the
>>> use case for encoding a given scancode according to one out of a number
>>> of protocols (and not even knowing which one)??
>>>
>>
>>I think bitmask was used simply for consistency reasons. Most of the
>>rc-core handles protocols in a bitmask (u64 protocols or some variant
>>of it).
>
> Yes, all the parts where multiple protocols make sense use a bitmap of
> protocols. If there's any part which uses a bitmap where only one
> protocol makes sense that'd be a bug...
>
>>Especially in the decoders the dev->enabled_protocols is used
>>to mean "decode any of these protocols but I don't care which one" and
>>the encoders were written to follow that logic.
>
> The fact that you might want to be able to receive and decode more than
> one protocol has no bearing on encoding when you're supposed to know
> what it is you want to encode....
>
> >From ir driver point of view it was also kind of nice to use the u64
>>enabled_wakeup_protocols from struct rc_dev which already exists and
>>is manipulated with the same sysfs code as the enabled_protocols
>>bitmask.
>
> But it makes no sense? "here's a scancode...I have no idea what it means
> since only knowing the protocol allows you to make any sense of the
> scancode...but please encode it to something...anything...."
>

Well it made sense from code simplicity point of view :)

But yes current use cases will most likely be valid when encoding only
to a single specific protocol at a time. However having a flexible api
allows for future expansion though if a new use case is needed to be
supported. And like I said earlier using bitmask is also consistent
with what rc-core already has.

That being said I don't object if you wish to propose a patch to
refactor the bitmask to be an enumeration instead.

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote:
>On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote:
>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote:
>>>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>>>>From: James Hogan <james@albanarts.com>
>>>>>
>>>>>Add a callback to raw ir handlers for encoding and modulating a scancode
>>>>>to a set of raw events. This could be used for transmit, or for
>>>>>converting a wakeup scancode filter to a form that is more suitable for
>>>>>raw hardware wake up filters.
>>>>>
>>>>>Signed-off-by: James Hogan <james@albanarts.com>
>>>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>>>>Cc: David Härdeman <david@hardeman.nu>
>>>>>---
>>>>>
>>>>>Notes:
>>>>>    Changes in v3:
>>>>>     - Ported to apply against latest media-tree
>>>>>
>>>>>    Changes in v2:
>>>>>     - Alter encode API to return -ENOBUFS when there isn't enough buffer
>>>>>       space. When this occurs all buffer contents must have been written
>>>>>       with the partial encoding of the scancode. This is to allow drivers
>>>>>       such as nuvoton-cir to provide a shorter buffer and still get a
>>>>>       useful partial encoding for the wakeup pattern.
>>>>>
>>>>> drivers/media/rc/rc-core-priv.h |  2 ++
>>>>> drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
>>>>> include/media/rc-core.h         |  3 +++
>>>>> 3 files changed, 42 insertions(+)
>>>>>
>>>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
>>>>>index b68d4f76..122c25f 100644
>>>>>--- a/drivers/media/rc/rc-core-priv.h
>>>>>+++ b/drivers/media/rc/rc-core-priv.h
>>>>>@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
>>>>>+                    struct ir_raw_event *events, unsigned int max);
>>>>>
>>>>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>>>>--- a/drivers/media/rc/rc-ir-raw.c
>>>>>+++ b/drivers/media/rc/rc-ir-raw.c
>>>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
>>>>>       return 0;
>>>>> }
>>>>>
>>>>>+/**
>>>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events
>>>>>+ *
>>>>>+ * @protocols:                permitted protocols
>>>>>+ * @scancode:         scancode filter describing a single scancode
>>>>>+ * @events:           array of raw events to write into
>>>>>+ * @max:              max number of raw events
>>>>>+ *
>>>>>+ * Attempts to encode the scancode as raw events.
>>>>>+ *
>>>>>+ * Returns:   The number of events written.
>>>>>+ *            -ENOBUFS if there isn't enough space in the array to fit the
>>>>>+ *            encoding. In this case all @max events will have been written.
>>>>>+ *            -EINVAL if the scancode is ambiguous or invalid, or if no
>>>>>+ *            compatible encoder was found.
>>>>>+ */
>>>>>+int ir_raw_encode_scancode(u64 protocols,
>>>>
>>>> Why a bitmask of protocols and not a single protocol enum? What's the
>>>> use case for encoding a given scancode according to one out of a number
>>>> of protocols (and not even knowing which one)??
>>>>
>>>
>>>I think bitmask was used simply for consistency reasons. Most of the
>>>rc-core handles protocols in a bitmask (u64 protocols or some variant
>>>of it).
>>
>> Yes, all the parts where multiple protocols make sense use a bitmap of
>> protocols. If there's any part which uses a bitmap where only one
>> protocol makes sense that'd be a bug...
>>
>>>Especially in the decoders the dev->enabled_protocols is used
>>>to mean "decode any of these protocols but I don't care which one" and
>>>the encoders were written to follow that logic.
>>
>> The fact that you might want to be able to receive and decode more than
>> one protocol has no bearing on encoding when you're supposed to know
>> what it is you want to encode....
>>
>> >From ir driver point of view it was also kind of nice to use the u64
>>>enabled_wakeup_protocols from struct rc_dev which already exists and
>>>is manipulated with the same sysfs code as the enabled_protocols
>>>bitmask.
>>
>> But it makes no sense? "here's a scancode...I have no idea what it means
>> since only knowing the protocol allows you to make any sense of the
>> scancode...but please encode it to something...anything...."
>>
>
>Well it made sense from code simplicity point of view :)
>
>But yes current use cases will most likely be valid when encoding only
>to a single specific protocol at a time. However having a flexible api
>allows for future expansion though if a new use case is needed to be
>supported. And like I said earlier using bitmask is also consistent
>with what rc-core already has.

* drivers/media/rc/img-ir/img-ir-hw.c
only seems to support one protocol at a time

* drivers/media/rc/rc-loopback.c
doesn't care

* drivers/media/rc/winbond-cir.c
seems hardware-wise very similar to nuvoton-cir.c, was not converted to
use the in-kernel encoders...has a private implementation

* drivers/media/rc/nuvoton-cir.c
is actually protocol agnostic but with your code it will encode
according to a randomly chosen protocol among the enabled ones, one that
will change depending on *module load order*...so unless I'm mistaken
you'll actually get different pulse/space timings written to hardware
depending on the order in which certain kernel modules have been
loaded...

Do you see why this is a bad idea?

>That being said I don't object if you wish to propose a patch to
>refactor the bitmask to be an enumeration instead.

Ehrm...I propose the patches be reverted until they're fixed.

-- 
David Härdeman

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 20 May 2015 at 23:45, David Härdeman <david@hardeman.nu> wrote:
> On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote:
>>On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote:
>>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote:
>>>>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
>>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>>>>>From: James Hogan <james@albanarts.com>
>>>>>>
>>>>>>Add a callback to raw ir handlers for encoding and modulating a scancode
>>>>>>to a set of raw events. This could be used for transmit, or for
>>>>>>converting a wakeup scancode filter to a form that is more suitable for
>>>>>>raw hardware wake up filters.
>>>>>>
>>>>>>Signed-off-by: James Hogan <james@albanarts.com>
>>>>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>>>>>Cc: David Härdeman <david@hardeman.nu>
>>>>>>---
>>>>>>
>>>>>>Notes:
>>>>>>    Changes in v3:
>>>>>>     - Ported to apply against latest media-tree
>>>>>>
>>>>>>    Changes in v2:
>>>>>>     - Alter encode API to return -ENOBUFS when there isn't enough buffer
>>>>>>       space. When this occurs all buffer contents must have been written
>>>>>>       with the partial encoding of the scancode. This is to allow drivers
>>>>>>       such as nuvoton-cir to provide a shorter buffer and still get a
>>>>>>       useful partial encoding for the wakeup pattern.
>>>>>>
>>>>>> drivers/media/rc/rc-core-priv.h |  2 ++
>>>>>> drivers/media/rc/rc-ir-raw.c    | 37 +++++++++++++++++++++++++++++++++++++
>>>>>> include/media/rc-core.h         |  3 +++
>>>>>> 3 files changed, 42 insertions(+)
>>>>>>
>>>>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
>>>>>>index b68d4f76..122c25f 100644
>>>>>>--- a/drivers/media/rc/rc-core-priv.h
>>>>>>+++ b/drivers/media/rc/rc-core-priv.h
>>>>>>@@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter *scancode,
>>>>>>+                    struct ir_raw_event *events, unsigned int max);
>>>>>>
>>>>>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>>>>>--- a/drivers/media/rc/rc-ir-raw.c
>>>>>>+++ b/drivers/media/rc/rc-ir-raw.c
>>>>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type)
>>>>>>       return 0;
>>>>>> }
>>>>>>
>>>>>>+/**
>>>>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events
>>>>>>+ *
>>>>>>+ * @protocols:                permitted protocols
>>>>>>+ * @scancode:         scancode filter describing a single scancode
>>>>>>+ * @events:           array of raw events to write into
>>>>>>+ * @max:              max number of raw events
>>>>>>+ *
>>>>>>+ * Attempts to encode the scancode as raw events.
>>>>>>+ *
>>>>>>+ * Returns:   The number of events written.
>>>>>>+ *            -ENOBUFS if there isn't enough space in the array to fit the
>>>>>>+ *            encoding. In this case all @max events will have been written.
>>>>>>+ *            -EINVAL if the scancode is ambiguous or invalid, or if no
>>>>>>+ *            compatible encoder was found.
>>>>>>+ */
>>>>>>+int ir_raw_encode_scancode(u64 protocols,
>>>>>
>>>>> Why a bitmask of protocols and not a single protocol enum? What's the
>>>>> use case for encoding a given scancode according to one out of a number
>>>>> of protocols (and not even knowing which one)??
>>>>>
>>>>
>>>>I think bitmask was used simply for consistency reasons. Most of the
>>>>rc-core handles protocols in a bitmask (u64 protocols or some variant
>>>>of it).
>>>
>>> Yes, all the parts where multiple protocols make sense use a bitmap of
>>> protocols. If there's any part which uses a bitmap where only one
>>> protocol makes sense that'd be a bug...
>>>
>>>>Especially in the decoders the dev->enabled_protocols is used
>>>>to mean "decode any of these protocols but I don't care which one" and
>>>>the encoders were written to follow that logic.
>>>
>>> The fact that you might want to be able to receive and decode more than
>>> one protocol has no bearing on encoding when you're supposed to know
>>> what it is you want to encode....
>>>
>>> >From ir driver point of view it was also kind of nice to use the u64
>>>>enabled_wakeup_protocols from struct rc_dev which already exists and
>>>>is manipulated with the same sysfs code as the enabled_protocols
>>>>bitmask.
>>>
>>> But it makes no sense? "here's a scancode...I have no idea what it means
>>> since only knowing the protocol allows you to make any sense of the
>>> scancode...but please encode it to something...anything...."
>>>
>>
>>Well it made sense from code simplicity point of view :)
>>
>>But yes current use cases will most likely be valid when encoding only
>>to a single specific protocol at a time. However having a flexible api
>>allows for future expansion though if a new use case is needed to be
>>supported. And like I said earlier using bitmask is also consistent
>>with what rc-core already has.
>
> * drivers/media/rc/img-ir/img-ir-hw.c
> only seems to support one protocol at a time
>
> * drivers/media/rc/rc-loopback.c
> doesn't care
>
> * drivers/media/rc/winbond-cir.c
> seems hardware-wise very similar to nuvoton-cir.c, was not converted to
> use the in-kernel encoders...has a private implementation
>
> * drivers/media/rc/nuvoton-cir.c
> is actually protocol agnostic but with your code it will encode
> according to a randomly chosen protocol among the enabled ones, one that
> will change depending on *module load order*...so unless I'm mistaken
> you'll actually get different pulse/space timings written to hardware
> depending on the order in which certain kernel modules have been
> loaded...
>
> Do you see why this is a bad idea?
>

I see that this is not optimal to leave the proper usage up to the
user but in a way we must work with what we have. See below:

>>That being said I don't object if you wish to propose a patch to
>>refactor the bitmask to be an enumeration instead.
>
> Ehrm...I propose the patches be reverted until they're fixed.
>

Reverting the patch series will not fix what I think you consider
broken. The series has very little to do with the sysfs api for wakeup
protocols that is already in place and which makes drivers (or
encoders) behave in a certain way.

I'm talking about the /sys/class/rc/<rc-device>/wakeup_protocols file
that was included around kernel 3.15 and is being already used by at
least img-ir driver regardless of whether support for encoding ir
scancodes is included or not. The wakeup_protocols allows user to
select multiple protocols because it works in a similar fashion to
protocols file.

I think that you would like to introduce a change to the behavior of
this file to only allow selecting single protocol at a time and
error-out if multiple protocols are enabled but this would break an
existing api to userspace which we are really not allowed to do.

And unless I'm mistaken even if we decided to change the behavior of
the wakeup_protcols file that change would have nothing to do with
this ir-encoding patch series (besides allowing usage of enumerations
instead of bitmasks).

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On 2015-05-21 09:53, Antti Seppälä wrote:
> On 20 May 2015 at 23:45, David Härdeman <david@hardeman.nu> wrote:
>> On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote:
>>> On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote:
>>>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote:
>>>>> On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
>>>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>>>>>> From: James Hogan <james@albanarts.com>
>>>>>>> 
>>>>>>> Add a callback to raw ir handlers for encoding and modulating a 
>>>>>>> scancode
>>>>>>> to a set of raw events. This could be used for transmit, or for
>>>>>>> converting a wakeup scancode filter to a form that is more 
>>>>>>> suitable for
>>>>>>> raw hardware wake up filters.
>>>>>>> 
>>>>>>> Signed-off-by: James Hogan <james@albanarts.com>
>>>>>>> Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>>>>>> Cc: David Härdeman <david@hardeman.nu>
>>>>>>> ---
>>>>>>> 
>>>>>>> Notes:
>>>>>>>    Changes in v3:
>>>>>>>     - Ported to apply against latest media-tree
>>>>>>> 
>>>>>>>    Changes in v2:
>>>>>>>     - Alter encode API to return -ENOBUFS when there isn't enough 
>>>>>>> buffer
>>>>>>>       space. When this occurs all buffer contents must have been 
>>>>>>> written
>>>>>>>       with the partial encoding of the scancode. This is to allow 
>>>>>>> drivers
>>>>>>>       such as nuvoton-cir to provide a shorter buffer and still 
>>>>>>> get a
>>>>>>>       useful partial encoding for the wakeup pattern.
>>>>>>> 
>>>>>>> drivers/media/rc/rc-core-priv.h |  2 ++
>>>>>>> drivers/media/rc/rc-ir-raw.c    | 37 
>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>> include/media/rc-core.h         |  3 +++
>>>>>>> 3 files changed, 42 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/drivers/media/rc/rc-core-priv.h 
>>>>>>> b/drivers/media/rc/rc-core-priv.h
>>>>>>> index b68d4f76..122c25f 100644
>>>>>>> --- a/drivers/media/rc/rc-core-priv.h
>>>>>>> +++ b/drivers/media/rc/rc-core-priv.h
>>>>>>> @@ -25,6 +25,8 @@ 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)(u64 protocols, const struct 
>>>>>>> rc_scancode_filter *scancode,
>>>>>>> +                    struct ir_raw_event *events, unsigned int 
>>>>>>> max);
>>>>>>> 
>>>>>>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>>>>>> --- a/drivers/media/rc/rc-ir-raw.c
>>>>>>> +++ b/drivers/media/rc/rc-ir-raw.c
>>>>>>> @@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev 
>>>>>>> *dev, u64 *rc_type)
>>>>>>>       return 0;
>>>>>>> }
>>>>>>> 
>>>>>>> +/**
>>>>>>> + * ir_raw_encode_scancode() - Encode a scancode as raw events
>>>>>>> + *
>>>>>>> + * @protocols:                permitted protocols
>>>>>>> + * @scancode:         scancode filter describing a single 
>>>>>>> scancode
>>>>>>> + * @events:           array of raw events to write into
>>>>>>> + * @max:              max number of raw events
>>>>>>> + *
>>>>>>> + * Attempts to encode the scancode as raw events.
>>>>>>> + *
>>>>>>> + * Returns:   The number of events written.
>>>>>>> + *            -ENOBUFS if there isn't enough space in the array 
>>>>>>> to fit the
>>>>>>> + *            encoding. In this case all @max events will have 
>>>>>>> been written.
>>>>>>> + *            -EINVAL if the scancode is ambiguous or invalid, 
>>>>>>> or if no
>>>>>>> + *            compatible encoder was found.
>>>>>>> + */
>>>>>>> +int ir_raw_encode_scancode(u64 protocols,
>>>>>> 
>>>>>> Why a bitmask of protocols and not a single protocol enum? What's 
>>>>>> the
>>>>>> use case for encoding a given scancode according to one out of a 
>>>>>> number
>>>>>> of protocols (and not even knowing which one)??
>>>>>> 
>>>>> 
>>>>> I think bitmask was used simply for consistency reasons. Most of 
>>>>> the
>>>>> rc-core handles protocols in a bitmask (u64 protocols or some 
>>>>> variant
>>>>> of it).
>>>> 
>>>> Yes, all the parts where multiple protocols make sense use a bitmap 
>>>> of
>>>> protocols. If there's any part which uses a bitmap where only one
>>>> protocol makes sense that'd be a bug...
>>>> 
>>>>> Especially in the decoders the dev->enabled_protocols is used
>>>>> to mean "decode any of these protocols but I don't care which one" 
>>>>> and
>>>>> the encoders were written to follow that logic.
>>>> 
>>>> The fact that you might want to be able to receive and decode more 
>>>> than
>>>> one protocol has no bearing on encoding when you're supposed to know
>>>> what it is you want to encode....
>>>> 
>>>> >From ir driver point of view it was also kind of nice to use the u64
>>>>> enabled_wakeup_protocols from struct rc_dev which already exists 
>>>>> and
>>>>> is manipulated with the same sysfs code as the enabled_protocols
>>>>> bitmask.
>>>> 
>>>> But it makes no sense? "here's a scancode...I have no idea what it 
>>>> means
>>>> since only knowing the protocol allows you to make any sense of the
>>>> scancode...but please encode it to something...anything...."
>>>> 
>>> 
>>> Well it made sense from code simplicity point of view :)
>>> 
>>> But yes current use cases will most likely be valid when encoding 
>>> only
>>> to a single specific protocol at a time. However having a flexible 
>>> api
>>> allows for future expansion though if a new use case is needed to be
>>> supported. And like I said earlier using bitmask is also consistent
>>> with what rc-core already has.
>> 
>> * drivers/media/rc/img-ir/img-ir-hw.c
>> only seems to support one protocol at a time
>> 
>> * drivers/media/rc/rc-loopback.c
>> doesn't care
>> 
>> * drivers/media/rc/winbond-cir.c
>> seems hardware-wise very similar to nuvoton-cir.c, was not converted 
>> to
>> use the in-kernel encoders...has a private implementation
>> 
>> * drivers/media/rc/nuvoton-cir.c
>> is actually protocol agnostic but with your code it will encode
>> according to a randomly chosen protocol among the enabled ones, one 
>> that
>> will change depending on *module load order*...so unless I'm mistaken
>> you'll actually get different pulse/space timings written to hardware
>> depending on the order in which certain kernel modules have been
>> loaded...
>> 
>> Do you see why this is a bad idea?
>> 
> 
> I see that this is not optimal to leave the proper usage up to the
> user but in a way we must work with what we have. See below:
> 
>>> That being said I don't object if you wish to propose a patch to
>>> refactor the bitmask to be an enumeration instead.
>> 
>> Ehrm...I propose the patches be reverted until they're fixed.
>> 
> 
> Reverting the patch series will not fix what I think you consider
> broken. The series has very little to do with the sysfs api for wakeup
> protocols that is already in place and which makes drivers (or
> encoders) behave in a certain way.
> 
> I'm talking about the /sys/class/rc/<rc-device>/wakeup_protocols file
> that was included around kernel 3.15 and is being already used by at
> least img-ir driver regardless of whether support for encoding ir
> scancodes is included or not. The wakeup_protocols allows user to
> select multiple protocols because it works in a similar fashion to
> protocols file.
> 
> I think that you would like to introduce a change to the behavior of
> this file to only allow selecting single protocol at a time and
> error-out if multiple protocols are enabled but this would break an
> existing api to userspace which we are really not allowed to do.
> 
> And unless I'm mistaken even if we decided to change the behavior of
> the wakeup_protcols file that change would have nothing to do with
> this ir-encoding patch series (besides allowing usage of enumerations
> instead of bitmasks).

I'm talking about ir_raw_encode_scancode() which is entirely broken in 
its current state. It will, given more than one enabled protocol, encode 
a scancode to pulse/space events according to the rules of a randomly 
chosen protocol. That random selection will be influenced by things like 
*module load order* (independent of the separate fact that passing 
multiple protocols to it is completely bogus in the first place).

To be clear: the same scancode may be encoded differently depending on 
if you've load the nec decoder before or after the rc5 decoder! That 
kind of behavior can't go into a release kernel (Mauro...).

Yes, the sysfs interface for selecting the wakeup protocol(s) is also 
not great and might need to be changed or augmented, but that's a 
different issue.

(Resent, forgot to do reply-all).

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
> On 2015-05-21 09:53, Antti Seppälä wrote:
>>
>> On 20 May 2015 at 23:45, David Härdeman <david@hardeman.nu> wrote:
>>>
>>> On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote:
>>>>
>>>> On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote:
>>>>>
>>>>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote:
>>>>>>
>>>>>> On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote:
>>>>>>>
>>>>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
>>>>>>>>
>>>>>>>> From: James Hogan <james@albanarts.com>
>>>>>>>>
>>>>>>>> Add a callback to raw ir handlers for encoding and modulating a
>>>>>>>> scancode
>>>>>>>> to a set of raw events. This could be used for transmit, or for
>>>>>>>> converting a wakeup scancode filter to a form that is more suitable
>>>>>>>> for
>>>>>>>> raw hardware wake up filters.
>>>>>>>>
>>>>>>>> Signed-off-by: James Hogan <james@albanarts.com>
>>>>>>>> Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
>>>>>>>> Cc: David Härdeman <david@hardeman.nu>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Notes:
>>>>>>>>    Changes in v3:
>>>>>>>>     - Ported to apply against latest media-tree
>>>>>>>>
>>>>>>>>    Changes in v2:
>>>>>>>>     - Alter encode API to return -ENOBUFS when there isn't enough
>>>>>>>> buffer
>>>>>>>>       space. When this occurs all buffer contents must have been
>>>>>>>> written
>>>>>>>>       with the partial encoding of the scancode. This is to allow
>>>>>>>> drivers
>>>>>>>>       such as nuvoton-cir to provide a shorter buffer and still get
>>>>>>>> a
>>>>>>>>       useful partial encoding for the wakeup pattern.
>>>>>>>>
>>>>>>>> drivers/media/rc/rc-core-priv.h |  2 ++
>>>>>>>> drivers/media/rc/rc-ir-raw.c    | 37
>>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>>> include/media/rc-core.h         |  3 +++
>>>>>>>> 3 files changed, 42 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/rc/rc-core-priv.h
>>>>>>>> b/drivers/media/rc/rc-core-priv.h
>>>>>>>> index b68d4f76..122c25f 100644
>>>>>>>> --- a/drivers/media/rc/rc-core-priv.h
>>>>>>>> +++ b/drivers/media/rc/rc-core-priv.h
>>>>>>>> @@ -25,6 +25,8 @@ 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)(u64 protocols, const struct rc_scancode_filter
>>>>>>>> *scancode,
>>>>>>>> +                    struct ir_raw_event *events, unsigned int max);
>>>>>>>>
>>>>>>>>       /* These two should only be used by the lirc 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 b732ac6..dd47fe5 100644
>>>>>>>> --- a/drivers/media/rc/rc-ir-raw.c
>>>>>>>> +++ b/drivers/media/rc/rc-ir-raw.c
>>>>>>>> @@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev,
>>>>>>>> u64 *rc_type)
>>>>>>>>       return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * ir_raw_encode_scancode() - Encode a scancode as raw events
>>>>>>>> + *
>>>>>>>> + * @protocols:                permitted protocols
>>>>>>>> + * @scancode:         scancode filter describing a single scancode
>>>>>>>> + * @events:           array of raw events to write into
>>>>>>>> + * @max:              max number of raw events
>>>>>>>> + *
>>>>>>>> + * Attempts to encode the scancode as raw events.
>>>>>>>> + *
>>>>>>>> + * Returns:   The number of events written.
>>>>>>>> + *            -ENOBUFS if there isn't enough space in the array to
>>>>>>>> fit the
>>>>>>>> + *            encoding. In this case all @max events will have been
>>>>>>>> written.
>>>>>>>> + *            -EINVAL if the scancode is ambiguous or invalid, or
>>>>>>>> if no
>>>>>>>> + *            compatible encoder was found.
>>>>>>>> + */
>>>>>>>> +int ir_raw_encode_scancode(u64 protocols,
>>>>>>>
>>>>>>>
>>>>>>> Why a bitmask of protocols and not a single protocol enum? What's the
>>>>>>> use case for encoding a given scancode according to one out of a
>>>>>>> number
>>>>>>> of protocols (and not even knowing which one)??
>>>>>>>
>>>>>>
>>>>>> I think bitmask was used simply for consistency reasons. Most of the
>>>>>> rc-core handles protocols in a bitmask (u64 protocols or some variant
>>>>>> of it).
>>>>>
>>>>>
>>>>> Yes, all the parts where multiple protocols make sense use a bitmap of
>>>>> protocols. If there's any part which uses a bitmap where only one
>>>>> protocol makes sense that'd be a bug...
>>>>>
>>>>>> Especially in the decoders the dev->enabled_protocols is used
>>>>>> to mean "decode any of these protocols but I don't care which one" and
>>>>>> the encoders were written to follow that logic.
>>>>>
>>>>>
>>>>> The fact that you might want to be able to receive and decode more than
>>>>> one protocol has no bearing on encoding when you're supposed to know
>>>>> what it is you want to encode....
>>>>>
>>>>> >From ir driver point of view it was also kind of nice to use the u64
>>>>>>
>>>>>> enabled_wakeup_protocols from struct rc_dev which already exists and
>>>>>> is manipulated with the same sysfs code as the enabled_protocols
>>>>>> bitmask.
>>>>>
>>>>>
>>>>> But it makes no sense? "here's a scancode...I have no idea what it
>>>>> means
>>>>> since only knowing the protocol allows you to make any sense of the
>>>>> scancode...but please encode it to something...anything...."
>>>>>
>>>>
>>>> Well it made sense from code simplicity point of view :)
>>>>
>>>> But yes current use cases will most likely be valid when encoding only
>>>> to a single specific protocol at a time. However having a flexible api
>>>> allows for future expansion though if a new use case is needed to be
>>>> supported. And like I said earlier using bitmask is also consistent
>>>> with what rc-core already has.
>>>
>>>
>>> * drivers/media/rc/img-ir/img-ir-hw.c
>>> only seems to support one protocol at a time
>>>
>>> * drivers/media/rc/rc-loopback.c
>>> doesn't care
>>>
>>> * drivers/media/rc/winbond-cir.c
>>> seems hardware-wise very similar to nuvoton-cir.c, was not converted to
>>> use the in-kernel encoders...has a private implementation
>>>
>>> * drivers/media/rc/nuvoton-cir.c
>>> is actually protocol agnostic but with your code it will encode
>>> according to a randomly chosen protocol among the enabled ones, one that
>>> will change depending on *module load order*...so unless I'm mistaken
>>> you'll actually get different pulse/space timings written to hardware
>>> depending on the order in which certain kernel modules have been
>>> loaded...
>>>
>>> Do you see why this is a bad idea?
>>>
>>
>> I see that this is not optimal to leave the proper usage up to the
>> user but in a way we must work with what we have. See below:
>>
>>>> That being said I don't object if you wish to propose a patch to
>>>> refactor the bitmask to be an enumeration instead.
>>>
>>>
>>> Ehrm...I propose the patches be reverted until they're fixed.
>>>
>>
>> Reverting the patch series will not fix what I think you consider
>> broken. The series has very little to do with the sysfs api for wakeup
>> protocols that is already in place and which makes drivers (or
>> encoders) behave in a certain way.
>>
>> I'm talking about the /sys/class/rc/<rc-device>/wakeup_protocols file
>> that was included around kernel 3.15 and is being already used by at
>> least img-ir driver regardless of whether support for encoding ir
>> scancodes is included or not. The wakeup_protocols allows user to
>> select multiple protocols because it works in a similar fashion to
>> protocols file.
>>
>> I think that you would like to introduce a change to the behavior of
>> this file to only allow selecting single protocol at a time and
>> error-out if multiple protocols are enabled but this would break an
>> existing api to userspace which we are really not allowed to do.
>>
>> And unless I'm mistaken even if we decided to change the behavior of
>> the wakeup_protcols file that change would have nothing to do with
>> this ir-encoding patch series (besides allowing usage of enumerations
>> instead of bitmasks).
>
>
> I'm talking about ir_raw_encode_scancode() which is entirely broken in its
> current state. It will, given more than one enabled protocol, encode a
> scancode to pulse/space events according to the rules of a randomly chosen
> protocol. That random selection will be influenced by things like *module
> load order* (independent of the separate fact that passing multiple
> protocols to it is completely bogus in the first place).
>
> To be clear: the same scancode may be encoded differently depending on if
> you've load the nec decoder before or after the rc5 decoder! That kind of
> behavior can't go into a release kernel (Mauro...).
>

So... if the ir_raw_handler_list is sorted to eliminate the randomness
caused by module load ordering you will be happy (or happier)?

That is something that could be useful even for the ir-decoding
functionality and might be worth a separate patch.

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On 2015-05-21 13:51, Antti Seppälä wrote:
> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
>> I'm talking about ir_raw_encode_scancode() which is entirely broken in 
>> its
>> current state. It will, given more than one enabled protocol, encode a
>> scancode to pulse/space events according to the rules of a randomly 
>> chosen
>> protocol. That random selection will be influenced by things like 
>> *module
>> load order* (independent of the separate fact that passing multiple
>> protocols to it is completely bogus in the first place).
>> 
>> To be clear: the same scancode may be encoded differently depending on 
>> if
>> you've load the nec decoder before or after the rc5 decoder! That kind 
>> of
>> behavior can't go into a release kernel (Mauro...).
>> 
> 
> So... if the ir_raw_handler_list is sorted to eliminate the randomness
> caused by module load ordering you will be happy (or happier)?

No, cause it's a horrible hack. And the caller of ir_raw_handler_list() 
still has no idea of knowing (given more than one protocol) which 
protocol a given scancode will be encoded according to.

> That is something that could be useful even for the ir-decoding
> functionality and might be worth a separate patch.

Useful how?

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote:
> On 2015-05-21 13:51, Antti Seppälä wrote:
>>
>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
>>>
>>> I'm talking about ir_raw_encode_scancode() which is entirely broken in
>>> its
>>> current state. It will, given more than one enabled protocol, encode a
>>> scancode to pulse/space events according to the rules of a randomly
>>> chosen
>>> protocol. That random selection will be influenced by things like *module
>>> load order* (independent of the separate fact that passing multiple
>>> protocols to it is completely bogus in the first place).
>>>
>>> To be clear: the same scancode may be encoded differently depending on if
>>> you've load the nec decoder before or after the rc5 decoder! That kind of
>>> behavior can't go into a release kernel (Mauro...).
>>>
>>
>> So... if the ir_raw_handler_list is sorted to eliminate the randomness
>> caused by module load ordering you will be happy (or happier)?
>
>
> No, cause it's a horrible hack. And the caller of ir_raw_handler_list()
> still has no idea of knowing (given more than one protocol) which protocol a
> given scancode will be encoded according to.
>

Okay, so how about "demuxing" the first protocol before handing them
to encoder callback? Simply something like below:

-               if (handler->protocols & protocols && handler->encode) {
+               if (handler->protocols & ffs(protocols) && handler->encode) {

Now the behavior is well-defined even when multiple protocols are selected.

>> That is something that could be useful even for the ir-decoding
>> functionality and might be worth a separate patch.
>
>
> Useful how?

Keeping dynamically filled lists sorted is a good practice if one
wishes to achieve determinism in behavior (like running decoders
always in certain order too).

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote:
>On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote:
>> On 2015-05-21 13:51, Antti Seppälä wrote:
>>>
>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
>>>>
>>>> I'm talking about ir_raw_encode_scancode() which is entirely broken in
>>>> its
>>>> current state. It will, given more than one enabled protocol, encode a
>>>> scancode to pulse/space events according to the rules of a randomly
>>>> chosen
>>>> protocol. That random selection will be influenced by things like *module
>>>> load order* (independent of the separate fact that passing multiple
>>>> protocols to it is completely bogus in the first place).
>>>>
>>>> To be clear: the same scancode may be encoded differently depending on if
>>>> you've load the nec decoder before or after the rc5 decoder! That kind of
>>>> behavior can't go into a release kernel (Mauro...).
>>>>
>>>
>>> So... if the ir_raw_handler_list is sorted to eliminate the randomness
>>> caused by module load ordering you will be happy (or happier)?
>>
>>
>> No, cause it's a horrible hack. And the caller of ir_raw_handler_list()
>> still has no idea of knowing (given more than one protocol) which protocol a
>> given scancode will be encoded according to.
>>
>
>Okay, so how about "demuxing" the first protocol before handing them
>to encoder callback? Simply something like below:
>
>-               if (handler->protocols & protocols && handler->encode) {
>+               if (handler->protocols & ffs(protocols) && handler->encode) {
>
>Now the behavior is well-defined even when multiple protocols are selected.

Your patchset introduced ir_raw_encode_scancode() as well as the only
two callers of that function:

drivers/media/rc/nuvoton-cir.c
drivers/media/rc/rc-loopback.c

I realize that the sysfs wakeup_protocols file (which bakes several
protocols into one label) makes defining *the* protocol difficult, but
if you're going to add hacks like this, keep them to the sole driver
using the API rather than the core API itself.

That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a
single protocol, no matter how many are passed to it (the img-ir driver
already sets a precedent here so it wouldn't be an API change to change
to a set of protocols which might be different than what the user
suggested). (Also...yes, that'll make supporting several versions of
e.g. RC6 impossible with the current sysfs code).

Then change both ir_raw_encode_scancode() to take a single protocol enum
and change the *encode function pointer in struct ir_raw_handler to also
take a single protocol enum.

That way encoders won't have to guess (using scanmasks...!?) what
protocol they should encode to.

And Mauro...I strongly suggest you revert all of this encoding stuff
until this has been fixed...it's broken.

>>
>> Useful how?
>
>Keeping dynamically filled lists sorted is a good practice if one
>wishes to achieve determinism in behavior (like running decoders
>always in certain order too).

That's a new "good practice" to me...

-- 
David Härdeman

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 21 May 2015 at 22:40, David Härdeman <david@hardeman.nu> wrote:
> On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote:
>>On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote:
>>> On 2015-05-21 13:51, Antti Seppälä wrote:
>>>>
>>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
>>>>>
>>>>> I'm talking about ir_raw_encode_scancode() which is entirely broken in
>>>>> its
>>>>> current state. It will, given more than one enabled protocol, encode a
>>>>> scancode to pulse/space events according to the rules of a randomly
>>>>> chosen
>>>>> protocol. That random selection will be influenced by things like *module
>>>>> load order* (independent of the separate fact that passing multiple
>>>>> protocols to it is completely bogus in the first place).
>>>>>
>>>>> To be clear: the same scancode may be encoded differently depending on if
>>>>> you've load the nec decoder before or after the rc5 decoder! That kind of
>>>>> behavior can't go into a release kernel (Mauro...).
>>>>>
>>>>
>>>> So... if the ir_raw_handler_list is sorted to eliminate the randomness
>>>> caused by module load ordering you will be happy (or happier)?
>>>
>>>
>>> No, cause it's a horrible hack. And the caller of ir_raw_handler_list()
>>> still has no idea of knowing (given more than one protocol) which protocol a
>>> given scancode will be encoded according to.
>>>
>>
>>Okay, so how about "demuxing" the first protocol before handing them
>>to encoder callback? Simply something like below:
>>
>>-               if (handler->protocols & protocols && handler->encode) {
>>+               if (handler->protocols & ffs(protocols) && handler->encode) {
>>
>>Now the behavior is well-defined even when multiple protocols are selected.
>
> Your patchset introduced ir_raw_encode_scancode() as well as the only
> two callers of that function:
>
> drivers/media/rc/nuvoton-cir.c
> drivers/media/rc/rc-loopback.c
>
> I realize that the sysfs wakeup_protocols file (which bakes several
> protocols into one label) makes defining *the* protocol difficult, but
> if you're going to add hacks like this, keep them to the sole driver
> using the API rather than the core API itself.
>
> That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a
> single protocol, no matter how many are passed to it (the img-ir driver
> already sets a precedent here so it wouldn't be an API change to change
> to a set of protocols which might be different than what the user
> suggested). (Also...yes, that'll make supporting several versions of
> e.g. RC6 impossible with the current sysfs code).
>

I think that approach too is far from perfect as it leaves us with
questions such as: How do we let the user know what variant of
protocol the label "rc-6" really means? If in nvt we hardcode it to
mean RC6-0-16 and a new driver cames along which chooses
RC_TYPE_RC6_6A_24 how do we tell the user that the implementations
differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE?

If only there were a sysfs api to set the exact variant life would be simpler...

> Then change both ir_raw_encode_scancode() to take a single protocol enum
> and change the *encode function pointer in struct ir_raw_handler to also
> take a single protocol enum.
>
> That way encoders won't have to guess (using scanmasks...!?) what
> protocol they should encode to.
>
> And Mauro...I strongly suggest you revert all of this encoding stuff
> until this has been fixed...it's broken.
>

Let me shortly recap the points made so far about the encoding stuff:

* If user enables multiple wakeup_protocols then the first matching
one will be used to do the encoding. "First" being the module that was
loaded first.

* Currently the encoders use heuristics to determine the intended
protocol variant from the scancode that was fed and from the length of
the scanmask. This causes the programmer using the encoder to not know
which exact variant will be used (until after the encoding is done
when the information can be made available if needed). The way current
sysfs api works using heuristics was a design choice since we don't
have a better way to specify the protocol variant.

Hope I didn't miss anything?

So yeah.. The code isn't "broken" in a sense that it wouldn't work.
It's more a question of what we want the api to look like.

Mauro what do you think should be done?

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On 2015-05-22 07:27, Antti Seppälä wrote:
> On 21 May 2015 at 22:40, David Härdeman <david@hardeman.nu> wrote:
>> On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote:
>>> On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote:
>>>> On 2015-05-21 13:51, Antti Seppälä wrote:
>>>>> 
>>>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
>>>>>> 
>>>>>> I'm talking about ir_raw_encode_scancode() which is entirely 
>>>>>> broken in
>>>>>> its
>>>>>> current state. It will, given more than one enabled protocol, 
>>>>>> encode a
>>>>>> scancode to pulse/space events according to the rules of a 
>>>>>> randomly
>>>>>> chosen
>>>>>> protocol. That random selection will be influenced by things like 
>>>>>> *module
>>>>>> load order* (independent of the separate fact that passing 
>>>>>> multiple
>>>>>> protocols to it is completely bogus in the first place).
>>>>>> 
>>>>>> To be clear: the same scancode may be encoded differently 
>>>>>> depending on if
>>>>>> you've load the nec decoder before or after the rc5 decoder! That 
>>>>>> kind of
>>>>>> behavior can't go into a release kernel (Mauro...).
>>>>>> 
>>>>> 
>>>>> So... if the ir_raw_handler_list is sorted to eliminate the 
>>>>> randomness
>>>>> caused by module load ordering you will be happy (or happier)?
>>>> 
>>>> 
>>>> No, cause it's a horrible hack. And the caller of 
>>>> ir_raw_handler_list()
>>>> still has no idea of knowing (given more than one protocol) which 
>>>> protocol a
>>>> given scancode will be encoded according to.
>>>> 
>>> 
>>> Okay, so how about "demuxing" the first protocol before handing them
>>> to encoder callback? Simply something like below:
>>> 
>>> -               if (handler->protocols & protocols && 
>>> handler->encode) {
>>> +               if (handler->protocols & ffs(protocols) && 
>>> handler->encode) {
>>> 
>>> Now the behavior is well-defined even when multiple protocols are 
>>> selected.
>> 
>> Your patchset introduced ir_raw_encode_scancode() as well as the only
>> two callers of that function:
>> 
>> drivers/media/rc/nuvoton-cir.c
>> drivers/media/rc/rc-loopback.c
>> 
>> I realize that the sysfs wakeup_protocols file (which bakes several
>> protocols into one label) makes defining *the* protocol difficult, but
>> if you're going to add hacks like this, keep them to the sole driver
>> using the API rather than the core API itself.
>> 
>> That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a
>> single protocol, no matter how many are passed to it (the img-ir 
>> driver
>> already sets a precedent here so it wouldn't be an API change to 
>> change
>> to a set of protocols which might be different than what the user
>> suggested). (Also...yes, that'll make supporting several versions of
>> e.g. RC6 impossible with the current sysfs code).
>> 
> 
> I think that approach too is far from perfect as it leaves us with
> questions such as: How do we let the user know what variant of
> protocol the label "rc-6" really means? If in nvt we hardcode it to
> mean RC6-0-16 and a new driver cames along which chooses
> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations
> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE?

I never claimed it was perfect.

For another (short-term, per-driver) solution, see the winbond-cir 
driver.

> If only there were a sysfs api to set the exact variant life would be 
> simpler...

Yes, and your patches made it harder to get to a sane solution.

>> Then change both ir_raw_encode_scancode() to take a single protocol 
>> enum
>> and change the *encode function pointer in struct ir_raw_handler to 
>> also
>> take a single protocol enum.
>> 
>> That way encoders won't have to guess (using scanmasks...!?) what
>> protocol they should encode to.
>> 
>> And Mauro...I strongly suggest you revert all of this encoding stuff
>> until this has been fixed...it's broken.
>> 
> 
> Let me shortly recap the points made so far about the encoding stuff:
> 
> * If user enables multiple wakeup_protocols then the first matching
> one will be used to do the encoding. "First" being the module that was
> loaded first.

That in itself would be a good enough reason to revert the patches.

> * Currently the encoders use heuristics to determine the intended
> protocol variant from the scancode that was fed and from the length of
> the scanmask. This causes the programmer using the encoder to not know
> which exact variant will be used (until after the encoding is done
> when the information can be made available if needed).

First, "currently" implies that the heuristics can later be changed. 
They can't once this becomes part of a released kernel (not without 
breaking the kernel API).

Second, how do you plan to pass the information about the chosen 
protocol back to userspace? And what is userspace supposed to do with 
the information?
* Userspace: please set the hardware to wake up if this RC6 mode0 
command is received
* Kernel: sure...I've set the hardware to wake up if a RC6 mode6a 
command is received
* Userspace: WTF?

Is the next step to go buy a new remote which matches what the kernel 
told you?

Third, that the scanmask is suddenly used to determine the meaning of a 
scancode is in itself an API break.

Fourth, the "programmer" is not the problem. The problem is the 
user(space). A user who writes e.g. a RC6-something wakeup scancode has 
no way of knowing according to which protocol the scancode will be 
interpreted. Meaning that even if:

* the user knows he has a remote control in his hand which generates RC6 
mode0 commands; and
* he limits the wake protocols to "rc6"; and
* he writes the correct scancode to sysfs

then he still can't know that the hardware is correctly configured. And 
that might change depending on things like scanmask heuristics, module 
load order and the phase of the moon.

> The way current
> sysfs api works using heuristics was a design choice since we don't
> have a better way to specify the protocol variant.
> 
> Hope I didn't miss anything?

You missed that once this goes in the API is going to be next to 
impossible to fix.

> So yeah.. The code isn't "broken" in a sense that it wouldn't work.

It's entirely broken in the sense that a user has no idea of knowing if 
the hardware has been properly configured to wake the computer or not. 
It's just as broken as the connect() system call would be if it randomly 
rewrote the destination address passed by the user, optionally 
connected, and most of the time returned zero independently of the 
result.

I can't really believe we're still debating *if* this code should stay 
in it's present form.

> It's more a question of what we want the api to look like.

And if the current version is part of a released kernel we can never fix 
it.

Look, there are fundamental issues right now in rc-core in that the only 
way a scancode can have any meaning is in a protocol-scancode tuple 
(single protocol, of course) and that information is missing in many 
places. That's something I'm trying to fix (see the updated set/get 
keytable ioctls for example) and Mauro seems to mostly want to forget 
about. Fixing it is probably going to be impossible without API breaks. 
Your code encodes more of that crap right in the core of rc-core and 
will require even more API breaks.

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 22 May 2015 at 13:33, David Härdeman <david@hardeman.nu> wrote:
> On 2015-05-22 07:27, Antti Seppälä wrote:
>>
>> On 21 May 2015 at 22:40, David Härdeman <david@hardeman.nu> wrote:
>>>
>>> On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote:
>>>>
>>>> On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote:
>>>>>
>>>>> On 2015-05-21 13:51, Antti Seppälä wrote:
>>>>>>
>>>>>>
>>>>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote:
>>>>>>>
>>>>>>>
>>>>>>> I'm talking about ir_raw_encode_scancode() which is entirely broken
>>>>>>> in
>>>>>>> its
>>>>>>> current state. It will, given more than one enabled protocol, encode
>>>>>>> a
>>>>>>> scancode to pulse/space events according to the rules of a randomly
>>>>>>> chosen
>>>>>>> protocol. That random selection will be influenced by things like
>>>>>>> *module
>>>>>>> load order* (independent of the separate fact that passing multiple
>>>>>>> protocols to it is completely bogus in the first place).
>>>>>>>
>>>>>>> To be clear: the same scancode may be encoded differently depending
>>>>>>> on if
>>>>>>> you've load the nec decoder before or after the rc5 decoder! That
>>>>>>> kind of
>>>>>>> behavior can't go into a release kernel (Mauro...).
>>>>>>>
>>>>>>
>>>>>> So... if the ir_raw_handler_list is sorted to eliminate the randomness
>>>>>> caused by module load ordering you will be happy (or happier)?
>>>>>
>>>>>
>>>>>
>>>>> No, cause it's a horrible hack. And the caller of ir_raw_handler_list()
>>>>> still has no idea of knowing (given more than one protocol) which
>>>>> protocol a
>>>>> given scancode will be encoded according to.
>>>>>
>>>>
>>>> Okay, so how about "demuxing" the first protocol before handing them
>>>> to encoder callback? Simply something like below:
>>>>
>>>> -               if (handler->protocols & protocols && handler->encode) {
>>>> +               if (handler->protocols & ffs(protocols) &&
>>>> handler->encode) {
>>>>
>>>> Now the behavior is well-defined even when multiple protocols are
>>>> selected.
>>>
>>>
>>> Your patchset introduced ir_raw_encode_scancode() as well as the only
>>> two callers of that function:
>>>
>>> drivers/media/rc/nuvoton-cir.c
>>> drivers/media/rc/rc-loopback.c
>>>
>>> I realize that the sysfs wakeup_protocols file (which bakes several
>>> protocols into one label) makes defining *the* protocol difficult, but
>>> if you're going to add hacks like this, keep them to the sole driver
>>> using the API rather than the core API itself.
>>>
>>> That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a
>>> single protocol, no matter how many are passed to it (the img-ir driver
>>> already sets a precedent here so it wouldn't be an API change to change
>>> to a set of protocols which might be different than what the user
>>> suggested). (Also...yes, that'll make supporting several versions of
>>> e.g. RC6 impossible with the current sysfs code).
>>>
>>
>> I think that approach too is far from perfect as it leaves us with
>> questions such as: How do we let the user know what variant of
>> protocol the label "rc-6" really means? If in nvt we hardcode it to
>> mean RC6-0-16 and a new driver cames along which chooses
>> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations
>> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE?
>
>
> I never claimed it was perfect.
>
> For another (short-term, per-driver) solution, see the winbond-cir driver.
>

Heh, funny you should mention that... Back in late 2013/early 2014 I
submitted a patch for nuvoton which was modeled after winbond-cir. The
feedback from that could be summarized as:
 - Scancodes should be used instead of raw pulse/spaces (the initial
version of the patch worked without encoding)
 - Encoders should be generalized for others to use them too
 - Sysfs -api should be used instead of module parameters

So the suggestions were a pretty much the exact opposite of what
winbond-cir does.

>> If only there were a sysfs api to set the exact variant life would be
>> simpler...
>
>
> Yes, and your patches made it harder to get to a sane solution.
>
>>> Then change both ir_raw_encode_scancode() to take a single protocol enum
>>> and change the *encode function pointer in struct ir_raw_handler to also
>>> take a single protocol enum.
>>>
>>> That way encoders won't have to guess (using scanmasks...!?) what
>>> protocol they should encode to.
>>>
>>> And Mauro...I strongly suggest you revert all of this encoding stuff
>>> until this has been fixed...it's broken.
>>>
>>
>> Let me shortly recap the points made so far about the encoding stuff:
>>
>> * If user enables multiple wakeup_protocols then the first matching
>> one will be used to do the encoding. "First" being the module that was
>> loaded first.
>
>
> That in itself would be a good enough reason to revert the patches.
>
>> * Currently the encoders use heuristics to determine the intended
>> protocol variant from the scancode that was fed and from the length of
>> the scanmask. This causes the programmer using the encoder to not know
>> which exact variant will be used (until after the encoding is done
>> when the information can be made available if needed).
>
>
> First, "currently" implies that the heuristics can later be changed. They
> can't once this becomes part of a released kernel (not without breaking the
> kernel API).
>
> Second, how do you plan to pass the information about the chosen protocol
> back to userspace? And what is userspace supposed to do with the
> information?

I don't plan to pass that information anywhere because there is no use
case for it. Anyways that is besides the point.

> * Userspace: please set the hardware to wake up if this RC6 mode0 command is
> received
> * Kernel: sure...I've set the hardware to wake up if a RC6 mode6a command is
> received
> * Userspace: WTF?
>
> Is the next step to go buy a new remote which matches what the kernel told
> you?
>
> Third, that the scanmask is suddenly used to determine the meaning of a
> scancode is in itself an API break.
>
> Fourth, the "programmer" is not the problem. The problem is the user(space).
> A user who writes e.g. a RC6-something wakeup scancode has no way of knowing
> according to which protocol the scancode will be interpreted. Meaning that
> even if:
>
> * the user knows he has a remote control in his hand which generates RC6
> mode0 commands; and
> * he limits the wake protocols to "rc6"; and
> * he writes the correct scancode to sysfs
>
> then he still can't know that the hardware is correctly configured. And that
> might change depending on things like scanmask heuristics, module load order
> and the phase of the moon.
>
>> The way current
>> sysfs api works using heuristics was a design choice since we don't
>> have a better way to specify the protocol variant.
>>
>> Hope I didn't miss anything?
>
>
> You missed that once this goes in the API is going to be next to impossible
> to fix.
>
>> So yeah.. The code isn't "broken" in a sense that it wouldn't work.
>
>
> It's entirely broken in the sense that a user has no idea of knowing if the
> hardware has been properly configured to wake the computer or not. It's just
> as broken as the connect() system call would be if it randomly rewrote the
> destination address passed by the user, optionally connected, and most of
> the time returned zero independently of the result.
>

I think you may be misunderstanding the sysfs api or at least the
connect() analogue here as well as the userspace-kernelspace example
above are actually not how the api works. Remember that userspace does
not know about the protocol variants.
Let me try to use your example and work-out how it really goes:
* Userspace: please set the hardware to wake up if the scancode
0x800f040c is received. I know this is RC6 scancode because it came
from the rc-6 decoder but I don't know the variant (and I don't really
care)
* Kernel: Ok (btw. based on the length of the scancode and the
bit-pattern in the front I've determined this to be rc6-mce type
scancode and encoded it accordingly)
* Userspace: Got it

So no changing anything behind users back or not leading to
misconfigured hardware AFAICT.

> I can't really believe we're still debating *if* this code should stay in
> it's present form.
>

Of course we end up having a discussion when it looks like we
undestand the code/api behavior differently and my goal is obviously
to know what/how/where/when the code is "broken" to determine if it is
true and if it can be fixed :)

>> It's more a question of what we want the api to look like.
>
>
> And if the current version is part of a released kernel we can never fix it.
>
> Look, there are fundamental issues right now in rc-core in that the only way
> a scancode can have any meaning is in a protocol-scancode tuple (single
> protocol, of course) and that information is missing in many places. That's
> something I'm trying to fix (see the updated set/get keytable ioctls for
> example) and Mauro seems to mostly want to forget about. Fixing it is
> probably going to be impossible without API breaks. Your code encodes more
> of that crap right in the core of rc-core and will require even more API
> breaks.
>

I'm sorry that the encoding functionality clashes with your intentions
of improving the rc-core. I guess Mauro likes encoders more than
improving rc-core fundamentals :)
Kidding aside the fact that this series got merged might suggest that
you and Mauro don't necessarily share the same views about the future
and possible api breaks of rc-core.

Tell you what, I'll agree to reverting the series. In exchange I would
hope that you and Mauro mutually agree and let me know on:
 - What are the issues that need to be fixed in the encoding series
prefarably with how to fix them (e.g. module load order ambiquity,
whether a new api is needed, or switching to a more limited
functionality is desired like you suggested then so be it etc.)
 - When is a good chance to re-submit the series (e.g. after
ioctl/scancode/whatever api break is done or some pending series is
merged or some other core refactoring work is finished etc.)

Deal?

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On 2015-05-23 13:34, Antti Seppälä wrote:
> On 22 May 2015 at 13:33, David Härdeman <david@hardeman.nu> wrote:
>> On 2015-05-22 07:27, Antti Seppälä wrote:
>>> I think that approach too is far from perfect as it leaves us with
>>> questions such as: How do we let the user know what variant of
>>> protocol the label "rc-6" really means? If in nvt we hardcode it to
>>> mean RC6-0-16 and a new driver cames along which chooses
>>> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations
>>> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE?
>> 
>> 
>> I never claimed it was perfect.
>> 
>> For another (short-term, per-driver) solution, see the winbond-cir 
>> driver.
>> 
> 
> Heh, funny you should mention that... Back in late 2013/early 2014 I
> submitted a patch for nuvoton which was modeled after winbond-cir. The
> feedback from that could be summarized as:
>  - Scancodes should be used instead of raw pulse/spaces (the initial
> version of the patch worked without encoding)
>  - Encoders should be generalized for others to use them too
>  - Sysfs -api should be used instead of module parameters
> 
> So the suggestions were a pretty much the exact opposite of what
> winbond-cir does.

Again, it was a short-term suggestion. A long-term "real" solution 
requires a definitive identification of the intended protocol.

One idea that I've had in the back of my head for a long time is to use 
the "flags" member of "struct rc_keymap_entry" in the new 
EVIOC[GS]KEYCODE_V2 ioctl variant (see 
http://www.spinics.net/lists/linux-media/msg88452.html).

If a RC_POWERON flag was defined, it could be used for that purpose...

>> It's entirely broken in the sense that a user has no idea of knowing 
>> if the
>> hardware has been properly configured to wake the computer or not. 
>> It's just
>> as broken as the connect() system call would be if it randomly rewrote 
>> the
>> destination address passed by the user, optionally connected, and most 
>> of
>> the time returned zero independently of the result.
>> 
> 
> I think you may be misunderstanding the sysfs api or at least the
> connect() analogue here as well as the userspace-kernelspace example
> above are actually not how the api works. Remember that userspace does
> not know about the protocol variants.

Userspace most definately does. Keymaps are a good example of that. 
Distributing keymaps for a certain remote should be possible.

> Let me try to use your example and work-out how it really goes:
> * Userspace: please set the hardware to wake up if the scancode
> 0x800f040c is received. I know this is RC6 scancode because it came
> from the rc-6 decoder but I don't know the variant (and I don't really
> care)

First of all...you assume that the only way of generating a valid wakeup 
scancode is by using the same remote on the same hardware first to see 
what it generates (which - thanks to things like boneheaded decisions on 
NEC scancode generation and the previously mentioned heuristics - may 
differ). So distributing a keymap from a central repository or just 
looking up scancodes from a vendor datasheet is no longer feasible with 
this API.

Second, you have absolutely nothing that ensures that the same 
heuristics are used in the decode/encode code paths and that the 
heuristics will remain in sync.

> * Kernel: Ok (btw. based on the length of the scancode and the
> bit-pattern in the front I've determined this to be rc6-mce type
> scancode and encoded it accordingly)
> * Userspace: Got it

The whole "btw" part won't be passed to userspace...so there's nothing 
to "get"

> So no changing anything behind users back or not leading to
> misconfigured hardware AFAICT.
...
> I'm sorry that the encoding functionality clashes with your intentions
> of improving the rc-core. I guess Mauro likes encoders more than
> improving rc-core fundamentals :)
> Kidding aside the fact that this series got merged might suggest that
> you and Mauro don't necessarily share the same views about the future
> and possible api breaks of rc-core.

If you've followed the development of rc-core during the last few years 
it should be pretty clear that Mauro has little to no long-term plan, 
understanding of the current issues or willingness to fix them. I 
wouldn't read too much into the fact that the code was merged.

> Tell you what, I'll agree to reverting the series. In exchange I would
> hope that you and Mauro mutually agree and let me know on:
>  - What are the issues that need to be fixed in the encoding series
> prefarably with how to fix them (e.g. module load order ambiquity,
> whether a new api is needed, or switching to a more limited
> functionality is desired like you suggested then so be it etc.)
>  - When is a good chance to re-submit the series (e.g. after
> ioctl/scancode/whatever api break is done or some pending series is
> merged or some other core refactoring work is finished etc.)
> 
> Deal?

Mauro....wake up? I hope you're not planning to push the current code 
upstream???

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Antti Seppälä]

On 14 June 2015 at 02:44, David Härdeman <david@hardeman.nu> wrote:

> One idea that I've had in the back of my head for a long time is to use the
> "flags" member of "struct rc_keymap_entry" in the new EVIOC[GS]KEYCODE_V2
> ioctl variant (see http://www.spinics.net/lists/linux-media/msg88452.html).
>
> If a RC_POWERON flag was defined, it could be used for that purpose...
>

Ooh, that approach would indeed provide a cleaner api for setting the
wakeup scancode. I like the idea though I haven't really had the
chance to try it out.

> ...
>>
>> I'm sorry that the encoding functionality clashes with your intentions
>> of improving the rc-core. I guess Mauro likes encoders more than
>> improving rc-core fundamentals :)
>> Kidding aside the fact that this series got merged might suggest that
>> you and Mauro don't necessarily share the same views about the future
>> and possible api breaks of rc-core.
>
>
> If you've followed the development of rc-core during the last few years it
> should be pretty clear that Mauro has little to no long-term plan,
> understanding of the current issues or willingness to fix them. I wouldn't
> read too much into the fact that the code was merged.
>
>> Tell you what, I'll agree to reverting the series. In exchange I would
>> hope that you and Mauro mutually agree and let me know on:
>>  - What are the issues that need to be fixed in the encoding series
>> prefarably with how to fix them (e.g. module load order ambiquity,
>> whether a new api is needed, or switching to a more limited
>> functionality is desired like you suggested then so be it etc.)
>>  - When is a good chance to re-submit the series (e.g. after
>> ioctl/scancode/whatever api break is done or some pending series is
>> merged or some other core refactoring work is finished etc.)
>>
>> Deal?
>
>
> Mauro....wake up? I hope you're not planning to push the current code
> upstream???
>

Yeah, I'm also inclined to target for a longer term solution with this
so the current patches can be reverted.

I think I now also have enough information to go and respin the
patches to utilize the new EVIOCSKEYCODE_V2 if and when that gets
included in the rc-core.

-- 
Antti

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Mauro Carvalho Chehab]

Em Sun, 14 Jun 2015 01:44:54 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On 2015-05-23 13:34, Antti Seppälä wrote:
> > On 22 May 2015 at 13:33, David Härdeman <david@hardeman.nu> wrote:
> >> On 2015-05-22 07:27, Antti Seppälä wrote:
> >>> I think that approach too is far from perfect as it leaves us with
> >>> questions such as: How do we let the user know what variant of
> >>> protocol the label "rc-6" really means? If in nvt we hardcode it to
> >>> mean RC6-0-16 and a new driver cames along which chooses
> >>> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations
> >>> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE?
> >> 
> >> 
> >> I never claimed it was perfect.
> >> 
> >> For another (short-term, per-driver) solution, see the winbond-cir 
> >> driver.
> >> 
> > 
> > Heh, funny you should mention that... Back in late 2013/early 2014 I
> > submitted a patch for nuvoton which was modeled after winbond-cir. The
> > feedback from that could be summarized as:
> >  - Scancodes should be used instead of raw pulse/spaces (the initial
> > version of the patch worked without encoding)
> >  - Encoders should be generalized for others to use them too
> >  - Sysfs -api should be used instead of module parameters
> > 
> > So the suggestions were a pretty much the exact opposite of what
> > winbond-cir does.
> 
> Again, it was a short-term suggestion. A long-term "real" solution 
> requires a definitive identification of the intended protocol.
> 
> One idea that I've had in the back of my head for a long time is to use 
> the "flags" member of "struct rc_keymap_entry" in the new 
> EVIOC[GS]KEYCODE_V2 ioctl variant (see 
> http://www.spinics.net/lists/linux-media/msg88452.html).
> 
> If a RC_POWERON flag was defined, it could be used for that purpose...
> 
> >> It's entirely broken in the sense that a user has no idea of knowing 
> >> if the
> >> hardware has been properly configured to wake the computer or not. 
> >> It's just
> >> as broken as the connect() system call would be if it randomly rewrote 
> >> the
> >> destination address passed by the user, optionally connected, and most 
> >> of
> >> the time returned zero independently of the result.
> >> 
> > 
> > I think you may be misunderstanding the sysfs api or at least the
> > connect() analogue here as well as the userspace-kernelspace example
> > above are actually not how the api works. Remember that userspace does
> > not know about the protocol variants.
> 
> Userspace most definately does. Keymaps are a good example of that. 
> Distributing keymaps for a certain remote should be possible.
> 
> > Let me try to use your example and work-out how it really goes:
> > * Userspace: please set the hardware to wake up if the scancode
> > 0x800f040c is received. I know this is RC6 scancode because it came
> > from the rc-6 decoder but I don't know the variant (and I don't really
> > care)
> 
> First of all...you assume that the only way of generating a valid wakeup 
> scancode is by using the same remote on the same hardware first to see 
> what it generates (which - thanks to things like boneheaded decisions on 
> NEC scancode generation and the previously mentioned heuristics - may 
> differ). So distributing a keymap from a central repository or just 
> looking up scancodes from a vendor datasheet is no longer feasible with 
> this API.
> 
> Second, you have absolutely nothing that ensures that the same 
> heuristics are used in the decode/encode code paths and that the 
> heuristics will remain in sync.
> 
> > * Kernel: Ok (btw. based on the length of the scancode and the
> > bit-pattern in the front I've determined this to be rc6-mce type
> > scancode and encoded it accordingly)
> > * Userspace: Got it
> 
> The whole "btw" part won't be passed to userspace...so there's nothing 
> to "get"
> 
> > So no changing anything behind users back or not leading to
> > misconfigured hardware AFAICT.
> ...
> > I'm sorry that the encoding functionality clashes with your intentions
> > of improving the rc-core. I guess Mauro likes encoders more than
> > improving rc-core fundamentals :)
> > Kidding aside the fact that this series got merged might suggest that
> > you and Mauro don't necessarily share the same views about the future
> > and possible api breaks of rc-core.
> 
> If you've followed the development of rc-core during the last few years 
> it should be pretty clear that Mauro has little to no long-term plan, 
> understanding of the current issues or willingness to fix them. I 
> wouldn't read too much into the fact that the code was merged.

You completely missed the point. Adding new drivers and new features
don't require much time to review, and don't require testing.

What you're trying to send as "fixes" is actually series of patches that
change the behavior of the subsystem, with would cause regressions.

Btw, a lot of the pull requests you've sent over the past years did cause
regressions. So, I can only review/apply them when I have a bunch of
spare time to test them. As I don't usually have a bunch of spare time,
nor we have a sub-maintainer for remote controllers that would have
time for such tests, they're delayed.

> > Tell you what, I'll agree to reverting the series. In exchange I would
> > hope that you and Mauro mutually agree and let me know on:
> >  - What are the issues that need to be fixed in the encoding series
> > prefarably with how to fix them (e.g. module load order ambiquity,
> > whether a new api is needed, or switching to a more limited
> > functionality is desired like you suggested then so be it etc.)
> >  - When is a good chance to re-submit the series (e.g. after
> > ioctl/scancode/whatever api break is done or some pending series is
> > merged or some other core refactoring work is finished etc.)
> > 
> > Deal?
> 
> Mauro....wake up? I hope you're not planning to push the current code 
> upstream???

What's there are planned to be sent upstream. If you think that something
is not mature enough to be applied, please send a patch reverting it,
with "[PATCH FIXES]" in the subject, clearly explaining why it should be
reverted for me to analyze. Having Antti/James acks on that would help.

Regards,
Mauro

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On 2015-06-18 23:23, Mauro Carvalho Chehab wrote:
> Em Sun, 14 Jun 2015 01:44:54 +0200
> David Härdeman <david@hardeman.nu> escreveu:
>> If you've followed the development of rc-core during the last few 
>> years
>> it should be pretty clear that Mauro has little to no long-term plan,
>> understanding of the current issues or willingness to fix them. I
>> wouldn't read too much into the fact that the code was merged.
> 
> You completely missed the point.

Of course...

> Adding new drivers and new features
> don't require much time to review, and don't require testing.

But a focus on adding new "features" (some of which further cement bad 
API) is dangerous when the foundations still need work.

> What you're trying to send as "fixes" is actually series of patches 
> that
> change the behavior of the subsystem, with would cause regressions.

Some things can't be fixed without changing some behavior...assuming 
you're not talking about the patches that add a rc-core chardev...that 
is indeed a whole new direction and I can completely take onboard that 
you'd like to see a better RFC discussion with a document describing the 
interface, changes, rationale, etc....and I'd be happy to produce a 
document like that when I have the time (I'm guessing the LinuxTV wiki 
might be a good place).

I have a total of six patches in my queue that are not related to the 
rc-core chardev:

One fixes a uevent bug and should be trivial
One converts rc-core to use an IDA, a cleanup which seems to fix a race 
as well
One removes lirc as a "protocol" and is not an API change as you thought
One prepares the lmedm04 driver for the next two patches
The last two adds protocol info to the EVIOC[GS]KEYCODE_V2 ioctl

The last two patches need the most careful scrutiny, but they are an 
attempt to finally fix a serious issue. I've already indicated that they 
are not 100% backwards compatible, but the corner cases they won't catch 
(can't catch) are pretty extreme. I'd be happy to discuss them further 
if you'd like.

I have no plans to move on to the rc-core chardev discussion before the 
above patches have been dealt with. I don't think it's a good use of 
anyone's time.

> Btw, a lot of the pull requests you've sent over the past years did 
> cause
> regressions.

Yes, trying to change/fix parts of the foundation of the rc-core code 
definitely carries a larger risk of regressions (especially when I don't 
even have the hardware). That's not a good reason to not try though.

> So, I can only review/apply them when I have a bunch of
> spare time to test them. As I don't usually have a bunch of spare time,
> nor we have a sub-maintainer for remote controllers that would have
> time for such tests, they're delayed.

I think we're getting off-topic.

>> Mauro....wake up? I hope you're not planning to push the current code
>> upstream???
> 
> What's there are planned to be sent upstream. If you think that 
> something
> is not mature enough to be applied, please send a patch reverting it,
> with "[PATCH FIXES]" in the subject, clearly explaining why it should 
> be
> reverted for me to analyze. Having Antti/James acks on that would help.

This thread should already provide you with all the information you need 
why the patches should be reverted (including Antii saying the patches 
should be reverted).

The current code includes hilarious "features" like producing different 
results depending on module load order and makes sure we'll be stuck 
with a bad API. Sending them upstream will look quite foolish...

Regards,
David

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On Tue, Jun 23, 2015 at 10:45:42PM +0200, David Härdeman wrote:
>On 2015-06-18 23:23, Mauro Carvalho Chehab wrote:
>>Em Sun, 14 Jun 2015 01:44:54 +0200
>>David Härdeman <david@hardeman.nu> escreveu:
>>>Mauro....wake up? I hope you're not planning to push the current code
>>>upstream???
>>
>>What's there are planned to be sent upstream. If you think that something
>>is not mature enough to be applied, please send a patch reverting it,
>>with "[PATCH FIXES]" in the subject, clearly explaining why it should be
>>reverted for me to analyze. Having Antti/James acks on that would help.
>
>This thread should already provide you with all the information you need why
>the patches should be reverted (including Antii saying the patches should be
>reverted).
>
>The current code includes hilarious "features" like producing different
>results depending on module load order and makes sure we'll be stuck with a
>bad API. Sending them upstream will look quite foolish...

And now the patches have been submitted and comitted upstream. What's
your plan? Leave it like this?

-- 
David Härdeman

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[David Härdeman]

On Mon, Jun 29, 2015 at 09:05:24PM +0200, David Härdeman wrote:
>On Tue, Jun 23, 2015 at 10:45:42PM +0200, David Härdeman wrote:
>>On 2015-06-18 23:23, Mauro Carvalho Chehab wrote:
>>>Em Sun, 14 Jun 2015 01:44:54 +0200
>>>David Härdeman <david@hardeman.nu> escreveu:
>>>>Mauro....wake up? I hope you're not planning to push the current code
>>>>upstream???
>>>
>>>What's there are planned to be sent upstream. If you think that something
>>>is not mature enough to be applied, please send a patch reverting it,
>>>with "[PATCH FIXES]" in the subject, clearly explaining why it should be
>>>reverted for me to analyze. Having Antti/James acks on that would help.
>>
>>This thread should already provide you with all the information you need why
>>the patches should be reverted (including Antii saying the patches should be
>>reverted).
>>
>>The current code includes hilarious "features" like producing different
>>results depending on module load order and makes sure we'll be stuck with a
>>bad API. Sending them upstream will look quite foolish...
>
>And now the patches have been submitted and comitted upstream. What's
>your plan? Leave it like this?

Mauro, I see that you've applied four of my patches...thanks for
that...but the question is still what you plan to do about the patches
that should be reverted....4.2-rc2 was recently released and I'm still
not seeing any action on this while time is running out...?

-- 
David Härdeman

Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback

[Mauro Carvalho Chehab]

Em Mon, 13 Jul 2015 19:47:50 +0200
David Härdeman <david@hardeman.nu> escreveu:

> On Mon, Jun 29, 2015 at 09:05:24PM +0200, David Härdeman wrote:
> >On Tue, Jun 23, 2015 at 10:45:42PM +0200, David Härdeman wrote:
> >>On 2015-06-18 23:23, Mauro Carvalho Chehab wrote:
> >>>Em Sun, 14 Jun 2015 01:44:54 +0200
> >>>David Härdeman <david@hardeman.nu> escreveu:
> >>>>Mauro....wake up? I hope you're not planning to push the current code
> >>>>upstream???
> >>>
> >>>What's there are planned to be sent upstream. If you think that something
> >>>is not mature enough to be applied, please send a patch reverting it,
> >>>with "[PATCH FIXES]" in the subject, clearly explaining why it should be
> >>>reverted for me to analyze. Having Antti/James acks on that would help.
> >>
> >>This thread should already provide you with all the information you need why
> >>the patches should be reverted (including Antii saying the patches should be
> >>reverted).
> >>
> >>The current code includes hilarious "features" like producing different
> >>results depending on module load order and makes sure we'll be stuck with a
> >>bad API. Sending them upstream will look quite foolish...
> >
> >And now the patches have been submitted and comitted upstream. What's
> >your plan? Leave it like this?
> 
> Mauro, I see that you've applied four of my patches...thanks for
> that...but the question is still what you plan to do about the patches
> that should be reverted....4.2-rc2 was recently released and I'm still
> not seeing any action on this while time is running out...?

What patches? I'm not seeing any such revert patches on my queue.
At least patchwork doesn't show any pending patches from you:
	https://patchwork.linuxtv.org/project/linux-media/list/?submitter=96

Nor from James:
	https://patchwork.linuxtv.org/project/linux-media/list/?submitter=1270

Nor from Antti:
	https://patchwork.linuxtv.org/project/linux-media/list/?submitter=650

Are you sure you submitted any reverse patch? If so, please point
the patches you sent for me to review/apply.

Thanks,
Mauro