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

From: Sinan Kaya
Date: Wed Nov 04 2015 - 21:22:56 EST

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

So, the Freescale driver was written before change went effective. I
guess in 2011 DMA Engine drivers should use issue pending.
Please, refactor since this behaviour is expected.


+ * 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->;

No point to do it here. It should be done on the function that
actually starts the transfer (see issue pending).

comment above

See above as well.


+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);

+ /* 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
+ rc = dma_set_mask_and_coherent(&pdev->dev,
+ }
+ if (rc)
+ goto dmafree;

Maybe move these two lines inside previous condition?


+ dmadev->lldev = hidma_ll_init(dmadev->,
+ dmadev->nr_descriptors, dmadev->dev_trca,
+ dmadev->dev_evca, dmadev->evridx);
+ if (!dmadev->lldev) {
+ 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.

IRQ is slightly different in workflow. In most cases, unfortunately,
there is no achievement by devm_ variant.
At least I know two for now. One of them is DMA Engine slave drivers,
though I didn't notice if you are using tasklet's here.
Otherwise it's okay.

I'm keeping it as it is for maintenance reasons.

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
Please read the FAQ at