From mboxrd@z Thu Jan 1 00:00:00 1970 From: kumarrav@codeaurora.org (Ravi Kumar V) Date: Fri, 20 Jan 2012 18:16:12 +0530 Subject: [PATCH v2 2/2] msm: DMAEngine: Add DMAEngine driver based on old MSM DMA APIs In-Reply-To: <1326810701.1540.161.camel@vkoul-udesk3> References: <1325854052-21402-1-git-send-email-kumarrav@codeaurora.org> <1325854052-21402-3-git-send-email-kumarrav@codeaurora.org> <1326810701.1540.161.camel@vkoul-udesk3> Message-ID: <4F196214.30608@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.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.