On Mon, Oct 09, 2017 at 11:09:07AM +0100, Ed Blake wrote:
> On 08/10/17 11:29, Vinod Koul wrote:
> > On Mon, Oct 02, 2017 at 10:44:40AM +0100, Ed Blake wrote:
> >> +static int img_mdc_runtime_suspend(struct device *dev);
> >> +static int img_mdc_runtime_resume(struct device *dev);
> > why fwd declaration?
>
> These functions are called from probe() and remove(). The alternative
> was to move the function definitions before probe, but I preferred to
> keep them with the system PM functions at the end. I can move the
> functions and remote the declarations if you prefer?
I usually like to avoid them, moving code before probe seems okay to me
>
> >> +
> >> static inline u32 mdc_readl(struct mdc_dma *mdma, u32 reg)
> >> {
> >> return readl(mdma->regs + reg);
> >> @@ -730,14 +734,23 @@ static int mdc_slave_config(struct dma_chan *chan,
> >> return 0;
> >> }
> >>
> >> +static int mdc_alloc_chan_resources(struct dma_chan *chan)
> >> +{
> >> + struct mdc_chan *mchan = to_mdc_chan(chan);
> >> + struct device *dev = mdma2dev(mchan->mdma);
> >> +
> >> + return pm_runtime_get_sync(dev);
> >> +}
> >> +
> >> static void mdc_free_chan_resources(struct dma_chan *chan)
> >> {
> >> struct mdc_chan *mchan = to_mdc_chan(chan);
> >> struct mdc_dma *mdma = mchan->mdma;
> >> + struct device *dev = mdma2dev(mdma);
> >>
> >> mdc_terminate_all(chan);
> >> -
> >> mdma->soc->disable_chan(mchan);
> >> + pm_runtime_put(dev);
> >> }
> >>
> >> static irqreturn_t mdc_chan_irq(int irq, void *dev_id)
> >> @@ -883,10 +896,6 @@ static int mdc_dma_probe(struct platform_device *pdev)
> >> if (IS_ERR(mdma->clk))
> >> return PTR_ERR(mdma->clk);
> >>
> >> - ret = clk_prepare_enable(mdma->clk);
> >> - if (ret)
> >> - return ret;
> >> -
> > why is this removed, maybe unrelated to this change?
>
> It's not unrelated; the clock is now being dynamically controlled using
> runtime PM, so should no longer be permanently enabled in probe().
ok, i think we should add this in the changelog
> >> +static int img_mdc_runtime_suspend(struct device *dev)
> >> +{
> >> + struct mdc_dma *mdma = dev_get_drvdata(dev);
> >> +
> >> + clk_disable_unprepare(mdma->clk);
> >> + return 0;
> >> +}
> >> +
> >> +static int img_mdc_runtime_resume(struct device *dev)
> >> +{
> >> + struct mdc_dma *mdma = dev_get_drvdata(dev);
> >> +
> >> + return clk_prepare_enable(mdma->clk);
> >> +}
> > seems similar to the ones in prev, i guess we might be able to optimize
> >
>
> Sorry, what do you mean by 'the ones in prev'?
ones in prev patch, see if you can modularize and use common helpers.. on a
first glance seemed doable
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
