Re: [PATCH] HID: Add support for 146b:0902 Bigben Interactive Kids' gamepad

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