Re: [PATCH V5 2/9] dmaengine: bcm2835-dma: add suspend/resume pm support

[Vinod Koul <vkoul@kernel.org>]

On 02-12-24, 19:51, Stefan Wahren wrote:
> Hi Vinod,
> 
> Am 02.12.24 um 17:52 schrieb Vinod Koul:
> > On 25-10-24, 12:36, Stefan Wahren wrote:
> > > bcm2835-dma provides the service to others, so it should
> > > suspend late and resume early.
> > > 
> > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> > > ---
> > >   drivers/dma/bcm2835-dma.c | 30 ++++++++++++++++++++++++++++++
> > >   1 file changed, 30 insertions(+)
> > > 
> > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> > > index e1b92b4d7b05..647dda9f3376 100644
> > > --- a/drivers/dma/bcm2835-dma.c
> > > +++ b/drivers/dma/bcm2835-dma.c
> > > @@ -875,6 +875,35 @@ static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec,
> > >   	return chan;
> > >   }
> > > 
> > > +static int bcm2835_dma_suspend_late(struct device *dev)
> > > +{
> > > +	struct bcm2835_dmadev *od = dev_get_drvdata(dev);
> > > +	struct bcm2835_chan *c, *next;
> > > +
> > > +	list_for_each_entry_safe(c, next, &od->ddev.channels,
> > > +				 vc.chan.device_node) {
> > > +		void __iomem *chan_base = c->chan_base;
> > > +
> > > +		if (readl(chan_base + BCM2835_DMA_ADDR)) {
> > > +			dev_warn(dev, "Suspend is prevented by chan %d\n",
> > > +				 c->ch);
> > > +			return -EBUSY;
> > > +		}
> > Can you help understand how this helps by logging... we are not adding
> > anything except checking this and resume is NOP as well!
> My intention of this patch is just to make sure, that no DMA transfer is
> in progress during late_suspend. So i followed the implementation of
> fsldma.c
> 
> Additionally i added this warning mostly to know if this ever occurs.
> But i wasn't able to trigger.

Okay in the case I dont think it is a abd idea. But the patch
description should mention that add a warning while suspending if
channels are active or something like that.
Patch title and description should mention this..

> 
> Should i drop the warning and make resume callback = NULL?

Yes that would be better as well, no point in having dummy code

-- 
~Vinod