Re: [PATCH] staging: ath6kl: Including a header file to fix a compilation error

[Greg KH <greg@kroah.com>]

On Fri, Sep 17, 2010 at 04:26:41PM -0700, Vipin Mehta wrote:
> > 
> > Ok, I'll not apply this until it is all resolved.
> The error popped up due a recent commit which happened after I generated the patch below. I can regenerate the patch that takes care of these new errors as well but the following patch does fix the problem originally reported.

{sigh}  Line length please...

> > > > diff --git a/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsconfig.h
> > b/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsconfig.h
> > > > index 4358834..4e5b7bf 100644
> > > > --- a/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsconfig.h
> > > > +++ b/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsconfig.h
> > > > @@ -36,15 +36,11 @@
> > > >
> > > >  #include <linux/fs.h>
> > > >  #include <linux/errno.h>
> > > > -#include <linux/string.h>
> > > >  #include <linux/signal.h>
> > > > -#include <linux/timer.h>
> > > >
> > > >
> > > >  #include <linux/ioctl.h>
> > > > -#include <linux/skbuff.h>
> > > >  #include <linux/firmware.h>
> > > > -#include <linux/wait.h>
> > 
> > Why are you removing these?
> The header files being removed are already included by a common header file osapi_linux.h. The file osapi_linux.h is included by all the source files. I saw little point in including these header files again.

But that has nothing to do with this fix, right?  Don't mix things in a
single patch.  Cleanup is good and nice, but don't do that in a "fix a
problem" patch at the same time.

Remember:
	One patch per "thing" you do.

> > > > -#include <linux/semaphore.h>
> > > >  #include <linux/wireless.h>
> > > >  #ifdef ATH6K_CONFIG_CFG80211
> > > >  #include <net/cfg80211.h>
> > 
> > And these?
> > 
> > All to fix the one build error?
> No. The header files being removed are just a minor clean up that I thought can be included along with the fix as they were kind of related. I guess I could use a separate patch.

Yes, that is required.

> > > >  #include <linux/timer.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/wait.h>
> > > > +#include <linux/semaphore.h>
> > 
> > Don't put #include in a .h file if at all possible please.
> I guess the header files can be included explicitly within the source files but it requires a bigger change to the driver. There are some header files which are included by all the source files and contain some commonly used header files. These commonly used header files will otherwise have to be included separately in each of the source files.

That's work that can be done later, let's just fix this build error
first please.

thanks,

greg k-h