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?
--
~Vinod
