On 13.01.2016 14:23, Vinod Koul wrote:

On Thu, Jan 07, 2016 at 05:33:04PM +0000, kernel at martin.sperl.org wrote:

quoted

+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
+{
+	/* dma channels >= 7 are LITE channels */
+	return (c->ch >= 7);

Why not DT data here as well

See other email with comments on DT.

quoted

+}
+
+static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)
+{
+	/* lite and normal channels have different max frame length */
+	return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;

Or rather get length from DT..

See other email with comments on DT.

quoted

+	/* allocate and setup the descriptor. */
+	d = kzalloc(sizeof(*d) + frames * sizeof(struct bcm2835_cb_entry),
+		    gfp);

odd style.. btw should flag be GFP_NOWAIT ..?

gfp was used as a method-argument - see your comment below.

quoted

+		/* fill in the control block */
+		control_block = cb_entry->cb;
+		control_block->info = info;
+		control_block->src = src;
+		control_block->dst = dst;
+		if (buf_len) {
+			control_block->length = min(max_len, len);
+			if (period_len &&
+			    (total_len + control_block->length >=
+			     period_len)) {
+				/* set to end of period_len */
+				control_block->length =
+					period_len - total_len;
+				/* add extrainfo when cyclic */
+				if (cyclic)
+					control_block->info |=
+						finalextrainfo;
+				/* and reset total_len */
+				total_len = 0;
+			}

this looks hard to read, perhpas a helper will make it look better

Perhaps - I can look...

quoted

+	/* the last frame requires extra flags */
+	d->cb_list[d->frames - 1].cb->info |= finalextrainfo;
+
+	/* check the size - if there is some missmatch,
+	 * then this is detected here
+	 */

this is not kernel style for multi-line comments

I know - this came up with a different patch-set

quoted

  	/* Grab configuration */
  	if (!is_slave_direction(direction)) {
-		dev_err(chan->device->dev, "%s: bad direction?\n", __func__);
+		dev_err(chan->device->dev,
+			"%s: bad direction?\n", __func__);

unrelated change

OK

quoted

-	/* Now allocate and setup the descriptor. */
-	d = kzalloc(sizeof(*d), GFP_NOWAIT);
-	if (!d)
-		return NULL;
+	/* warn if buf_len is not a multiple of period_lenas this may leed
+	 * to unexpected latencies for interrupts and thus audiable clicks
+	 */

here too

as this code is new and there are a few more issues that have been seen
it was included here...

quoted

  	/*
-	 * Iterate over all frames, create a control block
-	 * for each frame and link them together.
+	 * allocate the CB chain
+	 * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine

dmaengine drivers use GFP_NOWAIT in these cases

OK we were not sure if this was "permissible" or would result in rare
issues...

I will wait on the generic DT discussion before sending any updates
incorporating both...