Re: [PATCH V2 3/3] dma: add Qualcomm Technologies HIDMA channel driver

[Sinan Kaya]

On 11/3/2015 5:10 AM, Andy Shevchenko wrote:
> On Mon, Nov 2, 2015 at 8:07 AM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
> > This patch adds support for hidma engine. The driver
> > consists of two logical blocks. The DMA engine interface
> > and the low-level interface. The hardware only supports
> > memcpy/memset and this driver only support memcpy
> > interface. HW and driver doesn't support slave interface.
>
> > +/* Linux Foundation elects GPLv2 license only.
> > + */
>
> One line?
ok

> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <asm/dma.h>
>
> Do you need this one explicitly?

got rid of it

> > +#include <linux/atomic.h>
> > +#include <linux/pm_runtime.h>
>
> + empty line?
ok
> > +#include <asm/div64.h>
>
> + empty line?
ok
> > +#include "dmaengine.h"
> > +#include "qcom_hidma.h"
> > +
> > +/* Default idle time is 2 seconds. This parameter can
> > + * be overridden by changing the following
> > + * /sys/bus/platform/devices/QCOM8061:<xy>/power/autosuspend_delay_ms
> > + * during kernel boot.
> > + */
ok

> Block comments usually like
> /*
>   * text
>   */

