Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering

[James Hogan <james@albanarts.com>]

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]

On Sunday 16 March 2014 10:22:02 Antti Seppälä wrote:
> On 15 March 2014 01:04, James Hogan <james@albanarts.com> wrote:
> > A recent discussion about proposed interfaces for setting up the
> > hardware wakeup filter lead to the conclusion that it could help to have
> > the generic capability to encode and modulate scancodes into raw IR
> > events so that drivers for hardware with a low level wake filter (on the
> > level of pulse/space durations) can still easily implement the higher
> > level scancode interface that is proposed.
> > 
> > I posted an RFC patchset showing how this could work, and Antti Seppälä
> > posted additional patches to support rc5-sz and nuvoton-cir. This
> > patchset improves the original RFC patches and combines & updates
> > Antti's patches.
> > 
> > I'm happy these patches are a good start at tackling the problem, as
> > long as Antti is happy with them and they work for him of course.
> > 
> > Future work could include:
> >  - Encoders for more protocols.
> >  - Carrier signal events (no use unless a driver makes use of it).
> > 
> > Patch 1 adds the new encode API.
> > Patches 2-3 adds some modulation helpers.
> > Patches 4-6 adds some raw encode implementations.
> > Patch 7 adds some rc-core support for encode based wakeup filtering.
> > Patch 8 adds debug loopback of encoded scancode when filter set.
> > Patch 9 (untested) adds encode based wakeup filtering to nuvoton-cir.
> 
> Hi James.
> 
> This is looking very good. I've reviewed the series and have only
> minor comments to some of the patches which I'll post individually
> shortly.
> 
> I've also tested the nuvoton with actual hardware with rc-5-sz and nec
> encoders and both generate wakeup samples correctly and can wake the
> system.

Thanks for reviewing and testing!

> While doing my tests I also noticed that there is a small bug in the
> wakeup_protocols handling where one can enable multiple protocols with
> the + -notation. E.g. echo "+nec +rc-5" >
> /sys/class/rc/rc0/wakeup_protocols shouldn't probably succeed.

Yeh I'm in two minds about this now. It's actually a little awkward since some 
of the protocols have multiple variants (i.e. "rc-5" = RC5+RC5X), but an 
encoded message is only ever a single variant, so technically if you're going 
to draw the line for wakeup protocols it should probably be at one enabled 
variant, which isn't always convenient or necessary.

Note, ATM even disallowing "+proto" and "-proto" we would already have to 
guess which variant is desired from the scancode data, which in the case of 
NEC scancodes is a bit horrid since NEC scancodes are ambiguous. This actually 
means it's driver specific whether a filter mask of 0x0000ffff filters out 
NEC32/NEC-X messages (scancode/encode driver probably will since it needs to 
pick a variant, but software fallback won't).

Ideally there'd now be a way to specify the protocol variants exactly as well 
as whole protocols groups through this sysfs interface (and probably NEC 
should have protocol bits for each variant too, which is tricky ATM since 
keymaps can use scancodes of multiple variants and it's hard to guarantee 
which variant was intended for each key mapping by reading it).

Adding proto_names entries for each variant is easy enough, though I'm not 
sure what you'd call the one for RC_BIT_RC5 (since "rc-5" is taken to mean 
both RC5 and RC5X). We could then check that the enabled wakeup protocols fit 
entirely within one of the proto_names entry's proto mask if we think it's 
helpful (which would allow you to specify e.g. "sony12", or "sony" (sony 
12,15,20), or "sony -sony12" (sony 15,20), but not "+sony +nec").

Cheers
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]