On 09/10/17 11:35, Vinod Koul wrote:
> 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
Will do in v2.
>>>> +
>>>> 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
Will do in v2.
>>>> +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
>
Do you mean the system PM functions added in the first patch? These
change in the second patch to re-use the runtime PM functions.
Ed.
--
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
