[RFC][PATCH 1/1]nand/denali: Add runtime pm support for denali controller driver

[Dong, Chuanxiao]

Hi
See my comments below.
Thanks
--Chuanxiao

> -----Original Message-----
> From: Arjan van de Ven [mailto:arjan at infradead.org]
> Sent: Tuesday, September 07, 2010 3:53 AM
> To: Dong, Chuanxiao
> Cc: dwmw2 at infradead.org; linux-mtd at lists.infradead.org
> Subject: Re: [RFC][PATCH 1/1]nand/denali: Add runtime pm support for denali
> controller driver
> 
> On Mon, 6 Sep 2010 16:08:38 +0800
> "Chuanxiao.Dong" <chuanxiao.dong at intel.com> wrote:
> 
> 
> > +/* NOW denali NAND controller in MRST has no way to cut power off
> > + * once it is power on, so just let these functions be empty
> > + * */
> 
> this isn't right; you should put the device in PCI D3 state.

This will be update after I got a correct runtime PM framework using.

> 
> > +
> > +static struct dev_pm_ops denali_pm = {
> > +	.runtime_suspend = denali_runtime_suspend,
> > +	.runtime_resume = denali_runtime_resume,
> > +	.runtime_idle = denali_runtime_idle,
> 
> you don't really need a runtime_idle function
> 
> 
> > +};
> > +/* support denali runtime pm */
> > +static void denali_timer_add(struct denali_nand_info *denali)
> 
> and this is where I get very nervous about your patch.
> 
> The runtime pm infrastructure already has timers, you don't need to
> implement one!
> 
> in fact, I think the code is rather not using the runtime PM
> infrastructure right.
> 
> The runtime PM framework offers you a reference counter for activity,
> so what really ought to be done instead of this polling timer, is to
> take a refcount each time something is submitted to the hardware..
> 
> > +	if (chip->state != FL_READY ||
> 
> ... and decremented each time something is completed. This will change
> the chip->state for you... each place chip->state is used is a
> candidate for this proper refcounting.

And this is what I am confusing. Each place chip->state is used is in MTD framework....no in MTD driver
itself. MTD driver never know when it is opened or released.
Each time before hardware has something to do chip->state is already set to be something like READING/
WRITING by MTD framework. And after finishing the hardware work, chip->state does not change. Then
return back to MTD framework. If there are still something for hardware to do, chip->state will not change.
Only when there is nothing to do, MTD framework will set chip->state to be READY after MTD driver finishing
the last hardware work.
That is the process for MTD driver working. It only maintains the hardware working part. Before or after it finishes
the hardware work, the chip->state is always to be READING/WRITING. It never knows when there will have the
first or the last hardware access.
So that is why I add a timer for driver to detect the chip->state. And that is why I think it's better for MTD framework
to do some runtime PM work.

> 
> basically, this timer in the driver is rather misplaced and should be
> replaced by using the refcount from the runtime PM infrastructure
> correctly...... and I suspect your patch gets a LOT simpler as well for
> free.