Re: [PATCH v1 1/2] platform/x86: intel-vbtn: Remove redundant inclusions

[Andy Shevchenko]

On Wed, 2018-01-31 at 11:04 -0800, Darren Hart wrote:
> On Wed, Jan 31, 2018 at 08:59:20PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 31, 2018 at 8:50 PM, Darren Hart <dvhart@xxxxxxxxxxxxx>
> > wrote:
> > > On Wed, Jan 31, 2018 at 07:54:06PM +0200, Andy Shevchenko wrote:
> > > > Some headers are not needed since the driver can be built as
> > > > module.
> > > > Remove them.
> > > 
> > > Removing init because it's included by module.h, and removing
> > > acpi_bus.h
> > > because it's included by acpi.h - but not because it can be built
> > > as a
> > > module - right?  Just checking, the wording in the commit msg
> > > seemed odd
> > > to me.
> > 
> > Correct. I'll rephrase this in place.


My gosh, I forgot to do this and can't rebase anymore. Sorry.

> > > These removals seem appropriate to me, but so we have it recorded
> > > here -
> > > in general, including headers that you explicitly make use of is
> > > good
> > > practice, and not depending on others to include them. But in this
> > > case,
> > > the implicit includes are reasonable expectations as they are
> > > tightly
> > > coupled with the parent include.
> > 
> > There are two classes of headers (at least?):
> > - let say "user-visible" ones, which drivers usually include like
> > you
> > pointed above
> > - low level ones, which in most cases are not supposed to be
> > included directly
> > 
> > So, for first class I agree with you, and acpi_bus.h in this case
> > can
> > be considered as an example of second class.
> 
> Agreed, acpi_bus.h is tightly coupled with acpi.h. The practice I've
> seen from others and want to discourage / avoid is including acpi.h
> and
> then deleting ... say...  spinlock.h because somewhere somehow acpi.h
> also pulls it in. They are not tightly coupled conceptually, so
> spinlock.h should remain in the include list if the file uses
> spinlocks
> directly. I think we're in violent agreement here :-)

It's a problem of header organization I think. AFAIK Plan 9 has an idea
that each header is independent, and each C module has to include
headers in appropriate order. (Always trade off between flexibility and
strict hierarchy).

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy