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

[Vinod Koul <vkoul@kernel.org>]

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