On Mon, Mar 31, 2014 at 3:00 PM, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Sat, 2014-03-29 at 20:58 +0530, Srikanth Thokala wrote:
>> This is the driver for the AXI Direct Memory Access (AXI DMA)
>> core, which is a soft Xilinx IP core that provides high-
>> bandwidth direct memory access between memory and AXI4-Stream
>> type target peripherals.
>>
>> This module works on Zynq (ARM Based SoC) and Microblaze platforms.
>
> Few nitpicks below.
>
>>
>> Signed-off-by: Srikanth Thokala <sthokal@xxxxxxxxxx>
>> ---
>> - This driver patch is created on top of earlier series,
>> 1/2 - "dma: Add Xilinx Video DMA DT Binding Documentation"
>> 2/2 - "dma: Add Xilinx AXI Video Direct Memory Access Engine driver support"
>> - Rebased on v3.14.0-rc8
[...]
>> +
>> + for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
>> + chan->seg_v[i].hw.next_desc =
>> + chan->seg_p + sizeof(*chan->seg_v) *
>> + ((i + 1) % XILINX_DMA_NUM_DESCS);
>> + chan->seg_v[i].phys =
>> + chan->seg_p + sizeof(*chan->seg_v) *
>> + (i % XILINX_DMA_NUM_DESCS);
>
> i can't be higher than NUM_DESCS - 1, thus, i % XILINX_DMA_NUM_DESCS =
> i.
I will correct it, Thanks.
>
>> + list_add_tail(&chan->seg_v[i].node, &chan->free_seg_list);
>> + }
>> +
>> + dma_cookie_init(dchan);
>> + return 0;
>> +}
>> +
[...]
>> +
>> +/**
>> + * xilinx_dma_is_running - Check if DMA channel is running
>> + * @chan: Driver specific DMA channel
>> + *
>> + * Return: '1' if running, '0' if not.
>
> true, false.
Ok.
>
>> + */
>> +static bool xilinx_dma_is_running(struct xilinx_dma_chan *chan)
>> +{
>> + return !(dma_ctrl_read(chan, XILINX_DMA_REG_STATUS) &
>> + XILINX_DMA_SR_HALTED_MASK) &&
>> + (dma_ctrl_read(chan, XILINX_DMA_REG_CONTROL) &
>> + XILINX_DMA_CR_RUNSTOP_MASK);
>> +}
>> +
>> +/**
>> + * xilinx_dma_is_idle - Check if DMA channel is idle
>> + * @chan: Driver specific DMA channel
>> + *
>> + * Return: '1' if idle, '0' if not.
>
> Ditto.
Ok.
>
>> + */
>> +static bool xilinx_dma_is_idle(struct xilinx_dma_chan *chan)
>> +{
>> + return dma_ctrl_read(chan, XILINX_DMA_REG_STATUS) &
>> + XILINX_DMA_SR_IDLE_MASK;
>> +}
>> +
>> +/**
>> + * xilinx_dma_halt - Halt DMA channel
[...]
>> + goto error;
>> +
>> + /*
>> + * Calculate the maximum number of bytes to transfer,
>> + * making sure it is less than the hw limit
>> + */
>> + copy = min((size_t)(sg_dma_len(sg) - sg_used),
>> + (size_t)XILINX_DMA_MAX_TRANS_LEN);
>
> min_t(size_t, ...)
Ok, I will correct it.
>
>
>> + hw = &(segment->hw);
>
> Seems useless parentheses.
Ok.
>
>> +
>> + /* Fill in the descriptor */
>> + hw->buf_addr = sg_dma_address(sg) + sg_used;
>> +
>> + hw->control = copy;
>> +
>> + if ((direction == DMA_MEM_TO_DEV) && app_w)
>> + memcpy(hw->app, app_w,
>> + sizeof(u32) * XILINX_DMA_NUM_APP_WORDS);
>> +
>> + /* For the first DMA_MEM_TO_DEV transfer, set SOP */
>> + if (!i)
>> + if (direction == DMA_MEM_TO_DEV)
>> + hw->control |= XILINX_DMA_BD_SOP;
>
> You may group previous conditions like
> if (direction = ...) {
> if (app_w)
> ...
> if (!i)
> ...
> }
>
> But it's up to you.
I will organize them, Thanks.
>
>> +
>> + sg_used += copy;
>> +
>> + /*
>> + * Insert the segment into the descriptor segments
>> + * list.
>> + */
>> + list_add_tail(&segment->node, &desc->segments);
>> + }
>> + }
>> +
[...]
>> + return -EINVAL;
>> + }
>> +
>> + /* find the IRQ line, if it exists in the device tree */
>
> Maybe first letter should be capital across all comments in the code?
Ok, I will correct them in all places, if any.
Srikanth
[...]
>
>
> --
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Intel Finland Oy
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
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
