[ adding dmaengine ML to Cc: ]
Thanks for your feedback, Gerhard
2014-02-13 4:07 GMT+04:00 Gerhard Sittig <gsi@xxxxxxx>:
> On Wed, Feb 12, 2014 at 17:25 +0400, Alexander Popov wrote:
>> /*
>> - * This is initial version of MPC5121 DMA driver. Only memory to memory
>> - * transfers are supported (tested using dmatest module).
>> + * MPC512x and MPC8308 DMA driver. It supports
>> + * memory to memory data transfers (tested using dmatest module) and
>> + * data transfers between memory and peripheral I/O memory
>> + * by means of slave s/g with these limitations:
>> + * - chunked transfers (transfers with more than one part) are refused
>> + * as long as proper support for scatter/gather is missing;
>> + * - transfers on MPC8308 always start from software as this SoC appears
>> + * not to have external request lines for peripheral flow control;
>> + * - minimal memory <-> I/O memory transfer size is 4 bytes.
>> */
>
> Often I assume people would notice themselves, and apparently I'm
> wrong. :) Can you adjust the formatting such (here and
> elsewhere) that the bullet list is clearly visible as such?
> Flowing text like above obfuscates the fact that the content may
> have a structure ...
Ok, thanks :)
> There are known limitations which are not listed here, "minimal
> transfer size" is incomplete. It appears that you assume
> constraints on start addresses as well as sizes/lengths. Can you
> update the documentation to match the implementation?
Ok, I see. How about that?
* - minimal memory <-> I/O memory transfer chunk is 4 bytes and consequently
* source and destination addresses must be 4-byte aligned
* and transfer size must be aligned on (4 * maxburst) boundary;
>> + /* Grab either several mem-to-mem transfer descriptors
>> + * or one peripheral transfer descriptor,
>> + * don't mix mem-to-mem and peripheral transfer descriptors
>> + * within the same 'active' list. */
>> + if (mdesc->will_access_peripheral) {
>> + if (list_empty(&mchan->active))
>> + list_move_tail(&mdesc->node, &mchan->active);
>> + break;
>> + } else
>> + list_move_tail(&mdesc->node, &mchan->active);
>> + }
> There are style issues. Both in multi line comments, and in the
> braces of the if/else block.
Ah, thanks! I'll fix that.
>> + struct mpc_dma *mdma = dma_chan_to_mpc_dma(chan);
>> + struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
>> + struct mpc_dma_desc *mdesc = NULL;
>> + dma_addr_t per_paddr;
>> + u32 tcd_nunits;
>> + struct mpc_dma_tcd *tcd;
>> + unsigned long iflags;
>> + struct scatterlist *sg;
>> + size_t len;
>> + int iter, i;
> Personally I much dislike this style of mixing declarations and
> instructions. But others may disagree, and strongly so.
Excuse me, I would like to keep it similar to other parts of this driver
and not to change that style.
>> + mdesc = list_first_entry(&mchan->free, struct mpc_dma_desc,
>> + node);
> style (continuation and indentation)
Thanks! I'll fix that.
>
>> + if (!mdesc) {
>> + spin_unlock_irqrestore(&mchan->lock, iflags);
>> + /* try to free completed descriptors */
>> + mpc_dma_process_completed(mdma);
>> + return NULL;
>> + }
>> +
>> + list_del(&mdesc->node);
>> +
>> + per_paddr = mchan->per_paddr;
>> + tcd_nunits = mchan->tcd_nunits;
>> +
>> + spin_unlock_irqrestore(&mchan->lock, iflags);
>> +
>> + if (per_paddr == 0 || tcd_nunits == 0)
>> + goto err_prep;
>> +
>> + mdesc->error = 0;
>> + mdesc->will_access_peripheral = 1;
>> + tcd = mdesc->tcd;
>> +
>> + /* Prepare Transfer Control Descriptor for this transaction */
>> +
>> + memset(tcd, 0, sizeof(struct mpc_dma_tcd));
>> +
>> + if (!IS_ALIGNED(sg_dma_address(sg), 4))
>> + goto err_prep;
>
> You found multiple ways of encoding the "4 byte alignment", using
> both the fixed number as well as (several) symbolic identifiers.
> Can you look into making them use the same condition if the same
> motivation is behind the test?
Gerhard, I don't see checks which can be kind of "merged"
since they have different motivation behind:
1) src_addr_width and dst_addr_width have type "enum dma_slave_buswidth"
and are compared with DMA_SLAVE_BUSWIDTH_4_BYTES which has
the same type;
2) source and destination addresses are checked to be 4-byte aligned;
3) transfer size is checked to be aligned on nbytes boundary
which is (4 * maxburst).
I think the code provides good readability. What do you think?
Thank you!
Best regards,
Alexander
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
