Re: [PATCH] mtd: m25p80: Add Power Management support

[Brian Norris <computersforpeace@gmail.com>]

On Wed, Dec 11, 2013 at 04:19:30PM +0800, Hou Zhiqiang wrote:
> Add PM support using callback function suspend and resume in .driver of
> spi_driver.
> 
> Signed-off-by: Hou Zhiqiang <b48286@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..b0c2b8c 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -66,6 +66,8 @@
>  
>  /* Used for Spansion flashes only. */
>  #define	OPCODE_BRWR		0x17	/* Bank register write */
> +#define	OPCODE_DP		0xb9	/* Enter deep power down mode */
> +#define	OPCODE_RES		0xab	/* Exit deep power down mode */

Where did you get these opcodes from? They are not in the Spansion
datasheets I'm reading. And in fact, they are overloaded as RES (Read
Electronic Signature, 0xab) and Bank Register Access (0xb9) in the
datasheet I'm looking at. So this patch is wrong.

Also, can you describe the purpose of these "deep power down" modes?
I've never seen PM states where the *flash* needs to be put into a lower
power mode. Typically the flash is pretty low-power when idle, and it
may even be powered off completely when the system enters a lower-power
state. Anyway, please describe why this patch is needed.

>  
>  /* Status Register bits. */
>  #define	SR_WIP			1	/* Write in progress */
> @@ -1128,11 +1130,46 @@ static int m25p_remove(struct spi_device *spi)
>  	return mtd_device_unregister(&flash->mtd);
>  }
>  
> +#ifdef CONFIG_PM
> +static int m25p_suspend(struct device *dev, pm_message_t mesg)
> +{
> +	struct m25p *flash = dev_get_drvdata(dev);
> +	int ret;
> +
> +	flash->command[0] = OPCODE_DP;

As mentioned above, this opcode is not recognized by many flash
supported in this driver. So we might want one or more of the following:

 (1) to assign different suspend/resume opcodes for use in
     m25p_suspend/resume
 (2) to provide over-loadable callbacks so that different flash could
     use different suspend/resume routines

And of course, we need to avoid sending these commands at all to
unsupported flash.

> +	mutex_lock(&flash->lock);
> +	/* Wait until finished previous write/erase command. */
> +	ret = wait_till_ready(flash);
> +	if (ret) {
> +		mutex_unlock(&flash->lock);
> +		return ret;
> +	}
> +	ret = spi_write(flash->spi, flash->command, 1);
> +	mutex_unlock(&flash->lock);
> +
> +	return ret;
> +}
> +
> +static int m25p_resume(struct device *dev)
> +{
> +	struct m25p *flash = dev_get_drvdata(dev);
> +	int ret;
> +
> +	flash->command[0] = OPCODE_RES;
> +	ret = spi_write(flash->spi, flash->command, 1);
> +
> +	return ret;
> +}
> +#endif /* CONFIG_PM */
>  
>  static struct spi_driver m25p80_driver = {
>  	.driver = {
>  		.name	= "m25p80",
>  		.owner	= THIS_MODULE,
> +#ifdef CONFIG_PM
> +		.suspend = m25p_suspend,
> +		.resume = m25p_resume,
> +#endif
>  	},
>  	.id_table	= m25p_ids,
>  	.probe	= m25p_probe,

Brian