[alsa-devel] [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Barry Song]

2014-12-25 17:08 GMT+08:00 Barry Song <21cnbao at gmail.com>:
> 2014-12-25 17:02 GMT+08:00 Lars-Peter Clausen <lars at metafoo.de>:
>> On 12/25/2014 05:41 AM, Barry Song wrote:
>>>
>>> From: Barry Song <Baohua.Song at csr.com>
>>>
>>> preparing cyclic DMA description can fail either due to DMA desc list
>>> is full(-ENOMEM), or due to the coming DMA configuration is illegal or
>>> not supported by the acting DMA hardware(other ERR codes).
>>
>>
>> According to the API definition this returns either NULL or a valid
>> descriptor, so the behavior in pcm_dmaengine is correct. Maybe your
>> particular dmaengine driver as a incorrect implementation.
>
> i think it is something wrong. an functions returns pointer should return one of
> (1) right pointer
> (2) NULL
> (3) a wrong pointer from error CODES
>
> if for 2&3, we both return NULL, it actually means we are taking
> something wrong.
>
> this should be a generic rule as from clk and other subsystems. why
> does DMA want to do something special?

+ Vinod, i need your input for this, we rejected an unsupported DMA
configuration by returning PTR_ERR, and we returned NULL if there is
no free desc.
this is clearly two different scenarios and reasons for failed
dma_prep_cyclic(). i think this should be visible to the API users.
this is a necessary information not a blackbox to the API users.

static struct dma_async_tx_descriptor *
sirfsoc_dma_prep_cyclic(struct dma_chan *chan, dma_addr_t addr,
size_t buf_len, size_t period_len,
enum dma_transfer_direction direction, unsigned long flags)
{
struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
struct sirfsoc_dma_desc *sdesc = NULL;
unsigned long iflags;

/*
* we only support cycle transfer with 2 period
* If the X-length is set to 0, it would be the loop mode.
* The DMA address keeps increasing until reaching the end of a loop
* area whose size is defined by (DMA_WIDTH x (Y_LENGTH + 1)). Then
* the DMA address goes back to the beginning of this area.
* In loop mode, the DMA data region is divided into two parts, BUFA
* and BUFB. DMA controller generates interrupts twice in each loop:
* when the DMA address reaches the end of BUFA or the end of the
* BUFB
*/
if (buf_len !=  2 * period_len)
return ERR_PTR(-EINVAL);

/* Get free descriptor */
spin_lock_irqsave(&schan->lock, iflags);
if (!list_empty(&schan->free)) {
sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc,
node);
list_del(&sdesc->node);
}
spin_unlock_irqrestore(&schan->lock, iflags);

if (!sdesc)
return NULL;

...

return &sdesc->desc;
}

do you think we need to refine the API definition or do we need to
change the driver?


-barry