Hi Hanno,
A few suggestions for your patch based on a quick review (had limited time).
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'.
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).
Thanks,
Roderick
On Wed, May 23, 2018 at 8:57 AM, Hanno Zulla <[email protected]> wrote:
> Hello,
>
> this is a driver for the "BigBen Interactive Kid-friendly Wired
> Controller PS3OFMINIPAD SONY" [1] with USB id 146b:0902. It was
> originally sold as a PS3 accessory and serves as a very nice gamepad for
> Retropie.
>
> With the help of serialusb [2], it was possible to analyze the protocol
> used by the gamepad and fix the missing entries for the descriptor.
>
>
> This is my *first* driver, so please review accordingly.
>
>
> Three things where I'm not sure and hope to hear from you about:
>
> - is that the correct use of hid_validate_values() in bigben_probe()?
>
> - is the error handling in bigben_probe() correct?
>
> - how do I properly test possible suspend/resume scenarios?
>
> Thanks,
>
> Hanno
>
>
> [1]
> <https://www.bigben-interactive.co.uk/bigben-products/gaming-accessories/wired-mini-controller-ps3>
>
> [2] <https://github.com/matlo/serialusb>
On Tue, May 29, 2018 at 12:57 AM, Roderick Colenbrander
<[email protected]> wrote:
> Hi Hanno,
>
> A few suggestions for your patch based on a quick review (had limited time).
>
> 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'.
>
> 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).
Thanks a lot Roderick for the review.
In addition to those comments, please also try to follow the
submission guidelines (point 6 of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst).
It's easier for us to share comments if your patch is inlined in the
email, so we can directly pinpoint which line is in trouble.
Also, keep point 9 in mind ("Don't get discouraged - or impatient").
For the first submissions, it's usual to have several revisions of the
same series.
Cheers,
Benjamin
>
> Thanks,
> Roderick
>
> On Wed, May 23, 2018 at 8:57 AM, Hanno Zulla <[email protected]> wrote:
>> Hello,
>>
>> this is a driver for the "BigBen Interactive Kid-friendly Wired
>> Controller PS3OFMINIPAD SONY" [1] with USB id 146b:0902. It was
>> originally sold as a PS3 accessory and serves as a very nice gamepad for
>> Retropie.
>>
>> With the help of serialusb [2], it was possible to analyze the protocol
>> used by the gamepad and fix the missing entries for the descriptor.
>>
>>
>> This is my *first* driver, so please review accordingly.
>>
>>
>> Three things where I'm not sure and hope to hear from you about:
>>
>> - is that the correct use of hid_validate_values() in bigben_probe()?
>>
>> - is the error handling in bigben_probe() correct?
>>
>> - how do I properly test possible suspend/resume scenarios?
>>
>> Thanks,
>>
>> Hanno
>>
>>
>> [1]
>> <https://www.bigben-interactive.co.uk/bigben-products/gaming-accessories/wired-mini-controller-ps3>
>>
>> [2] <https://github.com/matlo/serialusb>
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
Hi,
> 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).
Sorry, that was wrong. I meant BTN_DPAD_* instead of HAT0, despite them
being digital buttons.
Kind regards,
Hanno