Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 25 Nov 2018 13:14:26 +0000
From:   Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
To:     Fabrice Gasnier <fabrice.gasnier@st.com>
Cc:     <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <mcoquelin.stm32@gmail.com>,
        <alexandre.torgue@st.com>, <linux-iio@vger.kernel.org>,
        <lars@metafoo.de>, <knaack.h@gmx.de>, <pmeerw@pmeerw.net>,
        <linux-stm32@st-md-mailman.stormreply.com>
Subject: Re: [PATCH 1/3] iio: adc: stm32-adc: move self-calibration to
 prepare routine
Message-ID: <20181125131426.07055b30@archlinux>
In-Reply-To: <20181125130339.61430399@archlinux>
References: <1542708752-26163-1-git-send-email-fabrice.gasnier@st.com>
        <1542708752-26163-2-git-send-email-fabrice.gasnier@st.com>
        <20181125130339.61430399@archlinux>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 25 Nov 2018 13:03:39 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 20 Nov 2018 11:12:30 +0100
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 
> > Move self-calibration routine to prepare routine.
> > - This is precursor patch to ease power management handling.
> > - This also allow to factorize few error cases (error handling).
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>  
> one trivial point inline.  Otherwise seems a sensible bit of refactoring.
Given this was the only 'issue' I found in the whole set I've
just applied it with that changed.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/stm32-adc.c | 59 ++++++++++++++++++---------------------------
> >  1 file changed, 24 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 3784118..dca8733 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -199,11 +199,13 @@ struct stm32_adc_trig_info {
> >   * @calfact_s: Calibration offset for single ended channels
> >   * @calfact_d: Calibration offset in differential
> >   * @lincalfact: Linearity calibration factor
> > + * @calibrated: Indicates calibration status
> >   */
> >  struct stm32_adc_calib {
> >  	u32			calfact_s;
> >  	u32			calfact_d;
> >  	u32			lincalfact[STM32H7_LINCALFACT_NUM];
> > +	bool			calibrated;
> >  };
> >  
> >  /**
> > @@ -251,7 +253,6 @@ struct stm32_adc_regspec {
> >   * @trigs:		external trigger sources
> >   * @clk_required:	clock is required
> >   * @has_vregready:	vregready status flag presence
> > - * @selfcalib:		optional routine for self-calibration
> >   * @prepare:		optional prepare routine (power-up, enable)
> >   * @start_conv:		routine to start conversions
> >   * @stop_conv:		routine to stop conversions
> > @@ -264,7 +265,6 @@ struct stm32_adc_cfg {
> >  	struct stm32_adc_trig_info	*trigs;
> >  	bool clk_required;
> >  	bool has_vregready;
> > -	int (*selfcalib)(struct stm32_adc *);
> >  	int (*prepare)(struct stm32_adc *);
> >  	void (*start_conv)(struct stm32_adc *, bool dma);
> >  	void (*stop_conv)(struct stm32_adc *);
> > @@ -777,6 +777,7 @@ static void stm32h7_adc_disable(struct stm32_adc *adc)
> >  /**
> >   * stm32h7_adc_read_selfcalib() - read calibration shadow regs, save result
> >   * @adc: stm32 adc instance
> > + * Note: Must be called once ADC is enabled, so LINCALRDYW[1..6] are writable
> >   */
> >  static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> >  {
> > @@ -784,11 +785,6 @@ static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> >  	int i, ret;
> >  	u32 lincalrdyw_mask, val;
> >  
> > -	/* Enable adc so LINCALRDYW1..6 bits are writable */
> > -	ret = stm32h7_adc_enable(adc);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Read linearity calibration */
> >  	lincalrdyw_mask = STM32H7_LINCALRDYW6;
> >  	for (i = STM32H7_LINCALFACT_NUM - 1; i >= 0; i--) {
> > @@ -801,7 +797,7 @@ static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> >  						   100, STM32_ADC_TIMEOUT_US);
> >  		if (ret) {
> >  			dev_err(&indio_dev->dev, "Failed to read calfact\n");
> > -			goto disable;
> > +			return ret;
> >  		}
> >  
> >  		val = stm32_adc_readl(adc, STM32H7_ADC_CALFACT2);
> > @@ -817,11 +813,9 @@ static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> >  	adc->cal.calfact_s >>= STM32H7_CALFACT_S_SHIFT;
> >  	adc->cal.calfact_d = (val & STM32H7_CALFACT_D_MASK);
> >  	adc->cal.calfact_d >>= STM32H7_CALFACT_D_SHIFT;
> > +	adc->cal.calibrated = true;
> >  
> > -disable:
> > -	stm32h7_adc_disable(adc);
> > -
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -898,9 +892,9 @@ static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
> >  #define STM32H7_ADC_CALIB_TIMEOUT_US		100000
> >  
> >  /**
> > - * stm32h7_adc_selfcalib() - Procedure to calibrate ADC (from power down)
> > + * stm32h7_adc_selfcalib() - Procedure to calibrate ADC
> >   * @adc: stm32 adc instance
> > - * Exit from power down, calibrate ADC, then return to power down.
> > + * Note: Must be called once ADC is out of power down.
> >   */
> >  static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >  {
> > @@ -908,9 +902,8 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >  	int ret;
> >  	u32 val;
> >  
> > -	ret = stm32h7_adc_exit_pwr_down(adc);
> > -	if (ret)
> > -		return ret;
> > +	if (adc->cal.calibrated)
> > +		return adc->cal.calibrated;  
> return true seems more logical given this is a boolean.
> 
> >  
> >  	/*
> >  	 * Select calibration mode:
> > @@ -927,7 +920,7 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >  					   STM32H7_ADC_CALIB_TIMEOUT_US);
> >  	if (ret) {
> >  		dev_err(&indio_dev->dev, "calibration failed\n");
> > -		goto pwr_dwn;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -944,18 +937,13 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >  					   STM32H7_ADC_CALIB_TIMEOUT_US);
> >  	if (ret) {
> >  		dev_err(&indio_dev->dev, "calibration failed\n");
> > -		goto pwr_dwn;
> > +		goto out;
> >  	}
> >  
> > +out:
> >  	stm32_adc_clr_bits(adc, STM32H7_ADC_CR,
> >  			   STM32H7_ADCALDIF | STM32H7_ADCALLIN);
> >  
> > -	/* Read calibration result for future reference */
> > -	ret = stm32h7_adc_read_selfcalib(adc);
> > -
> > -pwr_dwn:
> > -	stm32h7_adc_enter_pwr_down(adc);
> > -
> >  	return ret;
> >  }
> >  
> > @@ -972,19 +960,28 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >   */
> >  static int stm32h7_adc_prepare(struct stm32_adc *adc)
> >  {
> > -	int ret;
> > +	int calib, ret;
> >  
> >  	ret = stm32h7_adc_exit_pwr_down(adc);
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = stm32h7_adc_selfcalib(adc);
> > +	if (ret < 0)
> > +		goto pwr_dwn;
> > +	calib = ret;
> > +
> >  	stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
> >  
> >  	ret = stm32h7_adc_enable(adc);
> >  	if (ret)
> >  		goto pwr_dwn;
> >  
> > -	ret = stm32h7_adc_restore_selfcalib(adc);
> > +	/* Either restore or read calibration result for future reference */
> > +	if (calib)
> > +		ret = stm32h7_adc_restore_selfcalib(adc);
> > +	else
> > +		ret = stm32h7_adc_read_selfcalib(adc);
> >  	if (ret)
> >  		goto disable;
> >  
> > @@ -1880,12 +1877,6 @@ static int stm32_adc_probe(struct platform_device *pdev)
> >  		goto err_clk_disable;
> >  	stm32_adc_set_res(adc);
> >  
> > -	if (adc->cfg->selfcalib) {
> > -		ret = adc->cfg->selfcalib(adc);
> > -		if (ret)
> > -			goto err_clk_disable;
> > -	}
> > -
> >  	ret = stm32_adc_chan_of_init(indio_dev);
> >  	if (ret < 0)
> >  		goto err_clk_disable;
> > @@ -1961,7 +1952,6 @@ static int stm32_adc_remove(struct platform_device *pdev)
> >  	.regs = &stm32h7_adc_regspec,
> >  	.adc_info = &stm32h7_adc_info,
> >  	.trigs = stm32h7_adc_trigs,
> > -	.selfcalib = stm32h7_adc_selfcalib,
> >  	.start_conv = stm32h7_adc_start_conv,
> >  	.stop_conv = stm32h7_adc_stop_conv,
> >  	.prepare = stm32h7_adc_prepare,
> > @@ -1974,7 +1964,6 @@ static int stm32_adc_remove(struct platform_device *pdev)
> >  	.adc_info = &stm32h7_adc_info,
> >  	.trigs = stm32h7_adc_trigs,
> >  	.has_vregready = true,
> > -	.selfcalib = stm32h7_adc_selfcalib,
> >  	.start_conv = stm32h7_adc_start_conv,
> >  	.stop_conv = stm32h7_adc_stop_conv,
> >  	.prepare = stm32h7_adc_prepare,  
>