On Tue, Jun 6, 2017 at 7:54 PM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote: > On Tue, Jun 06, 2017 at 12:30:38PM +0300, Andy Shevchenko wrote: >> On Tue, Jun 6, 2017 at 6:16 AM, Andy Lutomirski <luto@xxxxxxxxxx> wrote: >> > +#include <linux/kernel.h> >> > +#include <linux/module.h> >> > +#include <linux/init.h> >> > +#include <linux/slab.h> >> > +#include <linux/types.h> >> > +#include <linux/input.h> >> > +#include <linux/input/sparse-keymap.h> >> > +#include <linux/acpi.h> >> > +#include <linux/string.h> >> > +#include <linux/dmi.h> >> > +#include <linux/wmi.h> >> > +#include <acpi/video.h> >> >> Alphabetical order? Up to you. > > OK, I failed to audit this... lots we don't need in here. > > The minimum to build is: > > #include <linux/wmi.h> > > So assuming this was copy/pasted from another file. > Again, no guidance in coding-style.rst on includes. Seems to me we should > include what we specifically require, regardless of whether or not another > header also happens to include it. Usually it's a sane choice. Regarding to order the rationale I see there is easiest way to detect (on the glance) what headers are already there and there is no duplication. I saw in the past few patches to remove header duplication since the original list wasn't in order in the first place. Of course there might be exceptions. > We need acpi for example, even though wmi > also includes it. > > We should include modules, even though acpi includes it. > > We use several other things we aren't including for, like > > memcpy > dev_kzalloc > sysfs_create_bin_file > > So I suggest: > > #include <linux/acpi.h> > #include <linux/device.h> > #include <linux/fs.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/string.h> > #include <linux/sysfs.h> > #include <linux/types.h> > #include <linux/wmi.h> > > Which removes: > #include <acpi/video.h> > #include <linux/dmi.h> > #include <linux/init.h> > #include <linux/input.h> > #include <linux/input/sparse-keymap.h> > #include <linux/slab.h> > > And adds: > #include <linux/device.h> > #include <linux/fs.h> > #include <linux/sysfs.h> Works for me! -- With Best Regards, Andy Shevchenko
