Messages in this thread | | | Date | Fri, 29 Sep 2023 22:35:27 +0200 | Subject | Re: [PATCH V6 1/1] dmaengine: amd: qdma: Add AMD QDMA driver | From | Christophe JAILLET <> |
| |
Le 29/09/2023 à 19:24, Lizhi Hou a écrit : > From: Nishad Saraf <nishads@amd.com> > > Adds driver to enable PCIe board which uses AMD QDMA (the Queue-based > Direct Memory Access) subsystem. For example, Xilinx Alveo V70 AI > Accelerator devices. > https://www.xilinx.com/applications/data-center/v70.html > > The QDMA subsystem is used in conjunction with the PCI Express IP block > to provide high performance data transfer between host memory and the > card's DMA subsystem. >
...
> +static int amd_qdma_probe(struct platform_device *pdev) > +{ > + struct qdma_platdata *pdata = dev_get_platdata(&pdev->dev); > + struct qdma_device *qdev; > + struct resource *res; > + void __iomem *regs; > + int ret; > + > + qdev = devm_kzalloc(&pdev->dev, sizeof(*qdev), GFP_KERNEL); > + if (!qdev) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, qdev); > + qdev->pdev = pdev; > + mutex_init(&qdev->ctxt_lock);
If this was done later, it could simplify some error handling below.
> + > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (!res) { > + qdma_err(qdev, "Failed to get IRQ resource"); > + ret = -ENODEV; > + goto failed; > + } > + qdev->err_irq_idx = pdata->irq_index; > + qdev->queue_irq_start = res->start + 1; > + qdev->queue_irq_num = res->end - res->start; > + > + regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL); > + if (IS_ERR(regs)) { > + ret = PTR_ERR(regs); > + qdma_err(qdev, "Failed to map IO resource, err %d", ret); > + goto failed; > + } > + > + qdev->regmap = devm_regmap_init_mmio(&pdev->dev, regs, > + &qdma_regmap_config); > + if (IS_ERR(qdev->regmap)) { > + ret = PTR_ERR(qdev->regmap); > + qdma_err(qdev, "Regmap init failed, err %d", ret); > + goto failed; > + } > + > + ret = qdma_device_verify(qdev); > + if (ret) > + goto failed; > + > + ret = qdma_get_hw_info(qdev); > + if (ret) > + goto failed; > + > + INIT_LIST_HEAD(&qdev->dma_dev.channels); > + > + ret = qdma_device_setup(qdev); > + if (ret) > + goto failed; > + > + ret = qdma_intr_init(qdev); > + if (ret) { > + qdma_err(qdev, "Failed to initialize IRQs %d", ret); > + return ret;
Should it be "goto failed"?
> + } > + qdev->status |= QDMA_DEV_STATUS_INTR_INIT; > + > + dma_cap_set(DMA_SLAVE, qdev->dma_dev.cap_mask); > + dma_cap_set(DMA_PRIVATE, qdev->dma_dev.cap_mask);
...
> +struct qdma_device { > + struct platform_device *pdev; > + struct dma_device dma_dev; > + struct regmap *regmap; > + struct mutex ctxt_lock; /* protect ctxt registers */ > + const struct qdma_reg_field *rfields; > + const struct qdma_reg *roffs; > + struct qdma_queue *h2c_queues; > + struct qdma_queue *c2h_queues; > + struct qdma_intr_ring *qintr_rings; > + u32 qintr_ring_num; > + u32 qintr_ring_idx; > + u32 chan_num; > + u32 queue_irq_start; > + u32 queue_irq_num; > + u32 err_irq_idx; > + u32 fid; > + u32 status;
Using such a mechanism with this 'status' in the probe and amd_qdma_remove(), apparently only to simplify the error handling of the probe looks really unusual to me.
CJ
> +};
...
| |