Messages in this thread | | | Subject | Re: [PATCH] HID: Add support for 146b:0902 Bigben Interactive Kids' gamepad | From | Hanno Zulla <> | Date | Tue, 29 May 2018 09:46:24 +0200 |
| |
Hi Roderick, Hi Benjamin,
thanks for the review!
> In terms of code style, I noticed a lot of C++ style comments which> are not part of the kernel code style instead use C comments. To check> for potential other issues as well, run your patch through> 'scripts/checkpatch.pl'.
Okay, will check with that.
> I noticed you fixed up reported descriptors a bit to get axes remapped > in a different way. This is reasonable as the default mappings are > often not good. However, I would suggest use the mapping functions > instead (e.g. see hid-sony.c and other drivers). It also allows you to > properly remap buttons as well. I suspect the current button mapping > is all over the place as well. Make sure axis and button mapping > complies to the Linux gamepad spec (see > Documentation/input/gamepad.rst).
The main reason for the fixup was to cleanup the output part of the descriptor, as the device's original descriptor seemed to use nonsensical/undefined "Usage" values.
Those two axis remappings were the only two necessary. The rest of the mappings are fine and look okay when compared to other drivers that do not fully adhere to the gamepad spec (e.g. xpad.c also doesn't map my XBox-Controller's D-Pad to NORTH/EAST/SOUTH/WEST but instead to HAT0 in the same way as this device does).
I had assumed that using a descriptor fixup and leaving the actual work to the standard driver was cleaner than meddling with the incoming data in a mapping function.
Actually, I was researching if I could fix the descriptor so that no LED/FF-specific code is necessary, but my knowledge of HID was too limited to achieve that.
> In addition to those comments, please also try to follow the > submission guidelines
Ah, thank you.
> Also, keep point 9 in mind ("Don't get discouraged - or impatient").
Nah. Don't worry! :-D
Thanks & kind regards,
Hanno
| |