Re: [PATCH v2] Add LED / GPIO support for the PC Engines APU2.

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Steven,

Thanks for the patches. Few initial notes:

- generally it is preferred to submit patches with "git send-email",
   which simplifies review, as the remarks can be added directly in the
   code, in a reply to the patch,
- if you want do add some overall description of the patch set then you
   can add --cover-letter switch which will generate template for this
   purpose.
- please also use scripts/checkpatch.pl before submission
- make sure that the patches are based on the master branch of
   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git,
- always add on CC the maintainers of the affected kernel subsystems -
   this information can be found in the MAINTAINERS file
- in case of LED drivers cc also linux-kernel@vger.kernel.org,
- subscribe to the mailing list related to the subsystem the patches
   are dedicated to, and skim through some recent patches and reviews -
   it can allow for eliminating some common problems from your driver
   and reducing the number of review iterations.

Regarding the patches - gpio-nct5104d.c seems to be targeted for
GPIO subsystem - you should add Linus Walleij and
linux-gpio@vger.kernel.org on cc.

For leds-apu2.c - you shouldn't register gpio driver in the LED
subsystem but add the relevant dependency in the Kconfig. Nonetheless
I can't find gpio-apu2.c driver in the mainline kernel.

You should also use devm_led_classdev_register() for registering LED
class device. Please follow the design of the other LED class drivers
using platform_device_register_simple() for probing.

Best regards,
Jacek Anaszewski

On 10/29/2016 06:38 PM, Steven Haigh wrote:
> Patches attached.
>
> Changes:
> * Updated leds_apu driver. Included an older version in my previous email.
> * Added required nct5104d GPIO driver support.
>
> These are all against 4.4.28 right now - but should probably apply
> cleanly to any future builds with minimal (or nil) changes.
>
> Please include me in the reply as I'm not part of this list.
>