Messages in this thread Patch in this message | | | Date | Thu, 25 Mar 2010 21:58:04 +0100 (CET) | From | Guennadi Liakhovetski <> | Subject | Re: [PATCH 2/2] DMAENGINE: generic channel status |
| |
On Thu, 25 Mar 2010, Linus Walleij wrote:
> Convert the device_is_tx_complete() operation on the > DMA engine to a generic device_tx_status()operation which > can return three states, DMA_TX_RUNNING, DMA_TX_COMPLETE, > DMA_TX_PAUSED. > > Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> > Cc: Maciej Sosnowski <maciej.sosnowski@intel.com> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Li Yang <leoli@freescale.com> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > Cc: Paul Mundt <lethal@linux-sh.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Haavard Skinnemoen <haavard.skinnemoen@atmel.com> > Cc: Magnus Damm <damm@opensource.se> > Cc: Liam Girdwood <lrg@slimlogic.co.uk> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > Cc: Joe Perches <joe@perches.com> > Cc: Roland Dreier <rdreier@cisco.com> > ---
[snip]
General: you converted all drivers to the new .device_tx_status() API, but since they don't implement "residue," you left it uninitialised everywhere. Wouldn't it be better to set it to 0 or total length, depending on the complete / not complete status?
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 0731802..c9f2c67 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -40,11 +40,13 @@ typedef s32 dma_cookie_t; > * enum dma_status - DMA transaction status > * @DMA_SUCCESS: transaction completed successfully > * @DMA_IN_PROGRESS: transaction not yet processed > + * @DMA_PAUSED: transaction is paused > * @DMA_ERROR: transaction failed > */ > enum dma_status { > DMA_SUCCESS, > DMA_IN_PROGRESS, > + DMA_PAUSED, > DMA_ERROR, > }; > > @@ -249,6 +251,21 @@ struct dma_async_tx_descriptor { > }; > > /** > + * struct dma_tx_state - filled in to report the status of > + * a transfer. > + * @last: last completed DMA cookie > + * @used: last issued DMA cookie (i.e. the one in progress) > + * @residue: the remaining number of bytes left to transmit > + * on the selected transfer for states DMA_IN_PROGRESS and > + * DMA_PAUSED if this is implemented in the driver, else 0 > + */ > +struct dma_tx_state { > + dma_cookie_t last; > + dma_cookie_t used; > + u32 residue;
In the original proposal by Dan Williams the last member was "unsigned long pos." I don't think, even on 64-bit systems anyone would kick off a > 4GB transfer, but who knows... And - I don't particularly like the name "pos," but I do like the idea of returning bytes transfered better, than bytes left. Can we change this?
> +}; > + > +/** > * struct dma_device - info on the entity supplying DMA services > * @chancnt: how many DMA channels are supported > * @privatecnt: how many DMA channels are requested by dma_request_channel > @@ -276,7 +293,10 @@ struct dma_async_tx_descriptor { > * @device_prep_slave_sg: prepares a slave dma operation > * @device_control: manipulate all pending operations on a channel, returns > * zero or error code > - * @device_is_tx_complete: poll for transaction completion > + * @device_tx_status: poll for transaction completion, the optional > + * txstate parameter can be supplied with a pointer to get a > + * struct with some transfer information, else the call will just
Maybe "with auxiliary transfer status information, otherwise..."
> + * return a simple status code > * @device_issue_pending: push pending transactions to hardware > */ > struct dma_device { > @@ -329,9 +349,9 @@ struct dma_device { > unsigned long flags); > int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd); > > - enum dma_status (*device_is_tx_complete)(struct dma_chan *chan, > - dma_cookie_t cookie, dma_cookie_t *last, > - dma_cookie_t *used); > + enum dma_status (*device_tx_status)(struct dma_chan *chan, > + dma_cookie_t cookie, > + struct dma_tx_state *txstate); > void (*device_issue_pending)(struct dma_chan *chan); > }; > > @@ -572,7 +592,13 @@ static inline void dma_async_issue_pending(struct dma_chan *chan) > static inline enum dma_status dma_async_is_tx_complete(struct dma_chan *chan, > dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used) > { > - return chan->device->device_is_tx_complete(chan, cookie, last, used); > + struct dma_tx_state state; > + enum dma_status status; > + > + status = chan->device->device_tx_status(chan, cookie, &state); > + *last = state.last; > + *used = state.used;
Both last and used can be NULL.
> + return status; > } > > #define dma_async_memcpy_complete(chan, cookie, last, used)\ > -- > 1.6.3.3
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
| |