On 2015-05-21 09:53, Antti Seppälä wrote:
On 20 May 2015 at 23:45, David Härdeman <david@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:
On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote:
From: James Hogan <james@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Antti Seppälä <a.seppala@xxxxxxxxx>
Cc: David Härdeman <david@xxxxxxxxxxx>
---
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).
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
