Hi Vinod, I send those series patchs(V5) on June 25, 2019. I haven't received any comments yet. Their current state is "Not Applicable", so please let me know what I need to do next. Thanks very much for your comments. Patch link: https://patchwork.kernel.org/patch/11015035/ https://patchwork.kernel.org/patch/11015033/ Best Regards, Peng >-----Original Message----- >From: Vinod Koul <vkoul@xxxxxxxxxx> >Sent: 2019年6月25日 0:46 >To: Peng Ma <peng.ma@xxxxxxx> >Cc: dan.j.williams@xxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; >linux-kernel@xxxxxxxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx >Subject: [EXT] Re: [V4 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA >controller driver for Layerscape SoCs > >Caution: EXT Email > >On 13-06-19, 10:13, Peng Ma wrote: >> DPPA2(Data Path Acceleration Architecture 2) qDMA supports channel >> virtualization by allowing DMA > >typo virtualization > >> jobs to be enqueued into different frame queues. >> Core can initiate a DMA transaction by preparing a frame >> descriptor(FD) for each DMA job and enqueuing this job to a frame >> queue. through a hardware portal. The qDMA > ^^^ >why this full stop? > >> +static struct dpaa2_qdma_comp * >> +dpaa2_qdma_request_desc(struct dpaa2_qdma_chan *dpaa2_chan) { >> + struct dpaa2_qdma_comp *comp_temp = NULL; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags); >> + if (list_empty(&dpaa2_chan->comp_free)) { >> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags); >> + comp_temp = kzalloc(sizeof(*comp_temp), GFP_NOWAIT); >> + if (!comp_temp) >> + goto err; >> + comp_temp->fd_virt_addr = >> + dma_pool_alloc(dpaa2_chan->fd_pool, >GFP_NOWAIT, >> + &comp_temp->fd_bus_addr); >> + if (!comp_temp->fd_virt_addr) >> + goto err_comp; >> + >> + comp_temp->fl_virt_addr = >> + dma_pool_alloc(dpaa2_chan->fl_pool, >GFP_NOWAIT, >> + &comp_temp->fl_bus_addr); >> + if (!comp_temp->fl_virt_addr) >> + goto err_fd_virt; >> + >> + comp_temp->desc_virt_addr = >> + dma_pool_alloc(dpaa2_chan->sdd_pool, >GFP_NOWAIT, >> + >&comp_temp->desc_bus_addr); >> + if (!comp_temp->desc_virt_addr) >> + goto err_fl_virt; >> + >> + comp_temp->qchan = dpaa2_chan; >> + return comp_temp; >> + } >> + >> + comp_temp = list_first_entry(&dpaa2_chan->comp_free, >> + struct dpaa2_qdma_comp, list); >> + list_del(&comp_temp->list); >> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags); >> + >> + comp_temp->qchan = dpaa2_chan; >> + >> + return comp_temp; >> + >> +err_fl_virt: > >no err logs? how will you know what went wrong? > >> +static enum >> +dma_status dpaa2_qdma_tx_status(struct dma_chan *chan, >> + dma_cookie_t cookie, >> + struct dma_tx_state *txstate) { >> + return dma_cookie_status(chan, cookie, txstate); > >why not set dma_cookie_status as this callback? > >> +static int __cold dpaa2_qdma_setup(struct fsl_mc_device *ls_dev) { >> + struct dpaa2_qdma_priv_per_prio *ppriv; >> + struct device *dev = &ls_dev->dev; >> + struct dpaa2_qdma_priv *priv; >> + u8 prio_def = DPDMAI_PRIO_NUM; >> + int err = -EINVAL; >> + int i; >> + >> + priv = dev_get_drvdata(dev); >> + >> + priv->dev = dev; >> + priv->dpqdma_id = ls_dev->obj_desc.id; >> + >> + /* Get the handle for the DPDMAI this interface is associate with */ >> + err = dpdmai_open(priv->mc_io, 0, priv->dpqdma_id, >&ls_dev->mc_handle); >> + if (err) { >> + dev_err(dev, "dpdmai_open() failed\n"); >> + return err; >> + } >> + dev_info(dev, "Opened dpdmai object successfully\n"); > >this is noise in kernel, consider debug level > >> +static int __cold dpaa2_dpdmai_bind(struct dpaa2_qdma_priv *priv) { >> + int err; >> + int i, num; >> + struct device *dev = priv->dev; >> + struct dpaa2_qdma_priv_per_prio *ppriv; >> + struct dpdmai_rx_queue_cfg rx_queue_cfg; >> + struct fsl_mc_device *ls_dev = to_fsl_mc_device(dev); > >the order is reverse than used in other fn, please stick to one style! >-- >~Vinod
