HI Antonio,
On Mon, Sep 17, 2012 at 1:04 PM, Antonio Ospite
<ospite@xxxxxxxxxxxxxxxxx> wrote:
> On Thu, 13 Sep 2012 23:04:52 -0400
> David Dillow <dave@xxxxxxxxxxxxxx> wrote:
>
> I'd use the official name "Sony PS3 BD Remote Control" in the subject
> repeating it as "Sony PS3 Blue-ray Disc Remote Control" in the long
> commit message to increase the googlable surface a little bit.
>
>> We also gain support for the Logitech Harmony Adapter for PS3.
>>
>> Signed-off-by: David Dillow <dave@xxxxxxxxxxxxxx>
>> --
>
> When you submit the patch to linux-input for inclusion remember to send
> one formatted with git-format-patch, for instance the diffstat
> separator is '---' not '--', with the latter in your last message the
> diffstat and the annotations are taken as part of the commit message
> when using git-am, while the annotations _after_ the diffstat are meant
> for discussion and to be ignored by git-am.
>
> Other comments inlined below.
>
>> drivers/hid/Kconfig | 7 +
>> drivers/hid/Makefile | 1 +
>> drivers/hid/hid-core.c | 2 +
>> drivers/hid/hid-ps3remote.c | 325 +++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 335 insertions(+), 0 deletions(-)
>>
>> On Thu, 2012-09-13 at 00:36 +0200, Antonio Ospite wrote:
>> On Wed, 12 Sep 2012 22:26:04 +0200 David Herrmann wrote:
>> > > So if the devices only need some short setup-command during
>> > > initialization and then they send HID reports whenever a button is
>> > > pressed, then everything should be very easy to get working. Even
>> > > auto-reconnect and 10min idle-disconnect are _very_ easy to get
>> > > working in the kernel with the current HID infrastructure.
>> >
>> > I too will take a look during the week-end about writing a kernel
>> > driver, if it's not that much of an effort I might take the project
>> > (no promises yet). I have a PS3 BD remote Bastien donated.
>>
>> Here's a first pass at a kernel driver to support the BD remote and
>> Harmony PS adapter. I've tested it on Fedora 16 with single keypresses
>> only, as I cannot send multiple simultaneous keypresses with the
>> Harmony. This patch is against kernel.org's bluetooth.git as of an hour
>> ago, but should apply to other versions without much fuss.
>>
>> 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.
>
>> David, could you point me at the controls for the 10min idle-disconnect?
>> auto-connect and idle-disconnect seem to be working on my Fedora 16 box,
>> though it looks like the Harmony adapter is initiating the disconnect --
>> this is likely an area for improvement with the Sony remote.
>>
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index fbf4950..7e6ab25 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -534,6 +534,13 @@ config HID_PRIMAX
>> Support for Primax devices that are not fully compliant with the
>> HID standard.
>>
>> +config HID_PS3REMOTE
>> + tristate "Sony PS3 Bluetooth BD Remote"
>
> Use "Sony PS3 BD Remote Control" here too and mention Bluetooth in the
> help section.
>
>> + depends on BT_HIDP
>> + ---help---
>> + Support for Sony PS3 Bluetooth BD Remote and Logitech Harmony Adapter
>> + for PS3.
>> +
>
> From scripts/checkpatch.pl:
> WARNING: please write a paragraph that describes the config symbol fully
> #60: FILE: drivers/hid/Kconfig:537:
> +config HID_PS3REMOTE
>
> This is because the description is too short (< 4 lines), but I think
> we can ignore this warning.
>
>> config HID_ROCCAT
>> tristate "Roccat device support"
>> depends on USB_HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index f975485..333ed6c 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -70,6 +70,7 @@ obj-$(CONFIG_HID_PANTHERLORD) += hid-pl.o
>> obj-$(CONFIG_HID_PETALYNX) += hid-petalynx.o
>> obj-$(CONFIG_HID_PICOLCD) += hid-picolcd.o
>> obj-$(CONFIG_HID_PRIMAX) += hid-primax.o
>> +obj-$(CONFIG_HID_PS3REMOTE) += hid-ps3remote.o
>> obj-$(CONFIG_HID_ROCCAT) += hid-roccat.o hid-roccat-common.o \
>> hid-roccat-arvo.o hid-roccat-isku.o hid-roccat-kone.o \
>> hid-roccat-koneplus.o hid-roccat-kovaplus.o hid-roccat-pyra.o \
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 60ea284..1838c12 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1591,6 +1591,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) },
>> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0306) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
>> @@ -1641,6 +1642,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
>> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
>> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, 0x0306) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_SUNPLUS, USB_DEVICE_ID_SUNPLUS_WDESKTOP) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb300) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb304) },
>
> As David H. already said, stick with the style of the subsystem for
> magic numbers, HID tend to prefer symbolic constants.
>
>> diff --git a/drivers/hid/hid-ps3remote.c b/drivers/hid/hid-ps3remote.c
>> new file mode 100644
>> index 0000000..a87346e
>> --- /dev/null
>> +++ b/drivers/hid/hid-ps3remote.c
>> @@ -0,0 +1,325 @@
>> +/*
>> + * HID driver for Sony PS3 BD Remote
>> + *
>> + * Copyright (c) 2012 David Dillow <dave@xxxxxxxxxxxxxx>
>> + * Based on a blend of the bluez fakehid user-space code by Marcel Holtmann
>> + * and other kernel HID drivers.
>> + */
>> +
>> +/*
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
>> +
>
> 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 :)
>
>> +#include <linux/device.h>
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +enum ps3remote_special_keys {
>> + PS3R_BIT_PS = 0,
>> + PS3R_BIT_ENTER = 3,
>> + PS3R_BIT_L2 = 8,
>> + PS3R_BIT_R2 = 9,
>> + PS3R_BIT_L1 = 10,
>> + PS3R_BIT_R1 = 11,
>> + PS3R_BIT_TRIANGLE = 12,
>> + PS3R_BIT_CIRCLE = 13,
>> + PS3R_BIT_CROSS = 14,
>> + PS3R_BIT_SQUARE = 15,
>> + PS3R_BIT_SELECT = 16,
>> + PS3R_BIT_L3 = 17,
>> + PS3R_BIT_R3 = 18,
>> + PS3R_BIT_START = 19,
>> + PS3R_BIT_UP = 20,
>> + PS3R_BIT_RIGHT = 21,
>> + PS3R_BIT_DOWN = 22,
>> + PS3R_BIT_LEFT = 23,
>> +};
>> +
>> +static unsigned int ps3remote_bits[] = {
>
> This could be const too.
>
>> + [PS3R_BIT_ENTER] = 0x0b,
>> + [PS3R_BIT_PS] = 0x43,
>> + [PS3R_BIT_SQUARE] = 0x5f,
>> + [PS3R_BIT_CROSS] = 0x5e,
>> + [PS3R_BIT_CIRCLE] = 0x5d,
>> + [PS3R_BIT_TRIANGLE] = 0x5c,
>> + [PS3R_BIT_R1] = 0x5b,
>> + [PS3R_BIT_L1] = 0x5a,
>> + [PS3R_BIT_R2] = 0x59,
>> + [PS3R_BIT_L2] = 0x58,
>> + [PS3R_BIT_LEFT] = 0x57,
>> + [PS3R_BIT_DOWN] = 0x56,
>> + [PS3R_BIT_RIGHT] = 0x55,
>> + [PS3R_BIT_UP] = 0x54,
>> + [PS3R_BIT_START] = 0x53,
>> + [PS3R_BIT_R3] = 0x52,
>> + [PS3R_BIT_L3] = 0x51,
>> + [PS3R_BIT_SELECT] = 0x50,
>> +};
>> +
>> +const static unsigned int ps3remote_keymap[] = {
>
> From scripts/checkpatch.pl:
>
> WARNING: storage class should be at the beginning of the declaration
> #171: FILE: drivers/hid/hid-ps3remote.c:64:
> +const static unsigned int ps3remote_keymap[] = {
>
>> + [0x16] = KEY_EJECTCD,
>> + [0x64] = KEY_AUDIO,
>> + [0x65] = KEY_ANGLE,
>> + [0x63] = KEY_SUBTITLE,
>> + [0x0f] = KEY_CLEAR,
>> + [0x28] = KEY_TIME,
>> + [0x00] = KEY_1,
>> + [0x01] = KEY_2,
>> + [0x02] = KEY_3,
>> + [0x03] = KEY_4,
>> + [0x04] = KEY_5,
>> + [0x05] = KEY_6,
>> + [0x06] = KEY_7,
>> + [0x07] = KEY_8,
>> + [0x08] = KEY_9,
>> + [0x09] = KEY_0,
>> + [0x81] = KEY_RED,
>> + [0x82] = KEY_GREEN,
>> + [0x80] = KEY_BLUE,
>> + [0x83] = KEY_YELLOW,
>> + [0x70] = KEY_INFO, /* display */
>> + [0x1a] = KEY_MENU, /* top menu */
>> + [0x40] = KEY_CONTEXT_MENU, /* pop up/menu */
>> + [0x0e] = KEY_ESC, /* return */
>> + [0x5c] = KEY_OPTION, /* options/triangle */
>> + [0x5d] = KEY_BACK, /* back/circle */
>> + [0x5f] = KEY_SCREEN, /* view/square */
>> + [0x5e] = BTN_0, /* cross */
>> + [0x54] = KEY_UP,
>> + [0x56] = KEY_DOWN,
>> + [0x57] = KEY_LEFT,
>> + [0x55] = KEY_RIGHT,
>> + [0x0b] = KEY_ENTER,
>> + [0x5a] = BTN_TL, /* L1 */
>> + [0x58] = BTN_TL2, /* L2 */
>> + [0x51] = BTN_THUMBL, /* L3 */
>> + [0x5b] = BTN_TR, /* R1 */
>> + [0x59] = BTN_TR2, /* R2 */
>> + [0x52] = BTN_THUMBR, /* R3 */
>> + [0x43] = KEY_HOMEPAGE, /* PS button */
>> + [0x50] = KEY_SELECT,
>> + [0x53] = BTN_START,
>> + [0x33] = KEY_REWIND, /* scan back */
>> + [0x32] = KEY_PLAY,
>> + [0x34] = KEY_FORWARD, /* scan forward */
>> + [0x30] = KEY_PREVIOUS,
>> + [0x38] = KEY_STOP,
>> + [0x31] = KEY_NEXT,
>> + [0x60] = KEY_FRAMEBACK, /* slow/step back */
>> + [0x39] = KEY_PAUSE,
>> + [0x61] = KEY_FRAMEFORWARD, /* slow/step forward */
>> +};
>> +
>> +static __u8 ps3remote_rdesc[] = {
>> + 0x05, 0x01, /* USAGE PAGE (Generic Desktop) */
>> + 0x09, 0x05, /* USAGE (Game Pad) */
>> + 0xa1, 0x01, /* COLLECTION (Application) */
>> + 0x05, 0x06, /* USAGE PAGE (Generic Device) */
>> + 0x09, 0x20, /* USAGE (Battery Strength) */
>> + 0x15, 0x00, /* LOGICAL MINIMUM (0) */
>> + 0x25, 0x64, /* LOGICAL MAXIMUM (100) */
>> + 0x75, 0x08, /* REPORT SIZE (8) */
>> + 0x95, 0x01, /* REPORT COUNT (1) */
>> + 0x81, 0x02, /* INPUT (Variable, Absolute) */
>> + 0x05, 0x09, /* USAGE PAGE (Button) */
>> + 0x19, 0x00, /* USAGE MINUMUM (0) */
>> + 0x29, 0xfe, /* USAGE MAXIMUM (254) */
>> + 0x15, 0x00, /* LOGICAL MINIMUM (0) */
>> + 0x25, 0xfe, /* LOGICAL MAXIMUM (254) */
>> + 0x75, 0x08, /* REPORT SIZE (8) */
>> + 0x95, 0x01, /* REPORT COUNT (11) */
>> + 0x81, 0x00, /* INPUT (Array, Absolute) */
>> + 0xc0, /* END_COLLECTION */
>> +};
>> +
>> +struct ps3remote_data {
>> + u8 report[12];
>
> Do we want to define a symbolic const for the report size and use that
> when handling data->report below? Just a suggestion tho, I don't have a
> strong opinion about that.
>
>> + u8 last_key;
>> + u32 last_mask;
>> +};
>> +
>> +static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
>> + unsigned int *rsize)
>> +{
>> + *rsize = sizeof(ps3remote_rdesc);
>> + return ps3remote_rdesc;
>> +}
>> +
>> +static int ps3remote_mapping(struct hid_device *hdev, struct hid_input *hi,
>> + struct hid_field *field, struct hid_usage *usage,
>> + unsigned long **bit, int *max)
>> +{
>> + unsigned int key = usage->hid & HID_USAGE;
>> +
>> + if ((usage->hid & HID_USAGE_PAGE) != HID_UP_BUTTON ||
>> + key >= ARRAY_SIZE(ps3remote_keymap))
>> + return -1;
>> +
>> + key = ps3remote_keymap[key];
>> + if (!key)
>> + return -1;
>> +
>> + hid_map_usage_clear(hi, usage, bit, max, EV_KEY, key);
>> + return 1;
>> +}
>> +
>> +static void ps3remote_keychange(struct ps3remote_data *data, u8 key, u8 pressed)
>> +{
>> + /*
>> + * Update our report to include/exclude the key that changed, but
>> + * make sure it doesn't get listed twice. It's OK if we don't have
>> + * room to store a keypress, as that means 11 keys are simultaneously
>> + * pressed.
>> + */
>> + u8 *p = memchr(data->report + 1, key, 11);
>> + if (pressed) {
>> + if (p)
>> + return;
>> +
>> + /* Not present, find a free spot to add it */
>> + p = memchr(data->report + 1, 0xff, 11);
>> + } else
>> + key = 0xff;
>
> In Documentation/CodingStyle is suggested to use braces here in the
> 'else' block as well if the 'if' branch has more then one instruction.
>
>> +
>> + if (p)
>> + *p = key;
>> +}
>> +
>> +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.
>
>> + int i;
>> +
>> + 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?
>
>> + }
>> +
>> + /* Battery appears to range from 0 to 5, convert into a percentage */
>> + data->report[0] = raw_data[11] * 20;
>> +
>
> What about writing (100 / 5) explicitly? The compiler is going to
> optimize that anyway but you _show_ that you are converting to a
> percentage beside _telling_ that in the comment. Defining some
> MAX_BATTERY constant maybe is overkill here, but I won't object if you
> do it :)
>
>> + /*
>> + * Sometimes key presses are reported in the bitmask, sometimes
>> + * standalone. It appears the bitmask is used for buttons that
>> + * may pressed at the same time.
>
> I think it should be "may be pressed" here: missing "be".
>
>> + */
>> + mask = be32_to_cpup((__be32 *) raw_data);
>> + mask &= ~0xff000000;
>> + key = raw_data[4];
>> + pressed = raw_data[10];
>> +
>> + changed = data->last_mask ^ mask;
>> + if (changed) {
>> + /* Update any changed multiple keypress reports */
>> + for (i = 0; i < 24; i++) {
>
> Consider replacing the 24 here with ARRAY_SIZE(ps3remote_bits).
>
>> + if ((changed & (1 << i)) && ps3remote_bits[i]) {
>> + ps3remote_keychange(data, ps3remote_bits[i],
>> + !!(mask & (1 << i)));
>> + }
>> + }
>> + data->last_mask = mask;
>> + } else if (pressed && key != 0xff) {
>> + /* Single key pressed */
>> + ps3remote_keychange(data, key, 1);
>> + data->last_key = key;
>> + } else if (!pressed && data->last_key != 0xff) {
>> + /* Single key released */
>> + ps3remote_keychange(data, data->last_key, 0);
>> + data->last_key = 0xff;
>> + } else {
>> + hid_dbg(hdev, "Incoherent report, %06x %06x %02x %02x %02x\n",
>> + mask, data->last_mask, key, data->last_key, pressed);
>> + }
>> +
>> + /* Substitue our updated report */
>
> Typo: "Substitute".
>
>> + memcpy(raw_data, data->report, 12);
>
> The 12 here maybe can be written as a sizeof.
>
>> + return 0;
>> +}
>> +
>> +static int ps3remote_probe(struct hid_device *hdev,
>> + const struct hid_device_id *id)
>> +{
>> + struct ps3remote_data *data;
>> + int ret = -ENOMEM;
>> +
>> + data = kmalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data) {
>> + hid_err(hdev, "Can't alloc private data\n");
>> + goto err;
>> + }
>> +
>> + data->report[0] = 0;
>> + memset(data->report + 1, 0xff, 11);
>> + data->last_key = 0xff;
>> + data->last_mask = 0;
>> +
>> + hid_set_drvdata(hdev, data);
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "HID parse failed\n");
>> + goto err;
>> + }
>> +
>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> + if (ret) {
>> + hid_err(hdev, "HW start failed\n");
>> + goto err;
>> + }
>> +
>> + return ret;
>> +
>> +err:
>> + kfree(data);
>> + return ret;
>> +}
>> +
>> +static void ps3remote_remove(struct hid_device *hdev)
>> +{
>> + struct ps3remote_data *data = hid_get_drvdata(hdev);
>> +
>> + hid_hw_stop(hdev);
>> + kfree(data);
>> +}
>> +
>> +static const struct hid_device_id ps3remote_devices[] = {
>> + /* PS3 BD Remote */
>> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, 0x0306) },
>> + /* Logitech Harmony Adapter for PS3 */
>> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0306) },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, ps3remote_devices);
>> +
>> +static struct hid_driver ps3remote_driver = {
>> + .name = "ps3_remote",
>
> From scripts/checkpatch.pl:
>
> WARNING: please, no space before tabs
> #410: FILE: drivers/hid/hid-ps3remote.c:303:
> +^I.name ^I^I= "ps3_remote",$
>
> I'd use spaces for aligning the '=' signs here.
> My rule of thumb is "TABs for indentation, spaces for alignment".
>
>> + .id_table = ps3remote_devices,
>> + .probe = ps3remote_probe,
>> + .remove = ps3remote_remove,
>> + .report_fixup = ps3remote_fixup,
>> + .input_mapping = ps3remote_mapping,
>> + .raw_event = ps3remote_event,
>> +};
>> +
>> +static int __init ps3remote_init(void)
>> +{
>> + return hid_register_driver(&ps3remote_driver);
>> +}
>> +
>> +static void __exit ps3remote_exit(void)
>> +{
>> + hid_unregister_driver(&ps3remote_driver);
>> +}
>> +
>> +module_init(ps3remote_init);
>> +module_exit(ps3remote_exit);
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("David Dillow <dave@xxxxxxxxxxxxxx>");
>>
>
> Thanks for working on that.
>
> Reviewed-by: Antonio Ospite <ospite@xxxxxxxxxxxxxxxxx>
>
> I just did a syntax/style review, maybe I will take another look at the
> actual logic in the future to see if the report decoding procedure can
> be improved but for now I think it's OK, we want to have this driver in
> ASAP.
>
> David D. please bring up again the issue about missing keypresses on
> re-connection when sending the driver to linux-input with a full
> description of what you observed, I don't have a clue about these
> matters but people on linux-input might.
I now have the remote, I tried using xinput test and most keys seems
to be working fine except the special buttons like subtitle, colors,
x, l1... Im not sure if they are not being mapped because they don't
have any representation or there is something wrong in the parser
itself (Bastien do they used to work for you?). Also got a problem on
suspend but I will have to reproduce it again to see if it is because
of the new driver or not.
--
Luiz Augusto von Dentz
--
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
