On Mon, 2012-09-17 at 12:04 +0200, Antonio Ospite wrote:
> On Thu, 13 Sep 2012 23:04:52 -0400
> David Dillow <dave@xxxxxxxxxxxxxx> wrote:
> > Antonio, if you could test this with the real remote you have, I'd be
> > very appreciative.
> >
>
> Works here as well with original PS3 BR remote, thanks David D.
>
> About multiple key-presses, when I hold two _different_ keys at the same
> time, I get the message:
>
> ps3_remote 0005:054C:0306.0004: Incoherent report, 000000 000000 ff 16
> 01
>
> I just tried a couple of combinations tho.
I found a typo in the custom report descriptor which could cause this --
where it says REPORT COUNT (11), I have 0x95, 0x01 as values in the
array. Those should be 0x95, 0x0b instead. This would prevent it from
looking at any more than the first key reported. Could you fix that up
and retest?
Looking at this gave me an idea that would avoid needing a raw event
handler; I've got a version built and will test in a bit, once the kids
are down. I'll send it along after that.
If both fail, it would be useful to see the output
from /sys/kernel/debug/hid/0005:*:0306:*/event while you are pressing
both single and multiple buttons.
> What about adding a note about the association maneuver? I mean the one
> when we press Start+Enter for a few second when we add the BD remote as
> a new BT device. I forget about it every time I associate the BD
> remote with a new system; I know kernel code is not the most
> user-friendly spot for this documentation but having it here wouldn't
> hurt.
>
> Now I noted it on the back of the battery cover too :)
Indeed; I don't have one of the real remotes -- could you provide text?
> > +static int ps3remote_event(struct hid_device *hdev, struct hid_report *report,
> > + u8 *raw_data, int size)
> > +{
> > + struct ps3remote_data *data = hid_get_drvdata(hdev);
> > + u32 mask, changed;
> > + u8 key, pressed;
>
> The preferred kernel style for declarations is to have one per line, no
> commas, but that's up to you.
I've seen it both ways, so I'll stick with the less vertical space. If
they were initialized here, I'd be more inclined to change.
> > + if (size != 12) {
> > + hid_dbg(hdev, "unsupported report size\n");
> > + return 1;
>
> The raw_event callback should return negative on error, is there a
> reason why 1 is returned here?
I was just eating the event rather than passing it on. It probably makes
more sense to return an error, but I didn't investigate how the rest of
the stack handled that.
I'll likely change this if I need to keep the raw event handler.
Thank you for the review, I've incorporated your other comments into the
driver before I started working on the simpler version.
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