> > +struct hidma_chan {
> > +       bool                            paused;
> > +       bool                            allocated;
> > +       char                            name[16];
>
> So, do you need specific name? There is already one in struct dma_chan.
OK, removed.

> > +/* process completed descriptors */
> > +static void hidma_process_completed(struct hidma_dev *mdma)
> > +{
> > +       dma_cookie_t last_cookie = 0;
> > +       struct hidma_chan *mchan;
> > +       struct hidma_desc *mdesc;
> > +       struct dma_async_tx_descriptor *desc;
> > +       unsigned long irqflags;
> > +       LIST_HEAD(list);
> > +       struct dma_chan *dmach = NULL;
> > +
> > +       list_for_each_entry(dmach, &mdma->ddev.channels,
> > +                       device_node) {
> > +               mchan = to_hidma_chan(dmach);
> > +
Found a bug here now. I should have initialized the list on each 
iteration of the loop.

> > +               /* Get all completed descriptors */
> > +               spin_lock_irqsave(&mchan->lock, irqflags);
> > +               if (!list_empty(&mchan->completed))

Removed this one.

> > +                       list_splice_tail_init(&mchan->completed, &list);
> > +               spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +               if (list_empty(&list))
> > +                       continue;

> Redundant check. It's done in both list_for_each_entry() and
> list_splice_tail_init().

ok
> > +
> > +               /* Execute callbacks and run dependencies */
> > +               list_for_each_entry(mdesc, &list, node) {
> > +                       desc = &mdesc->desc;
> > +
> > +                       spin_lock_irqsave(&mchan->lock, irqflags);
> > +                       dma_cookie_complete(desc);
> > +                       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +                       if (desc->callback &&
> > +                               (hidma_ll_status(mdma->lldev, mdesc->tre_ch)
> > +                               == DMA_COMPLETE))
> > +                               desc->callback(desc->callback_param);
> > +
> > +                       last_cookie = desc->cookie;
> > +                       dma_run_dependencies(desc);
> > +               }
> > +
> > +               /* Free descriptors */
> > +               spin_lock_irqsave(&mchan->lock, irqflags);
> > +               list_splice_tail_init(&list, &mchan->free);
> > +               spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +       }
> > +}
> > +
> > +/*
> > + * Execute all queued DMA descriptors.
> > + * This function is called either on the first transfer attempt in tx_submit
> > + * or from the callback routine when one transfer is finished. It can only be
> > + * called from a single location since both of places check active list to be
> > + * empty and will immediately fill the active list while lock is held.
> > + *
> > + * Following requirements must be met while calling hidma_execute():
> > + *     a) mchan->lock is locked,
> > + *     b) mchan->active list contains multiple entries.
> > + *     c) pm protected
> > + */
> > +static int hidma_execute(struct hidma_chan *mchan)
> > +{
> > +       struct hidma_dev *mdma = mchan->dmadev;
> > +       int rc;
> > +
> > +       if (!hidma_ll_isenabled(mdma->lldev))
> > +               return -ENODEV;
> > +
> > +       /* Start the transfer */
> > +       if (!list_empty(&mchan->active))
> > +               rc = hidma_ll_start(mdma->lldev);
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Called once for each submitted descriptor.
> > + * PM is locked once for each descriptor that is currently
> > + * in execution.
> > + */
> > +static void hidma_callback(void *data)
> > +{
> > +       struct hidma_desc *mdesc = data;
> > +       struct hidma_chan *mchan = to_hidma_chan(mdesc->desc.chan);
> > +       unsigned long irqflags;
> > +       struct dma_device *ddev = mchan->chan.device;
> > +       struct hidma_dev *dmadev = to_hidma_dev(ddev);
> > +       bool queued = false;
> > +
> > +       dev_dbg(dmadev->ddev.dev, "callback: data:0x%p\n", data);
> > +
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +
> > +       if (mdesc->node.next) {
> > +               /* Delete from the active list, add to completed list */
> > +               list_move_tail(&mdesc->node, &mchan->completed);
> > +               queued = true;
> > +       }
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +       hidma_process_completed(dmadev);
> > +
> > +       if (queued) {
> > +               pm_runtime_mark_last_busy(dmadev->ddev.dev);
> > +               pm_runtime_put_autosuspend(dmadev->ddev.dev);
> > +       }
> > +}
> > +
> > +static int hidma_chan_init(struct hidma_dev *dmadev, u32 dma_sig)
> > +{
> > +       struct hidma_chan *mchan;
> > +       struct dma_device *ddev;
> > +
> > +       mchan = devm_kzalloc(dmadev->ddev.dev, sizeof(*mchan), GFP_KERNEL);
> > +       if (!mchan)
> > +               return -ENOMEM;
> > +
> > +       ddev = &dmadev->ddev;
> > +       mchan->dma_sig = dma_sig;
> > +       mchan->dmadev = dmadev;
> > +       mchan->chan.device = ddev;
> > +       dma_cookie_init(&mchan->chan);
> > +
> > +       INIT_LIST_HEAD(&mchan->free);
> > +       INIT_LIST_HEAD(&mchan->prepared);
> > +       INIT_LIST_HEAD(&mchan->active);
> > +       INIT_LIST_HEAD(&mchan->completed);
> > +
> > +       spin_lock_init(&mchan->lock);
> > +       list_add_tail(&mchan->chan.device_node, &ddev->channels);
> > +       dmadev->ddev.chancnt++;
> > +       return 0;
> > +}
> > +
> > +static void hidma_issue_pending(struct dma_chan *dmach)
> > +{
>
> Wrong. It should actually start the transfer. tx_submit() just puts
> the descriptor to a queue.
Depends on the design.

I started from the Freescale driver (mpc512x_dma.c). It follows the same 
model.

I'll just drop the same comment into this code too.


/*
* We are posting descriptors to the hardware as soon as
* they are ready, so this function does nothing.
*/

> > +}
> > +
> > +static enum dma_status hidma_tx_status(struct dma_chan *dmach,
> > +                                       dma_cookie_t cookie,
> > +                                       struct dma_tx_state *txstate)
> > +{
> > +       enum dma_status ret;
> > +       unsigned long irqflags;
> > +       struct hidma_chan *mchan = to_hidma_chan(dmach);
> > +
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
>
> So, what are you protecting here? paused member, right?

yes.

> > +       if (mchan->paused)
> > +               ret = DMA_PAUSED;
> > +       else
> > +               ret = dma_cookie_status(dmach, cookie, txstate);
>
> This one has no need to be under spin lock.
ok, will remove it. Apparently, other drivers are not using locks either 
in this routine.
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +       return ret;
> > +}
> > +
> > +/*
> > + * Submit descriptor to hardware.
> > + * Lock the PM for each descriptor we are sending.
> > + */
> > +static dma_cookie_t hidma_tx_submit(struct dma_async_tx_descriptor *txd)
> > +{
> > +       struct hidma_chan *mchan = to_hidma_chan(txd->chan);
> > +       struct hidma_dev *dmadev = mchan->dmadev;
> > +       struct hidma_desc *mdesc;
> > +       unsigned long irqflags;
> > +       dma_cookie_t cookie;
> > +
> > +       if (!hidma_ll_isenabled(dmadev->lldev))
> > +               return -ENODEV;
> > +
> > +       pm_runtime_get_sync(dmadev->ddev.dev);
>
> No point to do it here. It should be done on the function that
> actually starts the transfer (see issue pending).
comment above

> > +       mdesc = container_of(txd, struct hidma_desc, desc);
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +
> > +       /* Move descriptor to active */
> > +       list_move_tail(&mdesc->node, &mchan->active);
> > +
> > +       /* Update cookie */
> > +       cookie = dma_cookie_assign(txd);
> > +
> > +       hidma_ll_queue_request(dmadev->lldev, mdesc->tre_ch);
> > +       hidma_execute(mchan);
> > +
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +       return cookie;
> > +}
> > +
> > +static int hidma_alloc_chan_resources(struct dma_chan *dmach)
> > +{
> > +       struct hidma_chan *mchan = to_hidma_chan(dmach);
> > +       struct hidma_dev *dmadev = mchan->dmadev;
> > +       int rc = 0;
> > +       struct hidma_desc *mdesc, *tmp;
> > +       unsigned long irqflags;
> > +       LIST_HEAD(descs);
> > +       u32 i;
> > +
> > +       if (mchan->allocated)
> > +               return 0;
> > +
> > +       /* Alloc descriptors for this channel */
> > +       for (i = 0; i < dmadev->nr_descriptors; i++) {
> > +               mdesc = kzalloc(sizeof(struct hidma_desc), GFP_KERNEL);
> > +               if (!mdesc) {
> > +                       dev_err(dmadev->ddev.dev, "Memory allocation error. ");
> > +                       rc = -ENOMEM;
> > +                       break;
> > +               }
> > +               dma_async_tx_descriptor_init(&mdesc->desc, dmach);
> > +               mdesc->desc.flags = DMA_CTRL_ACK;
> > +               mdesc->desc.tx_submit = hidma_tx_submit;
> > +
> > +               rc = hidma_ll_request(dmadev->lldev,
> > +                               mchan->dma_sig, "DMA engine", hidma_callback,
> > +                               mdesc, &mdesc->tre_ch);
> > +               if (rc != 1) {
>
> if (rc < 1) {

I'll fix hidma_ll_request instead and return 0 on success and change 
this line as if (rc)

> > +                       dev_err(dmach->device->dev,
> > +                               "channel alloc failed at %u\n", i);
>
> > +                       kfree(mdesc);
> > +                       break;
> > +               }
> > +               list_add_tail(&mdesc->node, &descs);
> > +       }
> > +
> > +       if (rc != 1) {
>
> if (rc < 1)

Fixed this too
> > +               /* return the allocated descriptors */
> > +               list_for_each_entry_safe(mdesc, tmp, &descs, node) {
> > +                       hidma_ll_free(dmadev->lldev, mdesc->tre_ch);
> > +                       kfree(mdesc);
> > +               }
> > +               return rc;
> > +       }
> > +
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +       list_splice_tail_init(&descs, &mchan->free);
> > +       mchan->allocated = true;
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +       dev_dbg(dmadev->ddev.dev,
> > +               "allocated channel for %u\n", mchan->dma_sig);
> > +       return rc;
> > +}
> > +
> > +static void hidma_free_chan_resources(struct dma_chan *dmach)
> > +{
> > +       struct hidma_chan *mchan = to_hidma_chan(dmach);
> > +       struct hidma_dev *mdma = mchan->dmadev;
> > +       struct hidma_desc *mdesc, *tmp;
> > +       unsigned long irqflags;
> > +       LIST_HEAD(descs);
> > +
> > +       if (!list_empty(&mchan->prepared) ||
> > +               !list_empty(&mchan->active) ||
> > +               !list_empty(&mchan->completed)) {
> > +               /* We have unfinished requests waiting.
> > +                * Terminate the request from the hardware.
> > +                */
> > +               hidma_cleanup_pending_tre(mdma->lldev, 0x77, 0x77);
>
> 0x77 is magic.

Changing with meaningful macros.

> > +
> > +               /* Give enough time for completions to be called. */
> > +               msleep(100);
> > +       }
> > +
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +       /* Channel must be idle */
> > +       WARN_ON(!list_empty(&mchan->prepared));
> > +       WARN_ON(!list_empty(&mchan->active));
> > +       WARN_ON(!list_empty(&mchan->completed));
> > +
> > +       /* Move data */
> > +       list_splice_tail_init(&mchan->free, &descs);
> > +
> > +       /* Free descriptors */
> > +       list_for_each_entry_safe(mdesc, tmp, &descs, node) {
> > +               hidma_ll_free(mdma->lldev, mdesc->tre_ch);
> > +               list_del(&mdesc->node);
> > +               kfree(mdesc);
> > +       }
> > +
> > +       mchan->allocated = 0;
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +       dev_dbg(mdma->ddev.dev, "freed channel for %u\n", mchan->dma_sig);
> > +}
> > +
> > +
> > +static struct dma_async_tx_descriptor *
> > +hidma_prep_dma_memcpy(struct dma_chan *dmach, dma_addr_t dma_dest,
> > +                       dma_addr_t dma_src, size_t len, unsigned long flags)
> > +{
> > +       struct hidma_chan *mchan = to_hidma_chan(dmach);
> > +       struct hidma_desc *mdesc = NULL;
> > +       struct hidma_dev *mdma = mchan->dmadev;
> > +       unsigned long irqflags;
> > +
> > +       dev_dbg(mdma->ddev.dev,
> > +               "memcpy: chan:%p dest:%pad src:%pad len:%zu\n", mchan,
> > +               &dma_dest, &dma_src, len);
> > +
> > +       /* Get free descriptor */
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +       if (!list_empty(&mchan->free)) {
> > +               mdesc = list_first_entry(&mchan->free, struct hidma_desc,
> > +                                       node);
> > +               list_del(&mdesc->node);
> > +       }
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +       if (!mdesc)
> > +               return NULL;
> > +
> > +       hidma_ll_set_transfer_params(mdma->lldev, mdesc->tre_ch,
> > +                       dma_src, dma_dest, len, flags);
> > +
> > +       /* Place descriptor in prepared list */
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +       list_add_tail(&mdesc->node, &mchan->prepared);
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +       return &mdesc->desc;
> > +}
> > +
> > +static int hidma_terminate_all(struct dma_chan *chan)
> > +{
> > +       struct hidma_dev *dmadev;
> > +       LIST_HEAD(head);
> > +       unsigned long irqflags;
> > +       LIST_HEAD(list);
> > +       struct hidma_desc *tmp, *mdesc = NULL;
> > +       int rc = 0;
>
> Useless assignment.

removed.

> > +       struct hidma_chan *mchan;
> > +
> > +       mchan = to_hidma_chan(chan);
> > +       dmadev = to_hidma_dev(mchan->chan.device);
> > +       dev_dbg(dmadev->ddev.dev, "terminateall: chan:0x%p\n", mchan);
> > +
> > +       pm_runtime_get_sync(dmadev->ddev.dev);
> > +       /* give completed requests a chance to finish */
> > +       hidma_process_completed(dmadev);
> > +
> > +       spin_lock_irqsave(&mchan->lock, irqflags);
> > +       list_splice_init(&mchan->active, &list);
> > +       list_splice_init(&mchan->prepared, &list);
> > +       list_splice_init(&mchan->completed, &list);
> > +       spin_unlock_irqrestore(&mchan->lock, irqflags);
> > +
> > +       /* this suspends the existing transfer */
> > +       rc = hidma_ll_pause(dmadev->lldev);
> > +       if (rc) {
> > +               dev_err(dmadev->ddev.dev, "channel did not pause\n");
> > +               goto out;
> > +       }
> > +
> > +       /* return all user requests */
> > +       list_for_each_entry_safe(mdesc, tmp, &list, node) {
> > +               struct dma_async_tx_descriptor  *txd = &mdesc->desc;
> > +               dma_async_tx_callback callback = mdesc->desc.callback;
> > +               void *param = mdesc->desc.callback_param;
> > +               enum dma_status status;
> > +
> > +               dma_descriptor_unmap(txd);
> > +
> > +               status = hidma_ll_status(dmadev->lldev, mdesc->tre_ch);
> > +               /*
> > +                * The API requires that no submissions are done from a
> > +                * callback, so we don't need to drop the lock here
> > +                */
> > +               if (callback && (status == DMA_COMPLETE))
> > +                       callback(param);
> > +
> > +               dma_run_dependencies(txd);
> > +
> > +               /* move myself to free_list */
> > +               list_move(&mdesc->node, &mchan->free);
> > +       }
> > +
> > +       /* reinitialize the hardware */
> > +       rc = hidma_ll_setup(dmadev->lldev);
> > +
> > +out:
> > +       pm_runtime_mark_last_busy(dmadev->ddev.dev);
> > +       pm_runtime_put_autosuspend(dmadev->ddev.dev);
> > +       return rc;
> > +}
> > +
> > +static int hidma_pause(struct dma_chan *chan)
> > +{
> > +       struct hidma_chan *mchan;
> > +       struct hidma_dev *dmadev;
> > +
> > +       mchan = to_hidma_chan(chan);
> > +       dmadev = to_hidma_dev(mchan->chan.device);
> > +       dev_dbg(dmadev->ddev.dev, "pause: chan:0x%p\n", mchan);
> > +
> > +       pm_runtime_get_sync(dmadev->ddev.dev);
>
> Why it's here? Here is nothing to do with the device, move it to _pause().

I'll move it inside the if statement. hidma_ll_pause touches the hardware.

> > +       if (!mchan->paused) {
> > +               if (hidma_ll_pause(dmadev->lldev))
> > +                       dev_warn(dmadev->ddev.dev, "channel did not stop\n");
> > +               mchan->paused = true;
> > +       }
> > +       pm_runtime_mark_last_busy(dmadev->ddev.dev);
> > +       pm_runtime_put_autosuspend(dmadev->ddev.dev);
> > +       return 0;
> > +}
> > +
> > +static int hidma_resume(struct dma_chan *chan)
> > +{
> > +       struct hidma_chan *mchan;
> > +       struct hidma_dev *dmadev;
> > +       int rc = 0;
> > +
> > +       mchan = to_hidma_chan(chan);
> > +       dmadev = to_hidma_dev(mchan->chan.device);
> > +       dev_dbg(dmadev->ddev.dev, "resume: chan:0x%p\n", mchan);
> > +
> > +       pm_runtime_get_sync(dmadev->ddev.dev);
>
> Ditto.

I'll do the samething as pause.

> > +       if (mchan->paused) {
> > +               rc = hidma_ll_resume(dmadev->lldev);
> > +               if (!rc)
> > +                       mchan->paused = false;
> > +               else
> > +                       dev_err(dmadev->ddev.dev,
> > +                                       "failed to resume the channel");
> > +       }
> > +       pm_runtime_mark_last_busy(dmadev->ddev.dev);
> > +       pm_runtime_put_autosuspend(dmadev->ddev.dev);
> > +       return rc;
> > +}
> > +
> > +static irqreturn_t hidma_chirq_handler(int chirq, void *arg)
> > +{
> > +       struct hidma_lldev **lldev_ptr = arg;
> > +       irqreturn_t ret;
> > +       struct hidma_dev *dmadev = to_hidma_dev_from_lldev(lldev_ptr);
> > +
> > +       pm_runtime_get_sync(dmadev->ddev.dev);
>
> Hmm... Do you have shared IRQ line or wakeup able one?
> Otherwise I can't see ways how device can generate interrupts.
> If there is a case other than described, put comment why it might happen.

All interrupts are request driven. HW doesn't send an interrupt by 
itself. I'll put some comment in the code.

> > +       ret = hidma_ll_inthandler(chirq, *lldev_ptr);
> > +       pm_runtime_mark_last_busy(dmadev->ddev.dev);
> > +       pm_runtime_put_autosuspend(dmadev->ddev.dev);
> > +       return ret;
> > +}
> > +
> > +static int hidma_probe(struct platform_device *pdev)
> > +{
> > +       struct hidma_dev *dmadev;
> > +       int rc = 0;
> > +       struct resource *trca_resource;
> > +       struct resource *evca_resource;
> > +       int chirq;
> > +       int current_channel_index = atomic_read(&channel_ref_count);
> > +
> > +       pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
> > +       pm_runtime_use_autosuspend(&pdev->dev);
> > +       pm_runtime_set_active(&pdev->dev);
> > +       pm_runtime_enable(&pdev->dev);
> > +
> > +       trca_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!trca_resource) {
> > +               rc = -ENODEV;
> > +               goto bailout;
> > +       }
> > +
> > +       evca_resource = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +       if (!evca_resource) {
> > +               rc = -ENODEV;
> > +               goto bailout;
> > +       }
>
>
> Consolidate these with devm_ioremap_resource();

ok

> > +
> > +       /* This driver only handles the channel IRQs.
> > +        * Common IRQ is handled by the management driver.
> > +        */
> > +       chirq = platform_get_irq(pdev, 0);
> > +       if (chirq < 0) {
> > +               rc = -ENODEV;
> > +               goto bailout;
> > +       }
> > +
> > +       dmadev = devm_kzalloc(&pdev->dev, sizeof(*dmadev), GFP_KERNEL);
> > +       if (!dmadev) {
> > +               rc = -ENOMEM;
> > +               goto bailout;
> > +       }
> > +
> > +       INIT_LIST_HEAD(&dmadev->ddev.channels);
> > +       spin_lock_init(&dmadev->lock);
> > +       dmadev->ddev.dev = &pdev->dev;
> > +       pm_runtime_get_sync(dmadev->ddev.dev);
> > +
> > +       dma_cap_set(DMA_MEMCPY, dmadev->ddev.cap_mask);
> > +       if (WARN_ON(!pdev->dev.dma_mask)) {
> > +               rc = -ENXIO;
> > +               goto dmafree;
> > +       }
> > +
> > +       dmadev->dev_evca = devm_ioremap_resource(&pdev->dev,
> > +                                               evca_resource);
> > +       if (IS_ERR(dmadev->dev_evca)) {
> > +               rc = -ENOMEM;
> > +               goto dmafree;
> > +       }
> > +
> > +       dmadev->dev_trca = devm_ioremap_resource(&pdev->dev,
> > +                                               trca_resource);
> > +       if (IS_ERR(dmadev->dev_trca)) {
> > +               rc = -ENOMEM;
> > +               goto dmafree;
> > +       }
> > +       dmadev->ddev.device_prep_dma_memcpy = hidma_prep_dma_memcpy;
> > +       dmadev->ddev.device_alloc_chan_resources =
> > +               hidma_alloc_chan_resources;
> > +       dmadev->ddev.device_free_chan_resources = hidma_free_chan_resources;
> > +       dmadev->ddev.device_tx_status = hidma_tx_status;
> > +       dmadev->ddev.device_issue_pending = hidma_issue_pending;
> > +       dmadev->ddev.device_pause = hidma_pause;
> > +       dmadev->ddev.device_resume = hidma_resume;
> > +       dmadev->ddev.device_terminate_all = hidma_terminate_all;
> > +       dmadev->ddev.copy_align = 8;
> > +
> > +       device_property_read_u32(&pdev->dev, "desc-count",
> > +                               &dmadev->nr_descriptors);
> > +
> > +       if (!dmadev->nr_descriptors && nr_desc_prm)
> > +               dmadev->nr_descriptors = nr_desc_prm;
> > +
> > +       if (!dmadev->nr_descriptors)
> > +               goto dmafree;
> > +
> > +       if (current_channel_index > MAX_HIDMA_CHANNELS)
> > +               goto dmafree;
> > +
> > +       dmadev->evridx = -1;
> > +       device_property_read_u32(&pdev->dev, "event-channel", &dmadev->evridx);
> > +
> > +       /* kernel command line override for the guest machine */
> > +       if (event_channel_idx[current_channel_index] != -1)
> > +               dmadev->evridx = event_channel_idx[current_channel_index];
> > +
> > +       if (dmadev->evridx == -1)
> > +               goto dmafree;
> > +
> > +       /* Set DMA mask to 64 bits. */
> > +       rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> > +       if (rc) {
> > +               dev_warn(&pdev->dev, "unable to set coherent mask to 64");
> > +               rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > +       }
> > +       if (rc)
> > +               goto dmafree;
> > +
> > +       dmadev->lldev = hidma_ll_init(dmadev->ddev.dev,
> > +                               dmadev->nr_descriptors, dmadev->dev_trca,
> > +                               dmadev->dev_evca, dmadev->evridx);
> > +       if (!dmadev->lldev) {
> > +               rc = -EPROBE_DEFER;
> > +               goto dmafree;
> > +       }
> > +
> > +       rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, 0,
> > +                             "qcom-hidma", &dmadev->lldev);
>
> Better to use request_irq().

why? I thought we favored managed functions over standalone functions in 
simplify the exit path.

> > +       if (rc)
> > +               goto uninit;
> > +
> > +       INIT_LIST_HEAD(&dmadev->ddev.channels);
> > +       rc = hidma_chan_init(dmadev, 0);
> > +       if (rc)
> > +               goto uninit;
> > +
> > +       rc = dma_selftest_memcpy(&dmadev->ddev);

Thanks for the review.

--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/