Re: [PATCH v5 4/6] dmaengine: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver

[Vinod Koul <vkoul@kernel.org>]

On 28-05-20, 05:52, Laurent Pinchart wrote:

> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -0,0 +1,1554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx ZynqMP DPDMA Engine driver
> + *
> + * Copyright (C) 2015 - 2019 Xilinx, Inc.

2020 please

> +static struct xilinx_dpdma_tx_desc *
> +xilinx_dpdma_chan_alloc_tx_desc(struct xilinx_dpdma_chan *chan)
> +{
> +	struct xilinx_dpdma_tx_desc *tx_desc;
> +
> +	tx_desc = kzalloc(sizeof(*tx_desc), GFP_KERNEL);

GFP_NOWAIT please, this is called from a prep call so needs to be atomic
context

> +static int xilinx_dpdma_config(struct dma_chan *dchan,
> +			       struct dma_slave_config *config)
> +{
> +	struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (config->direction != DMA_MEM_TO_DEV)
> +		return -EINVAL;

sorry but direction is deprecated and supposed to be remove, can you
please remove this
> +
> +	/*
> +	 * The destination address doesn't need to be specified as the DPDMA is
> +	 * hardwired to the destination (the DP controller). The transfer
> +	 * width, burst size and port window size are thus meaningless, they're
> +	 * fixed both on the DPDMA side and on the DP controller side.
> +	 */
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	/* Can't reconfigure a running channel. */
> +	if (chan->running) {
> +		ret = -EBUSY;
> +		goto unlock;
> +	}

why does this part matter? The configuration is passed here and should
be applied to next descriptor submitted, channel can be busy.

> +
> +	/*
> +	 * Abuse the slave_id to indicate that the channel is part of a video
> +	 * group.
> +	 */

Of course, what does video grp mean here? 

> +	if (chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)
> +		chan->video_group = config->slave_id != 0;

so only thing we care here is slave_id? What about dma burst parameters?
-- 
~Vinod