| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+mB=1Lw5AT4LNXwKckmvs3=BFowVQOKHK0pJHvNLZs2_9LR_A@mail.gmail.com>
Date: Mon, 31 Mar 2014 18:53:08 +0530
From: Srikanth Thokala <sthokal@...inx.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Srikanth Thokala <sthokal@...inx.com>,
"Williams, Dan J" <dan.j.williams@...el.com>,
Vinod Koul <vinod.koul@...el.com>,
Michal Simek <michal.simek@...inx.com>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Jaswinder Singh <jaswinder.singh@...aro.org>,
dmaengine@...r.kernel.org,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] dma: Add Xilinx AXI Direct Memory Access Engine
driver support
On Mon, Mar 31, 2014 at 3:00 PM, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> 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@...inx.com>
>> ---
>> - 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@...ux.intel.com>
> Intel Finland Oy
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists