Re: [PATCH 2/2] DMAENGINE: generic channel status

[Linus Walleij <linus.ml.walleij@gmail.com>]

2010/3/25 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:

> 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?

The total length is a bit hard to know without understanding all drivers
I'm afraid, but I can sure add txtstatus->residue = 0; everywhere,
I'll fix.

>> +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?

As Dan says consistency with other systems suggests something
residue or bytes_left.

>> - * @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..."

OK

>> @@ -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.

Good you spotted this! Thanks.

Linus