Re: New Force Feedback device support - GreenAsia 0x12

[Anssi Hannula <anssi.hannula@gmail.com>]

Łukasz Lubojański wrote:
> On Fri, Dec 5, 2008 at 7:32 PM, Anssi Hannula <anssi.hannula@gmail.com> wrote:
>> Łukasz Lubojański wrote:
>>> On Thu, Dec 4, 2008 at 9:35 PM, Łukasz Lubojański
>>> <lukasz@lubojanski.info> wrote:
>>>> 2008/11/29 Jiri Kosina <jkosina@suse.cz>:
>>>>> On Fri, 28 Nov 2008, Łukasz Lubojański wrote:
>>>>>
>>>>>>> It seems the protocol resembles more the hid-lg2ff one. The
>>>>>>> differences
>>>>>>> are the additional 0xfa 0xfe 0x0 report sent to the device, and the
>>>>>>> missing 0xf3 stop command.
>>>>>> Yep - different reports are send in case of Pantherlord and GreenAsia
>>>>>> 0x12 - It could be implemented in it but it will require checking what
>>>>>> hardware is used and send different reports.
>>>>> OK, so as the reports are not really identical, and in the future we
>>>>> might
>>>>> discover that there are many more other Greenasia devices which require
>>>>> a
>>>>> slightly different handling as well, I would rather prefer to have it as
>>>>> a
>>>>> separate driver, to avoid additions of here-and-there device-specific
>>>>> quirks to random places in the code. That's exactly what we are trying
>>>>> to
>>>>> avoid with the HID bus approach in the first place.
>>>>>
>>>>> So I think separate driver is fine.
>>>>>
>>>>> Thanks to both of you.
>>>>>
>>>>> --
>>>>> Jiri Kosina
>>>>> SUSE Labs
>>>> Hi,
>>>>
>>>> Here is new version of the GreenAsia patch - I hope this time
>>>> everything will be OK. It is based on the Pantherlord.
>>>>
>>>> Sorry to take so long but I have problems with the 2.6.28 (2.6.28-rc6
>>>> was not loading my driver and 2.6.28-rc7 is crashing when IO APIC is
>>>> enabled). Anyway I done it and I'm waiting for your feedback :D
>>
>> Is this really a HID_QUIRK_MULTI_INPUT device (Multiple controllers on one
>> device, for example a 2-in-1 adapter)? Just asking because your previous
>> patch didn't have this.
>>
>> If this is not the case, there is also no need to have 2 new Kconfig
>> entries, but a simple FF-only entry (see ZEROPLUS_FF / hid-zpff.c).
>>
> 
> In case of both devices that I have the HID_QUIRK_MULTI_INPUT is not
> set - so I think the multi input code can be removed. I did that and
> it still works properly in my test :D
> 
> I have seen that hid-zpff and hid-tmff are modules for FF only and
> hid-pl is "dummy" support for the device and if selected also for the
> FF. I was confused about it so I have chosen way of hid-pl because I
> based on it.

hid-pl just sets HID_QUIRK_MULTI_INPUT when FF is disabled. For gaff we 
do not need that.

> Because the module supports only the FF now - I have changed module
> name to hid-gaff.
[...]
> +	gaff->report->field[0]->value[0] = 0x51;
> +	gaff->report->field[0]->value[1] = 0x0;
> +	gaff->report->field[0]->value[2] = right;
> +	gaff->report->field[0]->value[3] = 0;
> +	gaff->report->field[0]->value[4] = left;
> +	gaff->report->field[0]->value[5] = 0;

[...]
> +	if (list_empty(report_list)) {
> +		dev_err(&hid->dev, "no output reports found\n");
> +		return -ENODEV;
> +	}
> +
> +	report_ptr = report_ptr->next;

> +	if (report_ptr == report_list) {
> +		dev_err(&hid->dev, "required output report is "
> +				"missing\n");
> +		return -ENODEV;
> +	}

Unneeded test. list_empty() call above already confirmed that there are 
output reports.

> +	report = list_entry(report_ptr, struct hid_report, list);
> +	if (report->maxfield < 1) {
> +		dev_err(&hid->dev, "no fields in the report\n");
> +		return -ENODEV;
> +	}
> +
> +	if (report->field[0]->report_count < 4) {
> +		dev_err(&hid->dev, "not enough values in the field\n");

Here you test for only 4, while in hid_gaff_play() you use 6.

-- 
Anssi Hannula

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html