Messages in this thread | | | Date | Tue, 26 Apr 2016 22:26:22 +0530 | From | Vinod Koul <> | Subject | Re: [PATCH 03/18] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support |
| |
On Thu, Apr 21, 2016 at 12:04:20PM +0100, Peter Griffin wrote:
> + if (!atomic_read(&fchan->fdev->fw_loaded)) { > + dev_err(fchan->fdev->dev, "%s: fdma fw not loaded\n", __func__); > + return NULL; > + }
so who is loading the fw and setting fw_loaded, it is not set in this patch?
> + if (direction == DMA_DEV_TO_MEM) { > + fchan->cfg.req_ctrl &= ~REQ_CTRL_WNR; > + maxburst = fchan->scfg.src_maxburst; > + width = fchan->scfg.src_addr_width; > + addr = fchan->scfg.src_addr; > + } else if (direction == DMA_MEM_TO_DEV) { > + fchan->cfg.req_ctrl |= REQ_CTRL_WNR; > + maxburst = fchan->scfg.dst_maxburst; > + width = fchan->scfg.dst_addr_width; > + addr = fchan->scfg.dst_addr; > + } else { > + return -EINVAL; > + }
switch please
> + > + fchan->cfg.req_ctrl &= ~REQ_CTRL_OPCODE_MASK; > + if (width == DMA_SLAVE_BUSWIDTH_1_BYTE) > + fchan->cfg.req_ctrl |= REQ_CTRL_OPCODE_LD_ST1; > + else if (width == DMA_SLAVE_BUSWIDTH_2_BYTES) > + fchan->cfg.req_ctrl |= REQ_CTRL_OPCODE_LD_ST2; > + else if (width == DMA_SLAVE_BUSWIDTH_4_BYTES) > + fchan->cfg.req_ctrl |= REQ_CTRL_OPCODE_LD_ST4; > + else if (width == DMA_SLAVE_BUSWIDTH_8_BYTES) > + fchan->cfg.req_ctrl |= REQ_CTRL_OPCODE_LD_ST8; > + else > + return -EINVAL;
here as well
> +static void fill_hw_node(struct st_fdma_hw_node *hw_node, > + struct st_fdma_chan *fchan, > + enum dma_transfer_direction direction) > +{ > + > + if (direction == DMA_MEM_TO_DEV) { > + hw_node->control |= NODE_CTRL_SRC_INCR; > + hw_node->control |= NODE_CTRL_DST_STATIC; > + hw_node->daddr = fchan->cfg.dev_addr; > + } else { > + hw_node->control |= NODE_CTRL_SRC_STATIC; > + hw_node->control |= NODE_CTRL_DST_INCR; > + hw_node->saddr = fchan->cfg.dev_addr; > + }
empty line here at other places too. The code looks very compressed and bit harder to read overall
> + fdesc = st_fdma_alloc_desc(fchan, sg_len); > + if (!fdesc) { > + dev_err(fchan->fdev->dev, "no memory for desc\n"); > + return NULL; > + } > + > + fdesc->iscyclic = false; > + > + for_each_sg(sgl, sg, sg_len, i) { > + hw_node = fdesc->node[i].desc; > + > + hw_node->next = fdesc->node[(i + 1) % sg_len].pdesc; > + hw_node->control = NODE_CTRL_REQ_MAP_DREQ(fchan->dreq_line); > + > + fill_hw_node(hw_node, fchan, direction); > + > + if (direction == DMA_MEM_TO_DEV) > + hw_node->saddr = sg_dma_address(sg); > + else > + hw_node->daddr = sg_dma_address(sg); > + > + hw_node->nbytes = sg_dma_len(sg); > + hw_node->generic.length = sg_dma_len(sg); > + } > + > + /* interrupt at end of last node */ > + hw_node->control |= NODE_CTRL_INT_EON; > + > + return vchan_tx_prep(&fchan->vchan, &fdesc->vdesc, flags);
bunch of this seems similar to cyclic case, can you create common setup routine for these, anyway cyclic is a special cases of slave_sg
> + > + ret = dma_cookie_status(chan, cookie, txstate); > + if (ret == DMA_COMPLETE) > + return ret; > + > + if (!txstate) > + return fchan->status;
why channel status, query is for descriptor
> +static int st_fdma_remove(struct platform_device *pdev) > +{ > + struct st_fdma_dev *fdev = platform_get_drvdata(pdev); > + > + st_fdma_clk_disable(fdev);
and you irq is still enabled and tasklets can be scheduled!!
-- ~Vinod
| |