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
JamesAttachment:
signature.asc
Description: This is a digitally signed message part.
