Re: [PATCH v4 2/2] dma: sprd: Add Spreadtrum DMA driver

[Vinod Koul]

On Tue, Oct 10, 2017 at 01:23:01PM +0800, Baolin Wang wrote:
> +++ b/drivers/dma/sprd-dma.c
> @@ -0,0 +1,988 @@
> +/*
> + * Copyright (C) 2017 Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)

MIT? See comment for MODULE_LICENSE


> +/* SPRD_DMA_CHN_CFG register definition */
> +#define SPRD_DMA_CHN_EN			BIT(0)
> +#define SPRD_DMA_WAIT_BDONE		24

24 decimal? Why not BIT or GENMASK?

> +#define SPRD_DMA_DONOT_WAIT_BDONE	1

BIT()

> +static void sprd_dma_chn_update(struct sprd_dma_chn *schan, u32 reg,
> +				u32 mask, u32 val)
> +{
> +	u32 orig = readl(schan->chn_base + reg);
> +	u32 tmp;
> +
> +	tmp = orig & ~mask;
> +	tmp |= val & mask;

how about:

	tmp = (orig & ~mask) | val;
> +
> +	writel(tmp, schan->chn_base + reg);
> +}

...

> +static enum sprd_dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *schan)
> +{
> +	u32 intc_sts = readl(schan->chn_base + SPRD_DMA_CHN_INTC) &
> +		       SPRD_DMA_CHN_INT_STS;
> +
> +	switch (intc_sts) {
> +	case SPRD_DMA_CFGERR_INT_STS:
> +		return SPRD_DMA_CFGERR_INT;
> +
> +	case SPRD_DMA_LIST_INT_STS:
> +		return SPRD_DMA_LIST_INT;
> +
> +	case SPRD_DMA_TRSC_INT_STS:
> +		return SPRD_DMA_TRANS_INT;
> +
> +	case SPRD_DMA_BLK_INT_STS:
> +		return SPRD_DMA_BLK_INT;
> +
> +	case SPRD_DMA_FRAG_INT_STS:
> +		return SPRD_DMA_FRAG_INT;
> +
> +	default:
> +		return SPRD_DMA_NO_INT;

should we not log this as error and say we are falling back?

> +static void sprd_dma_free_chan_resources(struct dma_chan *chan)
> +{
> +	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&schan->vc.lock, flags);
> +	sprd_dma_stop(schan);

shouldn't the channel be stopped already?

> +static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> +					  dma_cookie_t cookie,
> +					  struct dma_tx_state *txstate)
> +{
> +	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> +	struct virt_dma_desc *vd;
> +	unsigned long flags;
> +	enum dma_status ret;
> +	u32 pos;
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +
> +	spin_lock_irqsave(&schan->vc.lock, flags);
> +	vd = vchan_find_desc(&schan->vc, cookie);
> +	if (vd) {

pls check txstate being valid. No point continuing if that is not valid

> +struct dma_async_tx_descriptor *sprd_dma_prep_dma_memcpy(struct dma_chan *chan,
> +							 dma_addr_t dest,
> +							 dma_addr_t src,
> +							 size_t len,
> +							 unsigned long flags)

the argument can be moved to two line, it will help readablity.

> +static int sprd_dma_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sprd_dma_dev *sdev;
> +	struct sprd_dma_chn *dma_chn;
> +	struct resource *res;
> +	u32 chn_count;
> +	int ret, i;
> +
> +	ret = of_property_read_u32(np, "#dma-channels", &chn_count);

why not device_property_read_u32()?

> +	if (ret) {
> +		dev_err(&pdev->dev, "get dma channels count failed\n");
> +		return ret;
> +	}
> +
> +	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev) +
> +			    sizeof(*dma_chn) * chn_count,
> +			    GFP_KERNEL);
> +	if (!sdev)
> +		return -ENOMEM;
> +
> +	sdev->clk = devm_clk_get(&pdev->dev, "enable");
> +	if (IS_ERR(sdev->clk)) {
> +		dev_err(&pdev->dev, "get enable clock failed\n");
> +		return PTR_ERR(sdev->clk);
> +	}
> +
> +	/* ashb clock is optional for AGCP DMA */
> +	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
> +	if (IS_ERR(sdev->ashb_clk))
> +		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
> +
> +	/*
> +	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP
> +	 * DMA controller, it can do not request the irq to save system power

it can or do not, either would make sense but not both

> +	 * without resuming system by DMA interrupts. Thus the DMA interrupts
> +	 * property should be optional.
> +	 */
> +	sdev->irq = platform_get_irq(pdev, 0);
> +	if (sdev->irq > 0) {
> +		ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
> +				       0, "sprd_dma", (void *)sdev);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "request dma irq failed\n");
> +			return ret;
> +		}
> +	} else {
> +		dev_warn(&pdev->dev, "no interrupts for the dma controller\n");
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sdev->glb_base = devm_ioremap_nocache(&pdev->dev, res->start,
> +					      resource_size(res));
> +	if (!sdev->glb_base)
> +		return -ENOMEM;
> +
> +	dma_cap_set(DMA_MEMCPY, sdev->dma_dev.cap_mask);
> +	sdev->total_chns = chn_count;
> +	sdev->dma_dev.chancnt = chn_count;
> +	INIT_LIST_HEAD(&sdev->dma_dev.channels);
> +	INIT_LIST_HEAD(&sdev->dma_dev.global_node);
> +	sdev->dma_dev.dev = &pdev->dev;
> +	sdev->dma_dev.device_alloc_chan_resources = sprd_dma_alloc_chan_resources;
> +	sdev->dma_dev.device_free_chan_resources = sprd_dma_free_chan_resources;
> +	sdev->dma_dev.device_tx_status = sprd_dma_tx_status;
> +	sdev->dma_dev.device_issue_pending = sprd_dma_issue_pending;
> +	sdev->dma_dev.device_prep_dma_memcpy = sprd_dma_prep_dma_memcpy;
> +	sdev->dma_dev.device_pause = sprd_dma_pause;
> +	sdev->dma_dev.device_resume = sprd_dma_resume;
> +	sdev->dma_dev.device_terminate_all = sprd_dma_terminate_all;
> +
> +	for (i = 0; i < chn_count; i++) {
> +		dma_chn = &sdev->channels[i];
> +		dma_chn->chn_num = i;
> +		dma_chn->cur_desc = NULL;
> +		/* get each channel's registers base address. */
> +		dma_chn->chn_base = (void __iomem *)
> +			((unsigned long)sdev->glb_base +

the casts look pretty bad here, why are we casting it this way?

> +static int __maybe_unused sprd_dma_runtime_resume(struct device *dev)
> +{
> +	struct sprd_dma_dev *sdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = sprd_dma_enable(sdev);
> +	if (ret) {
> +		dev_err(sdev->dma_dev.dev, "enable dma failed\n");
> +		return ret;
> +	}
> +
> +	return 0;

return ret

we can avoid the two returns that way

> +MODULE_LICENSE("GPL v2");

and here only GPL v2, so which one is correct?

-- 
~Vinod