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;
Subject: Re: [PATCH 1/3] iio: adc: stm32-adc: move self-calibration to prepare
 routine
To:     Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
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>
References: <1542708752-26163-1-git-send-email-fabrice.gasnier@st.com>
 <1542708752-26163-2-git-send-email-fabrice.gasnier@st.com>
 <20181125130339.61430399@archlinux> <20181125131426.07055b30@archlinux>
From:   Fabrice Gasnier <fabrice.gasnier@st.com>
Message-ID: <e65c0f72-6270-1381-597b-9c100d9a189a@st.com>
Date:   Mon, 26 Nov 2018 11:41:00 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20181125131426.07055b30@archlinux>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/25/18 2:14 PM, Jonathan Cameron wrote:
> 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.

Hi Jonathan,

Many thanks.

Best Regards,
Fabrice
> 
> 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,  
>>
>