Re: [PATCH 1/1] HID: add BETOP game controller force feedback support

[Jiri Kosina]

On Mon, 24 Nov 2014, 黄波 wrote:

> From: Huang Bo <huangbobupt@xxxxxxx>
> 
> Adds force feedback support for BETOP USB game controllers.
> These devices are mass produced in China.

Thanks a lot for the patch. A few minor things below.

First, the whole driver formatting doesn't comply with our coding style 
(it has 4 spaces instead of tab). Please reformat that for v2.

[ ... snip ... ]
> diff --git a/drivers/hid/hid-betopff.c b/drivers/hid/hid-betopff.c
> new file mode 100644
> index 0000000..d97301a
> --- /dev/null
> +++ b/drivers/hid/hid-betopff.c
> @@ -0,0 +1,170 @@
> +/*
> + *  Force feedback support for Betop based devices
> + *
> + *  The devices are distributed under various names and the same USB device ID
> + *  can be used in both adapters and actual game controllers.
> + *
> + *  0x11C0:0x5506 "BTP2185 PC mode Joystick"
> + *   - tested with BTP2185 V2 PC Mode.
> + *
> + *  0x8380:0x1850 "BTP2185 V2 PC mode USB Gamepad"
> + *   - tested with BTP2185 PC Mode with another version.
> + *
> + *  0x20bc:0x5500 "BTP2185 V2 BFM mode Joystick"
> + *   - tested with BTP2185 BFM Mode.
> + *   - tested with BTP2171s.
> + *
> + *  Copyright (c) 2014 Huang Bo <huangbobupt@xxxxxxx>
> + */
> +
> +/*
> + * 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.
> + */
> +
> +/* #define DEBUG */
> +
> +#define debug(format, arg...) pr_debug("hid-betopff: " format "\n" , ## arg)
> +
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/hid.h>
> +
> +#include "hid-ids.h"
> +
> +struct betopff_device {
> +    struct hid_report *report;
> +};
> +
> +static int hid_betopff_play(struct input_dev *dev, void *data,
> +             struct ff_effect *effect)
> +{
> +    struct hid_device *hid = input_get_drvdata(dev);
> +    struct betopff_device *betopff = data;
> +    __u16 left, right;
> +
> +    left = effect->u.rumble.strong_magnitude;
> +    right = effect->u.rumble.weak_magnitude;
> +
> +    betopff->report->field[2]->value[0] = left / 256;
> +    betopff->report->field[3]->value[0] = right / 256;
> +    debug("running with 0x%02x 0x%02x", left, right);

Is there really a need for keeping this debug() in the "production" 
version?

[ ... snip ... ]
> +static const struct hid_device_id betop_devices[] = {
> +    { HID_USB_DEVICE(0x11C0, 0x5506), }, /* BTP2185 PC mode */
> +    { HID_USB_DEVICE(0x8380, 0x1850), }, /* BTP2185 V2 PC mode */
> +    { HID_USB_DEVICE(0x20bc, 0x5500), }, /* BTP2185 V2 BFM mode */
> +    { }

You also need an entry for these devices in hid_have_special_driver[] 
array in drivers/hid/hid-core.c, otherwise correct device <-> driver 
binding can't be guaranteed.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/