[v8,3/7] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs

[Vinod Koul <vkoul@kernel.org>]

On 17-08-18, 10:54, Wen He wrote:
> NXP Queue DMA controller(qDMA) on Layerscape SoCs supports channel
> virtuallization by allowing DMA jobs to be enqueued into different

typo virtuallization

> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>

all these of_ headers, are all required?

> +static inline u64
> +qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf)
> +{
> +	return le64_to_cpu(ccdf->data) & (U64_MAX >> 24);

so what is this *supposed* to do, data is u64 right?

> +static inline void
> +qdma_desc_addr_set64(struct fsl_qdma_format *ccdf, u64 addr)
> +{
> +	ccdf->addr_hi = upper_32_bits(addr);
> +	ccdf->addr_lo = cpu_to_le32(lower_32_bits(addr));
> +}
> +
> +static inline u64
> +qdma_ccdf_get_queue(const struct fsl_qdma_format *ccdf)
> +{
> +	return ccdf->cfg8b_w1 & U8_MAX;

why not return u8?

> +static struct fsl_qdma_queue *fsl_qdma_alloc_queue_resources(
> +					struct platform_device *pdev,
> +					unsigned int queue_num)
> +{
> +	struct fsl_qdma_queue *queue_head, *queue_temp;
> +	int ret, i;
> +	unsigned int queue_size[FSL_QDMA_QUEUE_MAX];

it would help in readability if this is preceding, inverted christmas
tree :)

> +
> +	if (queue_num > FSL_QDMA_QUEUE_MAX)
> +		queue_num = FSL_QDMA_QUEUE_MAX;
> +	queue_head = devm_kcalloc(&pdev->dev, queue_num,
> +				sizeof(*queue_head), GFP_KERNEL);
> +	if (!queue_head)
> +		return NULL;
> +
> +	ret = device_property_read_u32_array(&pdev->dev, "queue-sizes",
> +					queue_size, queue_num);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't get queue-sizes.\n");
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < queue_num; i++) {
> +		if (queue_size[i] > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX ||
> +			    queue_size[i] < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN) {
> +			dev_err(&pdev->dev, "Get wrong queue-sizes.\n");
> +			return NULL;
> +		}
> +		queue_temp = queue_head + i;
> +		queue_temp->cq = dma_alloc_coherent(&pdev->dev,
> +						sizeof(struct fsl_qdma_format) *
> +						queue_size[i],
> +						&queue_temp->bus_addr,
> +						GFP_KERNEL);
> +		if (!queue_temp->cq) {
> +			devm_kfree(&pdev->dev, queue_head);

why do you want to do that?

> +static struct fsl_qdma_queue *fsl_qdma_prep_status_queue(
> +						struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct fsl_qdma_queue *status_head;
> +	unsigned int status_size;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "status-sizes", &status_size);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't get status-sizes.\n");
> +		return NULL;
> +	}
> +	if (status_size > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX
> +			|| status_size < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN) {
> +		dev_err(&pdev->dev, "Get wrong status_size.\n");
> +		return NULL;
> +	}
> +	status_head = devm_kzalloc(&pdev->dev, sizeof(*status_head),
> +								GFP_KERNEL);

this is bad on my eyes! How about?

        status_head = devm_kzalloc(&pdev->dev,
                                  sizeof(*status_head), GFP_KERNEL);

for here and few other format style you used. running checkpatch with
--strict option would help as well

> +static int fsl_qdma_queue_transfer_complete(struct fsl_qdma_engine *fsl_qdma)
> +{
> +	struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> +	struct fsl_qdma_queue *fsl_status = fsl_qdma->status;
> +	struct fsl_qdma_queue *temp_queue;
> +	struct fsl_qdma_comp *fsl_comp;
> +	struct fsl_qdma_format *status_addr;
> +	struct fsl_qdma_format *csgf_src;
> +	struct fsl_pre_status pre;
> +	void __iomem *block = fsl_qdma->block_base;
> +	u32 reg, i, count;
> +	bool duplicate;
> +
> +	memset(&pre, 0, sizeof(struct fsl_pre_status));
> +	count = FSL_QDMA_MAX_SIZE;
> +
> +	while (count--) {
> +		duplicate = 0;
> +
> +		reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQSR);
> +		if (reg & FSL_QDMA_BSQSR_QE)
> +			return 0;
> +
> +		status_addr = fsl_status->virt_head;
> +		if (qdma_ccdf_get_queue(status_addr) == pre.queue &&
> +			qdma_ccdf_addr_get64(status_addr) == pre.addr)
> +			duplicate = 1;

bad indents :(

> +
> +		i = qdma_ccdf_get_queue(status_addr);
> +		pre.queue = qdma_ccdf_get_queue(status_addr);
> +		pre.addr = qdma_ccdf_addr_get64(status_addr);
> +		temp_queue = fsl_queue + i;
> +		spin_lock(&temp_queue->queue_lock);
> +		if (list_empty(&temp_queue->comp_used)) {
> +			if (!duplicate) {
> +				spin_unlock(&temp_queue->queue_lock);
> +				return -EAGAIN;
> +			}
> +		} else {
> +			fsl_comp = list_first_entry(&temp_queue->comp_used,
> +							struct fsl_qdma_comp,
> +							list);
> +			csgf_src = fsl_comp->virt_addr + 2;
> +			if (fsl_comp->bus_addr + 16 != pre.addr) {
> +				if (duplicate) {
> +					spin_unlock(&temp_queue->queue_lock);
> +					return -EAGAIN;
> +				}
> +			}
> +		}
> +
> +			if (duplicate) {
> +				reg = qdma_readl(fsl_qdma, block +
> +						FSL_QDMA_BSQMR);

this code block indent seems complete wrong or something is really
messed up here, not aligning with else above!

> +static irqreturn_t fsl_qdma_error_handler(int irq, void *dev_id)
> +{
> +	struct fsl_qdma_engine *fsl_qdma = dev_id;
> +	unsigned int intr;
> +	void __iomem *status = fsl_qdma->status_base;
> +
> +	intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
> +
> +	if (intr)
> +		dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error!\n");
> +
> +	/* Clear all error conditions and interrupts. */
> +	qdma_writel(fsl_qdma, FSL_QDMA_DEDR_CLEAR, status + FSL_QDMA_DEDR);

clear even if intr is not set?

> +static enum dma_status fsl_qdma_tx_status(struct dma_chan *chan,
> +		dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> +	enum dma_status ret;
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	if (ret == DMA_COMPLETE || !txstate)
> +		return ret;
> +
> +	return ret;

        return dma_cookie_status()

or use dma_cookie_status as your status fn?