Re: [PATCH] hid: add force feedback support for SmartJoy PLUS PS2/USB adapter

[Jiri Slaby <jslaby@suse.cz>]

On 05/11/2009 05:17 PM, Jiri Kosina wrote:
> On Fri, 8 May 2009, Jussi Kivilinna wrote:
> 
>> This driver adds force feedback support for SmartJoy PLUS PS2/USB 
>> adapter. I made this driver one device spesific instead of making 
>> generic 'wisegroup-ff' because I have another Wisegroup PS2/USB adapter 
>> that doesn't work same way as SmartJoy PLUS. If another device that is 
>> compatible pops up, this driver could be then renamed to something more 
>> generic.
> 
> Thanks for writing the driver!
> 
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1312,6 +1312,9 @@ static const struct hid_device_id hid_blacklist[] = {
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb651) },
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb654) },
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED, USB_DEVICE_ID_TOPSEED_CYBERLINK) },
>> +#if defined(CONFIG_SMARTJOYPLUS_FF) || defined(CONFIG_SMARTJOYPLUS_FF_MODULE)
>> +	{ HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SMARTJOY_PLUS) },
>> +#endif
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0005) },
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0030) },
> 
> Hmm, this isn't really compatible with what we have for other drivers, we 
> require the config option to be turned on in order to have support (i.e. 
> we include them into hid_blacklist unconditionally).

Strictly speaking, it depends on whether the device is able to be
managed by the generic layer or not. And it seems it can; this driver
adds only a FF support. If we disabled the FF support in the config and
the #if macro wasn't there, we would lose the device completely even
though it can be handled by the core, without FF though.

So I think, in this particular case, it is OK to have the conditional
line in there.

Or maybe better, to not taint the core code by such a bloat, leave
conditonal only the part of the driver which takes care of FF. The rest
of the driver (only the init/deinit) would be left as unable-to-disable
if !EMBEDDED, like other drivers.

I mean, two Kconfig entries, driver and FF support, driver is built
always if !EMBEDDED, FF is optional and dependent on the driver.
CONFIG_SMARTJOYPLUS_FF is then around sjoyff_init and *_play.



Here comes some of my comments to the rest:

+#define debug(format, arg...) pr_debug("hid-sjoyff: " format "\n" , ## arg)

Define pr_fmt instead of this.

+static int hid_sjoyff_play(struct input_dev *dev, void *data,
+			 struct ff_effect *effect)
+{
+	struct hid_device *hid = input_get_drvdata(dev);
+	struct sjoyff_device *sjoyff = data;
+	u32 left, right;
+
+	left = effect->u.rumble.strong_magnitude;
+	right = effect->u.rumble.weak_magnitude;
+	debug("called with 0x%08x 0x%08x", left, right);
+
+	left = left * 0xff / 0xffff;

Confused here, * ff / ffff, what's the point? Is there any difference
with left /= 0xff?

Thanks for the driver.