On Tue, May 14, 2013 at 11:07:06AM +0100, Ian Abbott wrote:
> On 2013-05-14 07:42, Dan Carpenter wrote:
> >On Mon, May 13, 2013 at 07:02:07PM -0500, H Hartley Sweeten wrote:
> >>On Monday, May 13, 2013 1:06 AM, Dan Carpenter wrote:
> >>>On Fri, May 10, 2013 at 11:37:38AM -0500, H Hartley Sweeten wrote:
> >>>>On Friday, May 10, 2013 6:07 AM, Ian Abbott wrote:
> >>>>>@@ -84,11 +86,15 @@ static void __comedi_buf_alloc(struct comedi_device *dev,
> >>>>> for (i = 0; i < n_pages; i++) {
> >>>>> buf = &async->buf_page_list[i];
> >>>>> if (s->async_dma_dir != DMA_NONE)
> >>>>>+#ifdef CONFIG_HAS_DMA
> >>>>> buf->virt_addr = dma_alloc_coherent(dev->hw_dev,
> >>>>> PAGE_SIZE,
> >>>>> &buf->dma_addr,
> >>>>> GFP_KERNEL |
> >>>>> __GFP_COMP);
> >>>>>+#else
> >>>>>+ break;
> >>>>>+#endif
> >>>>
> >>>>This break is unnecessary.
> >>>>
> >>>
> >>>You'd need something there or it would cause a parse error when
> >>>CONFIG_HAS_DMA is disabled. "break;" is probably as good as
> >>>"buf->virt_addr = NULL;" or whatever.
> >>
> >>async->buf_page_list is vzalloc'ed, so buf->virt_addr is already NULL.
> >>
> >
> >Yes. I know. So what you're suggesting is this?:
> >
> > if (s->async_dma_dir != DMA_NONE)
> >#ifdef CONFIG_HAS_DMA
> > buf->virt_addr = dma_alloc_coherent(dev->hw_dev,
> > PAGE_SIZE,
> > &buf->dma_addr,
> > GFP_KERNEL |
> > __GFP_COMP);
> >#else
> > ;
> >#endif
> >
> >That looks nasty to me. Actually this should probably be hidden
> >away in a .h file per kernel style guidelines.
> >
> >#ifdef CONFIG_HAS_DMA
> >
> >static inline void *
> >comedi_dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> > gfp_t gfp)
> >{
> > return dma_alloc_coherent(dev, size, dma_handle, gfp);
> >}
> >
> >#else
> >
> >static inline void *
> >comedi_dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> > gfp_t gfp)
> >{
> > return NULL;
> >}
> >
> >#endif
>
> I didn't want to hide it, in the hope that dummy versions of the dma
> functions would be added in the proper place one day.
I don't think they are ever going to add a dummy function. The
comments are clear that they deliberately intended it to break at
link time when CONFIG_HAS_DMA is not set.
Anyway, we can change it later at any point. It's just a matter of
deleting the dummy function from the .h file and the comedi_ prefix
from the function calls. No big deal.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
