On Tue, Mar 13, 2018 at 5:32 PM, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Mon, Mar 12, 2018 at 6:46 PM, Matan Ziv-Av <matan@xxxxxxxxxxx> wrote: >> >> Adds a driver for hotkeys, leds, battery charge limiter and fan mode on >> LG Gram laptops. > > Thanks for the patch, my concerns below. > >> Documentation/laptops/lg-laptop.txt | 71 +++ > > This should be in reST file format. > >> drivers/platform/x86/lg-laptop.c | 724 ++++++++++++++++++++++++++++++++++++ > > You have too many subtle issues, such as: > - redundant empty lines > - missed empty lines > - redundant conditionals (before kstrtox() call) > - shadowing returned error codes (kstrtox() usage) > - etc, I even would not like to look at > > So, please, run checkpatch.pl and see if it complains about anything. > > Besides that, the approaches you choose might be old or racy: > - device attribute handling (use sysfs attribute group) > - btw, where is the ABI description? > > Why did you choose so complex way to instantiate a driver? All those > ACPI handlers and additional burden for platform device... > Can it be simplified? > Can you utilize WMI bus as much as possible. Matan, do you have any updates on the driver? I'm sorry but I can't apply it in a such form and especially when you mark yourself as a maintainer and still have no fixes for the comments above. -- With Best Regards, Andy Shevchenko
