From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755209AbbIAILb (ORCPT ); Tue, 1 Sep 2015 04:11:31 -0400 Received: from eu-smtp-delivery-143.mimecast.com ([146.101.78.143]:48471 "EHLO eu-smtp-delivery-143.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752741AbbIAIL1 convert rfc822-to-8bit (ORCPT ); Tue, 1 Sep 2015 04:11:27 -0400 Date: Tue, 1 Sep 2015 16:11:17 +0800 From: Huang Shijie To: Nicholas Mc Guire Cc: Han Xu , dwmw2@infradead.org, computersforpeace@gmail.com, boris.brezillon@free-electrons.com, fabio.estevam@freescale.com, hofrat@osadl.org, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org, vinod.koul@intel.com, dan.j.williams@intel.com, dmaengine@vger.kernel.org Subject: Re: [PATCH v3 1/6] mtd: nand: gpmi: add gpmi dsm supend/resume support Message-ID: <20150901081117.GA14749@free64-pc.asiapac.arm.com> References: <1440790365-28072-1-git-send-email-b45815@freescale.com> <1440790365-28072-2-git-send-email-b45815@freescale.com> <20150829091740.GC5594@opentech.at> MIME-Version: 1.0 In-Reply-To: <20150829091740.GC5594@opentech.at> User-Agent: Mutt/1.5.23 (2014-03-12) X-OriginalArrivalTime: 01 Sep 2015 08:11:22.0996 (UTC) FILETIME=[CA56EB40:01D0E48D] X-MC-Unique: WJLi-7O1QdG4FcGcMUYi0Q-1 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Aug 29, 2015 at 11:17:40AM +0200, Nicholas Mc Guire wrote: > On Fri, 28 Aug 2015, Han Xu wrote: > > > From: Huang Shijie > > > > i.MX6SX supports deep sleep mode(DSM) that may turn off GPMI/BCH power > > during suspend, add gpmi nand suspend/resume function to release DMA > > channel in suspend function and re-init GPMI/BCH controller during > > resume function. > > > > Just a question regarding gpmi_pm_resume() errror reporting > Is there a reason that the retrun value of gpmi_extra_init() is not checked ? > gpmi_extra_init() can retrun -ENOMEM or -EINVAL via return enable_edo_mode(), > while the -EINVAL would produce a dev_err message, -ENOMEM does not so maybe > this also wants to include a if(ret) {...} ? - something like the below > > > Although it is not necessary to restore GPMI/BCH registers value for > > i.MX6QDL, the code doesn't distinguish different platforms to keep the > > code simple. > > > > Signed-off-by: Huang Shijie > > Signed-off-by: Han Xu > > --- > > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 45 +++++++++++++++++++++++++++++++++- > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > index 1b8f350..dfd0ba1 100644 > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > @@ -1,7 +1,7 @@ > > /* > > * Freescale GPMI NAND Flash Driver > > * > > - * Copyright (C) 2010-2011 Freescale Semiconductor, Inc. > > + * Copyright (C) 2010-2015 Freescale Semiconductor, Inc. > > * Copyright (C) 2008 Embedded Alley Solutions, Inc. > > * > > * This program is free software; you can redistribute it and/or modify > > @@ -2036,9 +2036,52 @@ static int gpmi_nand_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static int gpmi_pm_suspend(struct device *dev) > > +{ > > + struct gpmi_nand_data *this = dev_get_drvdata(dev); > > + > > + release_dma_channels(this); > > + return 0; > > +} > > + > > +static int gpmi_pm_resume(struct device *dev) > > +{ > > + struct gpmi_nand_data *this = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = acquire_dma_channels(this); > > + if (ret < 0) > > + return ret; > > + > > + /* re-init the GPMI registers */ > > + this->flags &= ~GPMI_TIMING_INIT_OK; > > + ret = gpmi_init(this); > > + if (ret) { > > + dev_err(this->dev, "Error setting GPMI : %d\n", ret); > > + return ret; > > + } > > + > > + /* re-init the BCH registers */ > > + ret = bch_set_geometry(this); > > + if (ret) { > > + dev_err(this->dev, "Error setting BCH : %d\n", ret); > > + return ret; > > + } > > + > > + /* re-init others */ > > + gpmi_extra_init(this); > > report failure of enable_edo_mode() ? > > if (ret) { > dev_err(this->dev, "Error enabling EDO mode : %d\n", ret); > return ret; > } I think it's better that we do not report the failure of the enable_edo_mode() here. If we do so, the gpmi may cannot work when the enable_edo_mode() fails. But if we do not report it, the gpmi still can work. thanks Huang Shijie