Re: Turn Sony motion controller into RGB led

Jiri Kosina
2015-04-09T21:50:50Z

On Thu, 9 Apr 2015, Pavel Machek wrote:

> I did not yet figure out how to get sensor data from the controller,
> but it works rather well as a RGB led.

Thanks for the patch, Pavel.

> And yes, this probably would need some more cleanup, but I'm out of
> time now. Perhaps someone else can help...

Yeah, this definitely can't be applied as-is. A few of the most
outstanding issues pointed out below.

> BTW what happened to the __u8->u8 conversion patch? That should go in AFAICT.

It hasn't been lost, still in my queue, but as it is of a very low
priority, it hasn't been processed yet. Please make this one independent
on it (i.e. do not intermix the type conversion with the actual RGB led
patch).

> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 56ce8c2..a1c8b63 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1941,6 +1941,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_BDREMOTE) },
> { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
> { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_MOTION_CONTROLLER) },

Minor nit -- we generally try to keep this list alphabetically sorted,
although even before your change USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER
is misplaced. Would be nice to have it fixed while you are at it.

[ ... snip ... ]
> -static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
> +static int motion_set_leds(struct hid_device *hdev, u8 r, u8 g, u8 b)
> +{
> + int ret;
> + struct motion_leds *buf = kzalloc(sizeof(struct motion_leds), GFP_KERNEL);

kzalloc() failure handling missing.

> +
> + if (sizeof(struct motion_leds) != 7) {

What is this trying to achieve? What error is it protecting against?

> + printk("Struct has bad size\n");

This needs to be prefixed by the driver / device identification (pr_/dev_
macros).

> + }
> +
> +#define SET_LEDS 0x02

Please put this outside of the function scope.

> + buf->type = SET_LEDS;
> + buf->r = r;
> + buf->g = g;
> + buf->b = b;
> +
> + ret = hid_hw_output_report(hdev, buf, sizeof(struct motion_leds));
> + printk("motion: set leds %d\n", ret);

Again, please make this (and most of the other printk()s added by the
patch) driver/device specific (and dependent on a debug option).

[ ... snip ... ]
> +static void motion_state_worker(struct work_struct *work)
> +{
> + struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
> + struct hid_device *hdev = sc->hdev;
> +
> + motion_set_leds(hdev, sc->led_state[0], sc->led_state[1], sc->led_state[2]);
> +
> +// schedule_delayed_work(&sc->state_worker, HZ*3);

The work gets scheduled whenever sony_set_leds() is called, so this can be
just removed, right?

[ ... snip ... ]
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 9c2d7c2..1342b71 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -151,7 +151,14 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>
> if ((report_type == HID_OUTPUT_REPORT) &&
> !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP)) {
> + printk("hid_hw_output_report(dev, buf, %d_REPORT);\n", count);
> ret = hid_hw_output_report(dev, buf, count);
> + {
> + int i;
> + for (i=0; i<count; i++)
> + printk("%02x ", buf[i]);
> + printk("\n");
> + }
[ ... snip ... ]
> + printk("hid_hw_raw_request(dev, %02x, buf, %d, %d, HID_REQ_SET_REPORT);\n",
> + buf[0], count, report_type);
> + {
> + int i;
> + for (i=0; i<count; i++)
> + printk("%02x ", buf[i]);
> + printk("\n");
> + }
> +

Both these need to go.

Maybe Frank or someone else would be willing to work on improving this
patch. I am not applying it in this form.

Thanks,

--
Jiri Kosina
SUSE Labs