| Message ID | 20180803072016.21544-8-sjhuang@iluvatar.ai (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Use dmaenginem_async_device_register to simplify code | expand |
On 2018-08-03 10:19, Huang Shijie wrote: > Use dmaenginem_async_device_register to simplify the code: > remove dma_async_device_unregister > > Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> > --- > drivers/dma/ti/omap-dma.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c > index a4a931ddf6f6..085748c6eb67 100644 > --- a/drivers/dma/ti/omap-dma.c > +++ b/drivers/dma/ti/omap-dma.c > @@ -1566,7 +1566,7 @@ static int omap_dma_probe(struct platform_device *pdev) > } > } > > - rc = dma_async_device_register(&od->ddev); > + rc = dmaenginem_async_device_register(&od->ddev); Why it is dmaenginem_async_device_register() and not aligned other resource managed functions (devm_* dmam_*), like devm_dma_async_device_register() and in dmaenginem_async_device_register() what is the 'm' in dmaenginem ? DMAengine Managed? > if (rc) { > pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n", > rc); > @@ -1584,7 +1584,6 @@ static int omap_dma_probe(struct platform_device *pdev) > of_dma_simple_xlate, &omap_dma_info); > if (rc) { > pr_warn("OMAP-DMA: failed to register DMA controller\n"); > - dma_async_device_unregister(&od->ddev); > omap_dma_free(od); > } > } > @@ -1606,8 +1605,6 @@ static int omap_dma_remove(struct platform_device *pdev) > irq = platform_get_irq(pdev, 1); > devm_free_irq(&pdev->dev, irq, od); > > - dma_async_device_unregister(&od->ddev); > - I think this is a bad idea in general. We need to unregister the dma-device before we clean up and free resources. > if (!od->legacy) { > /* Disable all interrupts */ > omap_dma_glbl_write(od, IRQENABLE_L0, 0); > I'm sorry to say, but it is a NACK from me. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 03, 2018 at 10:47:30AM +0300, Peter Ujfalusi wrote: > > > On 2018-08-03 10:19, Huang Shijie wrote: > > Use dmaenginem_async_device_register to simplify the code: > > remove dma_async_device_unregister > > > > Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> > > --- > > drivers/dma/ti/omap-dma.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c > > index a4a931ddf6f6..085748c6eb67 100644 > > --- a/drivers/dma/ti/omap-dma.c > > +++ b/drivers/dma/ti/omap-dma.c > > @@ -1566,7 +1566,7 @@ static int omap_dma_probe(struct platform_device *pdev) > > } > > } > > > > - rc = dma_async_device_register(&od->ddev); > > + rc = dmaenginem_async_device_register(&od->ddev); > > Why it is dmaenginem_async_device_register() and not aligned other > resource managed functions (devm_* dmam_*), like > devm_dma_async_device_register() Vinod prefer the this dmaenginem_async_device_register.. > > and in dmaenginem_async_device_register() what is the 'm' in dmaenginem ? > DMAengine Managed? Yes. > > > if (rc) { > > pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n", > > rc); > > @@ -1584,7 +1584,6 @@ static int omap_dma_probe(struct platform_device *pdev) > > of_dma_simple_xlate, &omap_dma_info); > > if (rc) { > > pr_warn("OMAP-DMA: failed to register DMA controller\n"); > > - dma_async_device_unregister(&od->ddev); > > omap_dma_free(od); > > } > > } > > @@ -1606,8 +1605,6 @@ static int omap_dma_remove(struct platform_device *pdev) > > irq = platform_get_irq(pdev, 1); > > devm_free_irq(&pdev->dev, irq, od); > > > > - dma_async_device_unregister(&od->ddev); > > - > > I think this is a bad idea in general. > We need to unregister the dma-device before we clean up and free resources. okay, thanks for pointing here. We can drop this patch now... > > > if (!od->legacy) { > > /* Disable all interrupts */ > > omap_dma_glbl_write(od, IRQENABLE_L0, 0); > > > > I'm sorry to say, but it is a NACK from me. No problem. Thanks Huang Shijie -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03-08-18, 10:47, Peter Ujfalusi wrote: > > > On 2018-08-03 10:19, Huang Shijie wrote: > > Use dmaenginem_async_device_register to simplify the code: > > remove dma_async_device_unregister > > > > Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> > > --- > > drivers/dma/ti/omap-dma.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c > > index a4a931ddf6f6..085748c6eb67 100644 > > --- a/drivers/dma/ti/omap-dma.c > > +++ b/drivers/dma/ti/omap-dma.c > > @@ -1566,7 +1566,7 @@ static int omap_dma_probe(struct platform_device *pdev) > > } > > } > > > > - rc = dma_async_device_register(&od->ddev); > > + rc = dmaenginem_async_device_register(&od->ddev); > > Why it is dmaenginem_async_device_register() and not aligned other > resource managed functions (devm_* dmam_*), like > devm_dma_async_device_register() > > and in dmaenginem_async_device_register() what is the 'm' in dmaenginem ? > DMAengine Managed? yup, as you rightly said we could have done devm_dmaengine... or like few others do dmaenginem_ Yes it makes a little odd API but am trying to move away from dma_ to dmaengine_ for everything new..
diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c index a4a931ddf6f6..085748c6eb67 100644 --- a/drivers/dma/ti/omap-dma.c +++ b/drivers/dma/ti/omap-dma.c @@ -1566,7 +1566,7 @@ static int omap_dma_probe(struct platform_device *pdev) } } - rc = dma_async_device_register(&od->ddev); + rc = dmaenginem_async_device_register(&od->ddev); if (rc) { pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n", rc); @@ -1584,7 +1584,6 @@ static int omap_dma_probe(struct platform_device *pdev) of_dma_simple_xlate, &omap_dma_info); if (rc) { pr_warn("OMAP-DMA: failed to register DMA controller\n"); - dma_async_device_unregister(&od->ddev); omap_dma_free(od); } } @@ -1606,8 +1605,6 @@ static int omap_dma_remove(struct platform_device *pdev) irq = platform_get_irq(pdev, 1); devm_free_irq(&pdev->dev, irq, od); - dma_async_device_unregister(&od->ddev); - if (!od->legacy) { /* Disable all interrupts */ omap_dma_glbl_write(od, IRQENABLE_L0, 0);
Use dmaenginem_async_device_register to simplify the code: remove dma_async_device_unregister Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> --- drivers/dma/ti/omap-dma.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)