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?
>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
--
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
