Re: [V4 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA controller driver for Layerscape SoCs

From: Vinod Koul
Date: Mon Jun 24 2019 - 12:49:10 EST


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