Am Sunday 15 February 2009 05:08:46 schrieb Mike Murphy:
> Greetings,
>
> The attached patch is a result of a few days of hacking to get better
> Linux support for Xbox 360 wireless controllers. Major functional
> improvements include support for the LEDs on the wireless controllers
> (which are auto-set to the controller number as they are on the
> official console), added support for rumble effects on wireless
> controllers, added support for dead zones on all supported
> controllers, and added support for a square axis mapping for the left
> stick on all supported controllers. A large amount of the bulk in this
> patch is from the addition of a sysfs interface to retrieve
> information and adjust the dead zone/square axis settings.
[..]
> Patch generated against 2.6.28.2 (no changes in relevant in-kernel
> driver from 2.6.28.2 to 2.6.28.5, so should apply against latest
> stable).
>
> Comments are appreciated. I am _NOT_ subscribed to the LKML, so please
> CC me directly.
1. You need to check the returns of sscanf
2. This is very ugly:
+/* read-only attrs */
+static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
+ char *buf)
+{
+ int value;
+ if (!strcmp(attr->attr.name, "controller_number"))
+ value = xd->controller_number;
+ else if (!strcmp(attr->attr.name, "pad_present"))
+ value = xd->pad_present;
+ else if (!strcmp(attr->attr.name, "controller_type"))
+ value = xd->controller_type;
+ else
+ value = 0;
+ return sprintf(buf, "%d\n", value);
+}
3. Possible memory leak in error case:
+static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
+ struct xpad_data *data = NULL;
+ int check;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return NULL;
+
+ check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
+ if (check) {
+ kobject_put(&data->kobj);
+ return NULL;
+ }
4. Why the cpup variety?
+ coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
5. What happens if this work is already scheduled?
if (data[0] & 0x08) {
+ padnum = xpad->controller_data->controller_number;
if (data[1] & 0x80) {
- xpad->pad_present = 1;
- usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
- } else
- xpad->pad_present = 0;
+ printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
+ xpad->controller_data->pad_present = 1;
+
+ INIT_WORK(&xpad->work, &xpad_work_controller);
+ schedule_work(&xpad->work);
6. No GFP_ATOMIC. If you can take a mutex you can sleep.
+ usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
