Re: [PATCH v2 2/2] msm: DMAEngine: Add DMAEngine driver based on old MSM DMA APIs

[Ravi Kumar V <kumarrav@codeaurora.org>]

On 1/17/2012 8:01 PM, Vinod Koul wrote:
> On Fri, 2012-01-06 at 18:17 +0530, Ravi Kumar V wrote:
>> +static int msm_dma_alloc_chan_resources(struct dma_chan *dchan)
>> +{
>> +	struct msm_dma_chan *chan = to_msm_chan(dchan);
>> +
>> +	/* Has this channel already been allocated? */
>> +	if (chan->desc_pool)
>> +		return 1;
> that is _wrong_. This would mean you have allocated 1 descriptor.
> Please read the documentation again.
Yes you are right, i will update in next patch release.
>> +
>> +/*
>> + *  Assignes cookie for each transfer descriptor passed.
>> + *  Returns
>> + *	Assigend cookie for success.
> typo	 ^^^^^^^^^
I will change
>> +
>> +
>> +/*
>> + *  Prepares the transfer descriptors for BOX transaction.
>> + *  Returns
>> + *      address of dma_async_tx_descriptor for success.
>> + *      Pointer of error value for failure.
>> + */
> pls use kernel-doc style for these...
I will update
>> +static struct dma_async_tx_descriptor *msm_dma_prep_box(
>> +		struct dma_chan *dchan,
>> +		struct dma_box_list *dst_box, struct dma_box_list *src_box,
>> +		unsigned int num_list,	unsigned long flags)
>> +{
>> +
>> +/*
>> + *  Controlling the hardware channel like stopping, flushing.
>> + */
>> +static int msm_dma_chan_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>> +							unsigned long arg)
>> +{
>> +	int cmd_type = (int) arg;
>> +
>> +	if (cmd == DMA_TERMINATE_ALL) {
>> +		switch (cmd_type) {
>> +		case GRACEFUL_FLUSH:
>> +				msm_dmov_stop_cmd(chan->chan_id, NULL, 1);
>> +			break;
>> +		case FORCED_FLUSH:
>> +			/*
>> +			 * We treate default as forced flush
>> +			 * so we fall through
>> +			 */
>> +		default:
>> +				msm_dmov_stop_cmd(chan->chan_id, NULL, 0);
>> +			break;
>> +		}
>> +	}
> for other cmds you _should_ not return 0
I will update
>> +	return 0;
>> +}
>> +
>> +static void msm_dma_chan_remove(struct msm_dma_chan *chan)
>> +{
>> +	tasklet_kill(&chan->tasklet);
>> +	list_del(&chan->channel.device_node);
>> +	kfree(chan);
>> +}
>> +
>> +static __devinit int msm_dma_chan_probe(struct msm_dma_device *qdev,
>> +					int channel)
>> +{
>> +	struct msm_dma_chan *chan;
>> +
>> +	chan = kzalloc(sizeof(*chan), GFP_KERNEL);
>> +	if (!chan) {
>> +		dev_err(qdev->dev, "no free memory for DMA channels!\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	spin_lock_init(&chan->lock);
>> +	INIT_LIST_HEAD(&chan->pending_list);
>> +	INIT_LIST_HEAD(&chan->active_list);
>> +
>> +	chan->chan_id = channel;
>> +	chan->completed_cookie = 0;
>> +	chan->channel.cookie = 0;
>> +	chan->max_len = MAX_TRANSFER_LENGTH;
>> +	chan->err = 0;
>> +	qdev->chan[channel] = chan;
>> +	chan->channel.device =&qdev->common;
>> +	chan->dev = qdev->dev;
>> +
>> +	tasklet_init(&chan->tasklet, dma_do_tasklet, (unsigned long)chan);
>> +
>> +	list_add_tail(&chan->channel.device_node,&qdev->common.channels);
>> +	qdev->common.chancnt++;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __devinit msm_dma_probe(struct platform_device *pdev)
>> +{
>> +	struct msm_dma_device *qdev;
>> +	int i;
>> +	int ret = 0;
>> +
>> +	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
> devm_kzalloc pls
I will update.
>> +	if (!qdev) {
>> +		dev_err(&pdev->dev, "Not enough memory for device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	qdev->dev =&pdev->dev;
>> +	INIT_LIST_HEAD(&qdev->common.channels);
>> +	qdev->common.device_alloc_chan_resources =
>> +				msm_dma_alloc_chan_resources;
>> +	qdev->common.device_free_chan_resources =
>> +				msm_dma_free_chan_resources;
>> +	dma_cap_set(DMA_SG, qdev->common.cap_mask);
>> +	dma_cap_set(DMA_BOX, qdev->common.cap_mask);
>> +
>> +	qdev->common.device_prep_dma_sg = msm_dma_prep_sg;
>> +	qdev->common.device_prep_dma_box = msm_dma_prep_box;
>> +	qdev->common.device_issue_pending = msm_dma_issue_pending;
>> +	qdev->common.dev =&pdev->dev;
>> +	qdev->common.device_tx_status = msm_tx_status;
>> +	qdev->common.device_control = msm_dma_chan_control;
>> +
>> +	for (i = SD3_CHAN_START; i<  MSM_DMA_MAX_CHANS_PER_DEVICE; i++) {
>> +		ret = msm_dma_chan_probe(qdev, i);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "channel %d probe failed\n", i);
>> +			goto chan_free;
>> +		}
>> +	}
>> +
>> +	dev_info(&pdev->dev, "registering dma device\n");
>> +
>> +	ret = dma_async_device_register(&qdev->common);
>> +
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Registering Dma device failed\n");
>> +		goto chan_free;
>> +	}
>> +
>> +	dev_set_drvdata(&pdev->dev, qdev);
>> +	return 0;
>> +chan_free:
>> +	for (i = SD3_CHAN_START; i<  MSM_DMA_MAX_CHANS_PER_DEVICE; i++) {
>> +		if (qdev->chan[i])
>> +			msm_dma_chan_remove(qdev->chan[i]);
>> +	}
>> +	kfree(qdev);
> you leak the chan memory allocated
I am freeing chan memory from msm_dma_chan_remove() called above.
>> +	return ret;
>> +}
>> +
>> +static int  __devexit msm_dma_remove(struct platform_device *pdev)
>> +{
>> +	struct msm_dma_device *qdev = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	dma_async_device_unregister(&qdev->common);
>> +
>> +	for (i = SD3_CHAN_START; i<  MSM_DMA_MAX_CHANS_PER_DEVICE; i++) {
>> +		if (qdev->chan[i])
>> +			msm_dma_chan_remove(qdev->chan[i]);
>> +	}
>> +
>> +	dev_set_drvdata(&pdev->dev, NULL);
>> +	kfree(qdev);
> ditto
I am freeing chan memory from msm_dma_chan_remove() called above.
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver msm_dma_driver = {
>> +	.remove = __devexit_p(msm_dma_remove),
>> +	.driver = {
>> +		.name = "msm_dma",
>> +		.owner = THIS_MODULE,
>> +	},
>> +};
>> +
>> +static __init int msm_dma_init(void)
>> +{
>> +	return platform_driver_probe(&msm_dma_driver, msm_dma_probe);
>> +}
>> +subsys_initcall(msm_dma_init);
>> +
>> +static void __exit msm_dma_exit(void)
>> +{
>> +	platform_driver_unregister(&msm_dma_driver);
>> +}
>> +module_exit(msm_dma_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm DMA driver");
>> +MODULE_LICENSE("GPL v2");
> More comments, once I understand what "BOX mode" is?
> Also, if you can add basic driver without box mode, we can merge fairly
> quickly, box mode can come once we understand what we want and how...
We also implemented SG mode using device_prep_dma_sg() but we need to 
pass extra parameter "command configuration" to our HW with every 
descriptor.
Please can you suggest a way to transfer private param to 
device_prep_dma_sg()

>
>


-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.