Re: [PATCH v2] dma: vdma: Fix compilation warnings

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

On Tue, Mar 31, 2015 at 07:35:43AM +1100, Stephen Rothwell wrote:
Hi Stephen,
>
> On Mon, 30 Mar 2015 22:48:46 +0530 Vinod Koul <vinod.koul@intel.com> wrote:
> >
> > On Mon, Mar 30, 2015 at 06:48:29PM +0530, Kedareswara rao Appana wrote:
> > > This patch fixes the following compilation warnings.
> > > In file included from drivers/dma/xilinx/xilinx_vdma.c:26:0:
> > > include/linux/dmapool.h:18:4: warning: 'struct device' declared inside parameter list
> > >     size_t size, size_t align, size_t allocation);
> > >     ^
> > > include/linux/dmapool.h:18:4: warning: its scope is only this definition or declaration, which is probably not what you want
> > > include/linux/dmapool.h:31:7: warning: 'struct device' declared inside parameter list
> > >        size_t size, size_t align, size_t allocation);
> > >        ^
> > > drivers/dma/xilinx/xilinx_vdma.c: In function 'xilinx_vdma_alloc_chan_resources':
> > > drivers/dma/xilinx/xilinx_vdma.c:501:20: warning: passing argument 2 of 'dma_pool_create' from incompatible pointer type
> > >   chan->desc_pool = dma_pool_create("xilinx_vdma_desc_pool",
> > >                     ^
> > > In file included from drivers/dma/xilinx/xilinx_vdma.c:26:0:
> > > include/linux/dmapool.h:17:18: note: expected 'struct device *' but argument is of type 'struct device *'
> > >  struct dma_pool *dma_pool_create(const char *name, struct device *dev, .
> > > 
> > Well this does fix this error but this can also be fixed by rearranging the
> > driver header files order.  Since I am not inclined to update a patch for
> > dmapool.h I would go for rearranging drivers header
> > 
> > --><8---------------><8--------------
> > 
> > diff --git a/drivers/dma/xilinx/xilinx_vdma.c
> > b/drivers/dma/xilinx/xilinx_vdma.c
> > index d8434d465885..356ca4bc0ea5 100644
> > --- a/drivers/dma/xilinx/xilinx_vdma.c
> > +++ b/drivers/dma/xilinx/xilinx_vdma.c
> > @@ -23,12 +23,12 @@
> >   */
> > 
> >  #include <linux/bitops.h>
> > -#include <linux/dmapool.h>
> >  #include <linux/dma/xilinx_dma.h>
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > +#include <linux/dmapool.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_dma.h>
> >  #include <linux/of_platform.h>
> > 
> > Any objections?
> 
> Yes.  The error is in dmapool.h so it should be fixed once and for
> all.  The supplied patch is very unintrusive and means that the problem
> won't reappear when someone does some rearrangement of includes in the
> future.  The file in question really has no particular maintainer.
> Even after your suggested patch, dmapool.h still depend on an implicit
> include of device.h.
I agree with your points, but isnt the order of headers also a thumb rule.
Typicaly a driver file will include core includes followed by subsystem
specfic includes.

Should a header have no dependency for its include ? I do come across
multiple examples of this in kernel

Yes fixing it in dmapool is also correct, so should we move to having
headers agnostic to the order of inclusion eventually ?

Thanks
-- 
~Vinod

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]