Re: [RFC 1/3] i2c: sh_mobile: add DMA support

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

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

> > @@ -33,6 +35,9 @@
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> >  #include <linux/of_device.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/sh_dma.h>
> 
> I would have tried to keep the headers alphabetically sorted, if they had been 
> sorted in the first place :-)

Sure, I can do that as a seperate patch.

> > +static void sh_mobile_i2c_cleanup_dma(struct sh_mobile_i2c_data *pd)
> > +{
> > +	if (pd->dma_direction == DMA_FROM_DEVICE)
> > +		dmaengine_terminate_all(pd->dma_rx);
> > +	if (pd->dma_direction == DMA_TO_DEVICE)
> 
> else ?

Yes, why not.

> > +	pd->buf_mapped = true;
> 
> Can't you use dma_direction != DMA_NONE to detect whether the buffer has been 
> mapped, instead of adding a new field to struct sh_mobile_i2c_data ? You could 
> then simplify sh_mobile_i2c_cleanup_dma() by returning immediately at the 
> beginning of the function if dma_direction == DMA_NONE.

I thought having this explicit might be more readable. Will try your
idea though and check.

> 
> Extra blank line here.

Yes.

Thanks for the review!


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