On Tue, May 29, 2018 at 12:57 AM, Roderick Colenbrander <thunderbird2k@xxxxxxxxx> 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 <abos@xxxxxxxx> 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> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
