Re: [PATCH v2 6/9] rc: ir-rc5-sz-decoder: Add ir encoding support

[James Hogan <james@albanarts.com>]

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

Hi Antti,

On Sunday 16 March 2014 10:34:31 Antti Seppälä wrote:
> > +/**
> > + * ir_rc5_sz_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_sz_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;
> > +
> > +       /* all important bits of scancode should be set in mask */
> > +       if (~scancode->mask & 0x2fff)
> 
> Do we want to be so restrictive here? In my opinion it's quite nice to
> be able to encode also the toggle bit if needed. Therefore a check
> against 0x3fff would be a better choice.
> 
> I think the ability to encode toggle bit might also be nice to have
> for rc-5(x) also.
> 

I don't believe the toggle bit is encoded in the scancode though, so I'm not 
sure it makes sense to treat it like that. I'm not an expert on RC-5 like 
protocols or the use of the toggle bit though.

> > +               return -EINVAL;
> > +       /* extra bits in mask should be zero in data */
> > +       if (scancode->mask & scancode->data & ~0x2fff)
> > +               return -EINVAL;
> > +
> > +       /* 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);
> 
> I'm not sure that the & 0x2fff is necessary. It has the ill effect of
> eventually writing something else to hardware while still committing
> the filter as unmodified original value. This will result in
> difference between what sysfs states was written when read back and
> what was actually written.
> 
> I think checks above are good enough to restrict the values and as
> little modification as possible of the data should be done after that.

I suspect it was the toggle bit I was thinking about, e.g.
echo 0x3fff > wakeup_filter
echo 0x2fff > wakeup_filter_mask

I had assumed shouldn't be able to affect the toggle bit (it's not set in 
mask).

Cheers
James

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