Re: [PATCH 1/3] iio: adc: stm32-adc: move self-calibration to prepare routine

[Jonathan Cameron <jic23@kernel.org>]

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.

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,