From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from vader.hardeman.nu ([95.142.160.32]:52452 "EHLO hardeman.nu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751629AbbEUJOY (ORCPT ); Thu, 21 May 2015 05:14:24 -0400 To: =?UTF-8?Q?Antti_Sepp=C3=A4l=C3=A4?= Subject: Re: [PATCH v3 1/7] rc: rc-ir-raw: Add scancode encoder callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Thu, 21 May 2015 11:14:22 +0200 From: =?UTF-8?Q?David_H=C3=A4rdeman?= Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab , James Hogan In-Reply-To: References: <1427824092-23163-1-git-send-email-a.seppala@gmail.com> <1427824092-23163-2-git-send-email-a.seppala@gmail.com> <20150519203851.GC18036@hardeman.nu> <20150520182901.GB13624@hardeman.nu> <20150520204557.GB15223@hardeman.nu> Message-ID: <32cae92aa099067315d1a13c7302957f@hardeman.nu> Sender: linux-media-owner@vger.kernel.org List-ID: On 2015-05-21 09:53, Antti Seppälä wrote: > On 20 May 2015 at 23:45, David Härdeman 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 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 wrote: >>>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>>>>> From: James Hogan >>>>>>> >>>>>>> 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 >>>>>>> Signed-off-by: Antti Seppälä >>>>>>> Cc: David Härdeman >>>>>>> --- >>>>>>> >>>>>>> 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//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).