On 05/24/2013 06:59 AM, Vinod Koul wrote:
> On Thu, May 23, 2013 at 10:36:24PM +0200, Lars-Peter Clausen wrote:
>> This patch adds dmaengine support for the JZ4740 DMA controller. For now the
>> driver will be a wrapper around the custom JZ4740 DMA API. Once all users of the
>> custom JZ4740 DMA API have been converted to the dmaengine API the custom API
>> will be removed and direct hardware access will be added to the dmaengine
>> driver.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
>> ---
>
>> +static enum jz4740_dma_width jz4740_dma_width(enum dma_slave_buswidth width)
>> +{
>> + switch (width) {
>> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> + return JZ4740_DMA_WIDTH_8BIT;
>> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> + return JZ4740_DMA_WIDTH_16BIT;
>> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> + return JZ4740_DMA_WIDTH_32BIT;
>> + default:
>> + return JZ4740_DMA_WIDTH_32BIT;
>> + }
> Only diff between the values here and dmaengien values is JZ4740_DMA_WIDTH_32BIT
> as 0. But the header tells me taht its default and SIZE one has values in that
> pattern too. If that is the case you maybe able to get rid on conversion and use
> dmaengine values directly.
>
I'd prefer to keep it the way it is. The JZ4740_DMA_WIDTH constants end up
being written to the hardware, while the DMA_SLAVE_BUSWIDTH constants are Linux
internal. I prefer to not mix these two up.
>> +}
>> +
>> +static enum jz4740_dma_transfer_size jz4740_dma_maxburst(u32 maxburst)
>> +{
>> + if (maxburst <= 1)
>> + return JZ4740_DMA_TRANSFER_SIZE_1BYTE;
>> + else if (maxburst <= 3)
>> + return JZ4740_DMA_TRANSFER_SIZE_2BYTE;
>> + else if (maxburst <= 15)
>> + return JZ4740_DMA_TRANSFER_SIZE_4BYTE;
>> + else if (maxburst <= 31)
>> + return JZ4740_DMA_TRANSFER_SIZE_16BYTE;
>> +
>> + return JZ4740_DMA_TRANSFER_SIZE_32BYTE;
>> +}
>> +
>> +static int jz4740_dma_slave_config(struct dma_chan *c,
>> + const struct dma_slave_config *config)
>> +{
>> + struct jz4740_dmaengine_chan *chan = to_jz4740_dma_chan(c);
>> + struct jz4740_dma_config jzcfg;
>> +
>> + switch (config->direction) {
>> + case DMA_MEM_TO_DEV:
>> + jzcfg.flags = JZ4740_DMA_SRC_AUTOINC;
>> + jzcfg.transfer_size = jz4740_dma_maxburst(config->dst_maxburst);
>> + break;
>> + case DMA_DEV_TO_MEM:
>> + jzcfg.flags = JZ4740_DMA_DST_AUTOINC;
>> + jzcfg.transfer_size = jz4740_dma_maxburst(config->src_maxburst);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> +
>> + jzcfg.src_width = jz4740_dma_width(config->src_addr_width);
>> + jzcfg.dst_width = jz4740_dma_width(config->dst_addr_width);
> this should be direction based, typically DMA engines have only one width to be
> programmed.
This one needs both.
>> + jzcfg.mode = JZ4740_DMA_MODE_SINGLE;
>> + jzcfg.request_type = config->slave_id;
>> +
>> + chan->config = *config;
>> +
>> + jz4740_dma_configure(chan->jz_chan, &jzcfg);
>> +
>> + return 0;
> You are NOT use src_addr/dstn_addr? How else are you passing the periphral
> address?
I'm saving the whole config, which will later be used to retrieve the source or
dest address.
>> +}
[...]
>> +static int jz4740_dma_alloc_chan_resources(struct dma_chan *c)
>> +{
>> + struct jz4740_dmaengine_chan *chan = to_jz4740_dma_chan(c);
>> +
>> + chan->jz_chan = jz4740_dma_request(chan, NULL);
>> + if (!chan->jz_chan)
>> + return -EBUSY;
>> +
>> + jz4740_dma_set_complete_cb(chan->jz_chan, jz4740_dma_complete_cb);
>> +
>> + return 0;
> Zero is not expected value, you need to return the descriptors allocated
> sucessfully.
Well, zero descriptors have been allocated. As far as I can see only a negative
return value is treated as an error. Also the core doesn't seem to use the
return value for anything else but checking if it is an error.
>> +}
>> +
[...]
>> + dd->chancnt = 6;
> hard coding is not advised
But there are 6 channels ;)
[...]
>> +
>> +static struct platform_driver jz4740_dma_driver = {
>> + .probe = jz4740_dma_probe,
>> + .remove = jz4740_dma_remove,
>> + .driver = {
>> + .name = "jz4740-dma",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +module_platform_driver(jz4740_dma_driver);
> typically lot of dma driver like to be higher up in the module order. The reason
> is to have device initialized before clients, pls check if you need that
I don't need it.
Thanks for the quick review.
- Lars
