[2/2] ASoC: DA7219: Implement error check on reg read and write

Commit Message

Akshu Agrawal Dec. 4, 2018, 6:36 p.m. UTC

Failed i2c transaction can lead to failure in reg read or write.
Need to have error check for each read write operation.

Signed-off-by: Akshu Agrawal <[e-mail]>
---
 sound/soc/codecs/da7219.c | 323 +++++++++++++++++++++++++++++++++++-----------
 sound/soc/codecs/da7219.h |   2 +-
 2 files changed, 247 insertions(+), 78 deletions(-)

Comments

Adam Thomson Dec. 4, 2018, 9:16 p.m. UTC | #1

On 04 December 2018 18:36, Akshu Agrawal wrote:

> Failed i2c transaction can lead to failure in reg read or write.
> Need to have error check for each read write operation.
>

I'm not really clear what this gains you. If I2C is broken this won't solve
anything, and you have far deeper problems. Seems to me like a lot of code which
adds almost nothing.
 
> Signed-off-by: Akshu Agrawal <[e-mail]>
> ---
>  sound/soc/codecs/da7219.c | 323 +++++++++++++++++++++++++++++++++++-
> ----------
>  sound/soc/codecs/da7219.h |   2 +-
>  2 files changed, 247 insertions(+), 78 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index
> e46e9f4..6757129 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
> snd_kcontrol *kcontrol,  }
> 
>  /* ALC */
> -static void da7219_alc_calib(struct snd_soc_component *component)
> +static int da7219_alc_calib(struct snd_soc_component *component)
>  {
> -	u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> +	int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> +	int ret;
> 
>  	/* Save current state of mic control register */
>  	mic_ctrl = snd_soc_component_read32(component,
> DA7219_MIC_1_CTRL);
> +	if (mic_ctrl < 0)
> +		return mic_ctrl;
> 
>  	/* Save current state of input mixer control register */
>  	mixin_ctrl = snd_soc_component_read32(component,
> DA7219_MIXIN_L_CTRL);
> +	if (mixin_ctrl < 0)
> +		return mixin_ctrl;
> 
>  	/* Save current state of input ADC control register */
>  	adc_ctrl = snd_soc_component_read32(component,
> DA7219_ADC_L_CTRL);
> +	if (adc_ctrl < 0)
> +		return adc_ctrl;
> 
>  	/* Enable then Mute MIC PGAs */
> -	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> DA7219_MIC_1_AMP_EN_MASK,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_MIC_1_CTRL,
> +			    DA7219_MIC_1_AMP_EN_MASK,
>  			    DA7219_MIC_1_AMP_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = snd_soc_component_update_bits(component,
> DA7219_MIC_1_CTRL,
>  			    DA7219_MIC_1_AMP_MUTE_EN_MASK,
>  			    DA7219_MIC_1_AMP_MUTE_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Enable input mixers unmuted */
> -	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_MIXIN_L_CTRL,
>  			    DA7219_MIXIN_L_AMP_EN_MASK |
>  			    DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
>  			    DA7219_MIXIN_L_AMP_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Enable input filters unmuted */
> -	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_ADC_L_CTRL,
>  			    DA7219_ADC_L_MUTE_EN_MASK |
> DA7219_ADC_L_EN_MASK,
>  			    DA7219_ADC_L_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Perform auto calibration */
> -	snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
>  			    DA7219_ALC_AUTO_CALIB_EN_MASK,
>  			    DA7219_ALC_AUTO_CALIB_EN_MASK);
> +	if (ret < 0)
> +		return ret;
>  	do {
>  		calib_ctrl = snd_soc_component_read32(component,
> DA7219_ALC_CTRL1);
> +		if (calib_ctrl < 0)
> +			return calib_ctrl;
>  	} while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
> 
>  	/* If auto calibration fails, disable DC offset, hybrid ALC */
>  	if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
>  		dev_warn(component->dev,
>  			 "ALC auto calibration failed with overflow\n");
> -		snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
>  				    DA7219_ALC_OFFSET_EN_MASK |
>  				    DA7219_ALC_SYNC_MODE_MASK, 0);
> +		if (ret < 0)
> +			return ret;
>  	} else {
>  		/* Enable DC offset cancellation, hybrid mode */
> -		snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
>  				    DA7219_ALC_OFFSET_EN_MASK |
>  				    DA7219_ALC_SYNC_MODE_MASK,
>  				    DA7219_ALC_OFFSET_EN_MASK |
>  				    DA7219_ALC_SYNC_MODE_MASK);
> +		if (ret < 0)
> +			return ret;
>  	}
> 
>  	/* Restore input filter control register to original state */
> -	snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
> +	ret = snd_soc_component_write(component, DA7219_ADC_L_CTRL,
> adc_ctrl);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Restore input mixer control registers to original state */
> -	snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
> mixin_ctrl);
> +	ret = snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
> +				mixin_ctrl);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Restore MIC control registers to original states */
> -	snd_soc_component_write(component, DA7219_MIC_1_CTRL, mic_ctrl);
> +	ret = snd_soc_component_write(component, DA7219_MIC_1_CTRL,
> mic_ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
>  }
> 
>  static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol, @@ -391,7
> +425,7 @@ static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol,
>  	 * make sure calibrated offsets are updated.
>  	 */
>  	if ((ret == 1) && (da7219->alc_en))
> -		da7219_alc_calib(component);
> +		ret = da7219_alc_calib(component);
> 
>  	return ret;
>  }
> @@ -401,11 +435,14 @@ static int da7219_alc_sw_put(struct snd_kcontrol
> *kcontrol,  {
>  	struct snd_soc_component *component =
> snd_soc_kcontrol_component(kcontrol);
>  	struct da7219_priv *da7219 =
> snd_soc_component_get_drvdata(component);
> +	int ret;
> 
> 
>  	/* Force ALC offset calibration if enabling ALC */
>  	if ((ucontrol->value.integer.value[0]) && (!da7219->alc_en)) {
> -		da7219_alc_calib(component);
> +		ret = da7219_alc_calib(component);
> +		if (ret < 0)
> +			return ret;
>  		da7219->alc_en = true;
>  	} else {
>  		da7219->alc_en = false;
> @@ -813,16 +850,20 @@ static int da7219_dai_event(struct
> snd_soc_dapm_widget *w,
>  					return ret;
>  				}
>  			} else {
> -				snd_soc_component_update_bits(component,
> +				ret =
> snd_soc_component_update_bits(component,
> 
> DA7219_DAI_CLK_MODE,
> 
> DA7219_DAI_CLK_EN_MASK,
> 
> DA7219_DAI_CLK_EN_MASK);
> +				if (ret < 0)
> +					return ret;
>  			}
>  		}
> 
>  		/* PC synchronised to DAI */
> -		snd_soc_component_update_bits(component,
> DA7219_PC_COUNT,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_PC_COUNT,
>  				    DA7219_PC_FREERUN_MASK, 0);
> +		if (ret < 0)
> +			return ret;
> 
>  		/* Slave mode, if SRM not enabled no need for status checks */
>  		pll_ctrl = snd_soc_component_read32(component,
> DA7219_PLL_CTRL); @@ -846,19 +887,23 @@ static int da7219_dai_event(struct
> snd_soc_dapm_widget *w,
>  		return 0;
>  	case SND_SOC_DAPM_POST_PMD:
>  		/* PC free-running */
> -		snd_soc_component_update_bits(component,
> DA7219_PC_COUNT,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_PC_COUNT,
>  				    DA7219_PC_FREERUN_MASK,
>  				    DA7219_PC_FREERUN_MASK);
> +		if (ret < 0)
> +			return ret;
> 
>  		/* Disable DAI clks if in master mode */
>  		if (da7219->master) {
>  			if (da7219->dai_clks)
>  				clk_disable_unprepare(da7219->dai_clks);
>  			else
> -				snd_soc_component_update_bits(component,
> +				ret =
> snd_soc_component_update_bits(component,
> 
> DA7219_DAI_CLK_MODE,
> 
> DA7219_DAI_CLK_EN_MASK,
>  							      0);
> +				if (ret < 0)
> +					return ret;
>  		}
> 
>  		return 0;
> @@ -887,6 +932,7 @@ static int da7219_mixout_event(struct
> snd_soc_dapm_widget *w,  {
>  	struct snd_soc_component *component =
> snd_soc_dapm_to_component(w->dapm);
>  	u8 hp_ctrl, min_gain_mask;
> +	int ret;
> 
>  	switch (w->reg) {
>  	case DA7219_MIXOUT_L_CTRL:
> @@ -904,15 +950,20 @@ static int da7219_mixout_event(struct
> snd_soc_dapm_widget *w,
>  	switch (event) {
>  	case SND_SOC_DAPM_PRE_PMD:
>  		/* Enable minimum gain on HP to avoid pops */
> -		snd_soc_component_update_bits(component, hp_ctrl,
> min_gain_mask,
> -				    min_gain_mask);
> +		ret = snd_soc_component_update_bits(component, hp_ctrl,
> +				    min_gain_mask, min_gain_mask);
> +		if (ret < 0)
> +			return ret;
> 
>  		msleep(DA7219_MIN_GAIN_DELAY);
> 
>  		break;
>  	case SND_SOC_DAPM_POST_PMU:
>  		/* Remove minimum gain on HP */
> -		snd_soc_component_update_bits(component, hp_ctrl,
> min_gain_mask, 0);
> +		ret = snd_soc_component_update_bits(component, hp_ctrl,
> +				    min_gain_mask, 0);
> +		if (ret < 0)
> +			return ret;
> 
>  		break;
>  	}
> @@ -925,21 +976,29 @@ static int da7219_gain_ramp_event(struct
> snd_soc_dapm_widget *w,  {
>  	struct snd_soc_component *component =
> snd_soc_dapm_to_component(w->dapm);
>  	struct da7219_priv *da7219 =
> snd_soc_component_get_drvdata(component);
> +	int ret;
> 
>  	switch (event) {
>  	case SND_SOC_DAPM_PRE_PMU:
>  	case SND_SOC_DAPM_PRE_PMD:
>  		/* Ensure nominal gain ramping for DAPM sequence */
> -		da7219->gain_ramp_ctrl =
> -			snd_soc_component_read32(component,
> DA7219_GAIN_RAMP_CTRL);
> -		snd_soc_component_write(component,
> DA7219_GAIN_RAMP_CTRL,
> -			      DA7219_GAIN_RAMP_RATE_NOMINAL);
> +		da7219->gain_ramp_ctrl =
> snd_soc_component_read32(component,
> +						DA7219_GAIN_RAMP_CTRL);
> +		if (da7219->gain_ramp_ctrl < 0)
> +			return da7219->gain_ramp_ctrl;
> +		 ret = snd_soc_component_write(component,
> +				DA7219_GAIN_RAMP_CTRL,
> +				DA7219_GAIN_RAMP_RATE_NOMINAL);
> +		if (ret < 0)
> +			return ret;
>  		break;
>  	case SND_SOC_DAPM_POST_PMU:
>  	case SND_SOC_DAPM_POST_PMD:
>  		/* Restore previous gain ramp settings */
> -		snd_soc_component_write(component,
> DA7219_GAIN_RAMP_CTRL,
> +		ret = snd_soc_component_write(component,
> DA7219_GAIN_RAMP_CTRL,
>  			      da7219->gain_ramp_ctrl);
> +		if (ret < 0)
> +			return ret;
>  		break;
>  	}
> 
> @@ -1177,13 +1236,17 @@ static int da7219_set_dai_sysclk(struct snd_soc_dai
> *codec_dai,
> 
>  	switch (clk_id) {
>  	case DA7219_CLKSRC_MCLK_SQR:
> -		snd_soc_component_update_bits(component,
> DA7219_PLL_CTRL,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_PLL_CTRL,
>  				    DA7219_PLL_MCLK_SQR_EN_MASK,
>  				    DA7219_PLL_MCLK_SQR_EN_MASK);
> +		if (ret < 0)
> +			return ret;
>  		break;
>  	case DA7219_CLKSRC_MCLK:
> -		snd_soc_component_update_bits(component,
> DA7219_PLL_CTRL,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_PLL_CTRL,
>  				    DA7219_PLL_MCLK_SQR_EN_MASK, 0);
> +		if (ret < 0)
> +			return ret;
>  		break;
>  	default:
>  		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
> @@ -1219,6 +1282,7 @@ int da7219_set_pll(struct snd_soc_component
> *component, int source, unsigned int
>  	u8 pll_frac_top, pll_frac_bot, pll_integer;
>  	u32 freq_ref;
>  	u64 frac_div;
> +	int ret;
> 
>  	/* Verify 2MHz - 54MHz MCLK provided, and set input divider */
>  	if (da7219->mclk_rate < 2000000) {
> @@ -1252,9 +1316,11 @@ int da7219_set_pll(struct snd_soc_component
> *component, int source, unsigned int
>  	switch (source) {
>  	case DA7219_SYSCLK_MCLK:
>  		pll_ctrl |= DA7219_PLL_MODE_BYPASS;
> -		snd_soc_component_update_bits(component,
> DA7219_PLL_CTRL,
> +		ret = snd_soc_component_update_bits(component,
> DA7219_PLL_CTRL,
>  				    DA7219_PLL_INDIV_MASK |
>  				    DA7219_PLL_MODE_MASK, pll_ctrl);
> +		if (ret < 0)
> +			return ret;
>  		return 0;
>  	case DA7219_SYSCLK_PLL:
>  		pll_ctrl |= DA7219_PLL_MODE_NORMAL;
> @@ -1275,12 +1341,23 @@ int da7219_set_pll(struct snd_soc_component
> *component, int source, unsigned int
>  	pll_frac_bot = (frac_div) & DA7219_BYTE_MASK;
> 
>  	/* Write PLL config & dividers */
> -	snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
> pll_frac_top);
> -	snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
> pll_frac_bot);
> -	snd_soc_component_write(component, DA7219_PLL_INTEGER,
> pll_integer);
> -	snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
> +	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
> +				pll_frac_top);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
> +				pll_frac_bot);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_write(component, DA7219_PLL_INTEGER,
> +				pll_integer);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
>  			    DA7219_PLL_INDIV_MASK |
> DA7219_PLL_MODE_MASK,
>  			    pll_ctrl);
> +	if (ret < 0)
> +		return ret;
> 
>  	return 0;
>  }
> @@ -1304,6 +1381,7 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
> *codec_dai, unsigned int fmt)
>  	struct snd_soc_component *component = codec_dai->component;
>  	struct da7219_priv *da7219 =
> snd_soc_component_get_drvdata(component);
>  	u8 dai_clk_mode = 0, dai_ctrl = 0;
> +	int ret;
> 
>  	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>  	case SND_SOC_DAIFMT_CBM_CFM:
> @@ -1379,12 +1457,16 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
> *codec_dai, unsigned int fmt)
>  	/* By default 64 BCLKs per WCLK is supported */
>  	dai_clk_mode |= DA7219_DAI_BCLKS_PER_WCLK_64;
> 
> -	snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
>  			    DA7219_DAI_BCLKS_PER_WCLK_MASK |
>  			    DA7219_DAI_CLK_POL_MASK |
> DA7219_DAI_WCLK_POL_MASK,
>  			    dai_clk_mode);
> -	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> DA7219_DAI_FORMAT_MASK,
> -			    dai_ctrl);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> +			    DA7219_DAI_FORMAT_MASK, dai_ctrl);
> +	if (ret < 0)
> +		return ret;
> 
>  	return 0;
>  }
> @@ -1398,15 +1480,22 @@ static int da7219_set_dai_tdm_slot(struct
> snd_soc_dai *dai,
>  	u8 dai_bclks_per_wclk;
>  	u16 offset;
>  	u32 frame_size;
> +	int ret;
> 
>  	/* No channels enabled so disable TDM, revert to 64-bit frames */
>  	if (!tx_mask) {
> -		snd_soc_component_update_bits(component,
> DA7219_DAI_TDM_CTRL,
> +		ret = snd_soc_component_update_bits(component,
> +				    DA7219_DAI_TDM_CTRL,
>  				    DA7219_DAI_TDM_CH_EN_MASK |
>  				    DA7219_DAI_TDM_MODE_EN_MASK, 0);
> -		snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
> +		if (ret < 0)
> +			return ret;
> +		ret = snd_soc_component_update_bits(component,
> +				    DA7219_DAI_CLK_MODE,
>  				    DA7219_DAI_BCLKS_PER_WCLK_MASK,
>  				    DA7219_DAI_BCLKS_PER_WCLK_64);
> +		if (ret < 0)
> +			return ret;
>  		return 0;
>  	}
> 
> @@ -1444,19 +1533,25 @@ static int da7219_set_dai_tdm_slot(struct
> snd_soc_dai *dai,
>  		return -EINVAL;
>  	}
> 
> -	snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
>  			    DA7219_DAI_BCLKS_PER_WCLK_MASK,
>  			    dai_bclks_per_wclk);
> +	if (ret < 0)
> +		return ret;
> 
>  	offset = cpu_to_le16(rx_mask);
> -	regmap_bulk_write(da7219->regmap, DA7219_DAI_OFFSET_LOWER,
> +	ret = regmap_bulk_write(da7219->regmap,
> DA7219_DAI_OFFSET_LOWER,
>  			  &offset, sizeof(offset));
> +	if (ret < 0)
> +		return ret;
> 
> -	snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAI_TDM_CTRL,
>  			    DA7219_DAI_TDM_CH_EN_MASK |
>  			    DA7219_DAI_TDM_MODE_EN_MASK,
>  			    (tx_mask << DA7219_DAI_TDM_CH_EN_SHIFT) |
>  			    DA7219_DAI_TDM_MODE_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	return 0;
>  }
> @@ -1468,6 +1563,7 @@ static int da7219_hw_params(struct
> snd_pcm_substream *substream,
>  	struct snd_soc_component *component = dai->component;
>  	u8 dai_ctrl = 0, fs;
>  	unsigned int channels;
> +	int ret;
> 
>  	switch (params_width(params)) {
>  	case 16:
> @@ -1533,11 +1629,15 @@ static int da7219_hw_params(struct
> snd_pcm_substream *substream,
>  		return -EINVAL;
>  	}
> 
> -	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> +	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
>  			    DA7219_DAI_WORD_LENGTH_MASK |
>  			    DA7219_DAI_CH_NUM_MASK,
>  			    dai_ctrl);
> -	snd_soc_component_write(component, DA7219_SR, fs);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_write(component, DA7219_SR, fs);
> +	if (ret < 0)
> +		return ret;
> 
>  	return 0;
>  }
> @@ -1692,9 +1792,12 @@ static int da7219_set_bias_level(struct
> snd_soc_component *component,
>  	case SND_SOC_BIAS_STANDBY:
>  		if (snd_soc_component_get_bias_level(component) ==
> SND_SOC_BIAS_OFF)
>  			/* Master bias */
> -			snd_soc_component_update_bits(component,
> DA7219_REFERENCES,
> +			ret = snd_soc_component_update_bits(component,
> +					    DA7219_REFERENCES,
>  					    DA7219_BIAS_EN_MASK,
>  					    DA7219_BIAS_EN_MASK);
> +			if (ret < 0)
> +				return ret;
> 
>  		if (snd_soc_component_get_bias_level(component) ==
> SND_SOC_BIAS_PREPARE) {
>  			/* Remove MCLK */
> @@ -1705,8 +1808,11 @@ static int da7219_set_bias_level(struct
> snd_soc_component *component,
>  	case SND_SOC_BIAS_OFF:
>  		/* Only disable master bias if we're not a wake-up source */
>  		if (!da7219->wakeup_source)
> -			snd_soc_component_update_bits(component,
> DA7219_REFERENCES,
> +			ret = snd_soc_component_update_bits(component,
> +					    DA7219_REFERENCES,
>  					    DA7219_BIAS_EN_MASK, 0);
> +			if (ret < 0)
> +				return ret;
> 
>  		break;
>  	}
> @@ -1754,10 +1860,16 @@ static int da7219_handle_supplies(struct
> snd_soc_component *component)
>  	}
> 
>  	/* Ensure device in active mode */
> -	snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
> DA7219_SYSTEM_ACTIVE_MASK);
> +	ret = snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
> +				DA7219_SYSTEM_ACTIVE_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Update IO voltage level range */
> -	snd_soc_component_write(component, DA7219_IO_CTRL,
> io_voltage_lvl);
> +	ret = snd_soc_component_write(component, DA7219_IO_CTRL,
> +				io_voltage_lvl);
> +	if (ret < 0)
> +		return ret;
> 
>  	return 0;
>  }
> @@ -1768,10 +1880,13 @@ static int da7219_dai_clks_prepare(struct clk_hw
> *hw)
>  	struct da7219_priv *da7219 =
>  		container_of(hw, struct da7219_priv, dai_clks_hw);
>  	struct snd_soc_component *component = da7219->aad->component;
> +	int ret;
> 
> -	snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
>  				      DA7219_DAI_CLK_EN_MASK,
>  				      DA7219_DAI_CLK_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	return 0;
>  }
> @@ -1781,9 +1896,12 @@ static void da7219_dai_clks_unprepare(struct clk_hw
> *hw)
>  	struct da7219_priv *da7219 =
>  		container_of(hw, struct da7219_priv, dai_clks_hw);
>  	struct snd_soc_component *component = da7219->aad->component;
> +	int ret;
> 
> -	snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAI_CLK_MODE,
>  				      DA7219_DAI_CLK_EN_MASK, 0);
> +	if (ret < 0)
> +		dev_err(component->dev, "Failed to disable clk: %d\n", ret);
>  }
> 
>  static int da7219_dai_clks_is_prepared(struct clk_hw *hw) @@ -1791,9 +1909,11
> @@ static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
>  	struct da7219_priv *da7219 =
>  		container_of(hw, struct da7219_priv, dai_clks_hw);
>  	struct snd_soc_component *component = da7219->aad->component;
> -	u8 clk_reg;
> +	int clk_reg;
> 
>  	clk_reg = snd_soc_component_read32(component,
> DA7219_DAI_CLK_MODE);
> +	if (clk_reg < 0)
> +		return clk_reg;
> 
>  	return !!(clk_reg & DA7219_DAI_CLK_EN_MASK);  } @@ -1844,10
> +1964,11 @@ static void da7219_register_dai_clks(struct snd_soc_component
> *component)  static inline void da7219_register_dai_clks(struct
> snd_soc_component *component) {}  #endif /* CONFIG_COMMON_CLK */
> 
> -static void da7219_handle_pdata(struct snd_soc_component *component)
> +static int da7219_handle_pdata(struct snd_soc_component *component)
>  {
>  	struct da7219_priv *da7219 =
> snd_soc_component_get_drvdata(component);
>  	struct da7219_pdata *pdata = da7219->pdata;
> +	int ret;
> 
>  	if (pdata) {
>  		u8 micbias_lvl = 0;
> @@ -1869,7 +1990,10 @@ static void da7219_handle_pdata(struct
> snd_soc_component *component)
>  			break;
>  		}
> 
> -		snd_soc_component_write(component, DA7219_MICBIAS_CTRL,
> micbias_lvl);
> +		ret = snd_soc_component_write(component,
> DA7219_MICBIAS_CTRL,
> +					micbias_lvl);
> +		if (ret < 0)
> +			return ret;
> 
>  		/*
>  		 * Calculate delay required to compensate for DC offset in @@ -
> 1884,11 +2008,15 @@ static void da7219_handle_pdata(struct
> snd_soc_component *component)
>  		case DA7219_MIC_AMP_IN_SEL_DIFF:
>  		case DA7219_MIC_AMP_IN_SEL_SE_P:
>  		case DA7219_MIC_AMP_IN_SEL_SE_N:
> -			snd_soc_component_write(component,
> DA7219_MIC_1_SELECT,
> -				      pdata->mic_amp_in_sel);
> +			ret = snd_soc_component_write(component,
> +					DA7219_MIC_1_SELECT,
> +					pdata->mic_amp_in_sel);
> +			if (ret < 0)
> +				return ret;
>  			break;
>  		}
>  	}
> +	return 0;
>  }
> 
>  static struct reg_sequence da7219_rev_aa_patch[] = { @@ -1934,7 +2062,9 @@
> static int da7219_probe(struct snd_soc_component *component)
>  	if (!da7219->pdata)
>  		da7219->pdata = da7219_fw_to_pdata(component);
> 
> -	da7219_handle_pdata(component);
> +	ret = da7219_handle_pdata(component);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Check if MCLK provided */
>  	da7219->mclk = devm_clk_get(component->dev, "mclk"); @@ -1948,36
> +2078,57 @@ static int da7219_probe(struct snd_soc_component *component)
>  	}
> 
>  	/* Default PC counter to free-running */
> -	snd_soc_component_update_bits(component, DA7219_PC_COUNT,
> DA7219_PC_FREERUN_MASK,
> -			    DA7219_PC_FREERUN_MASK);
> +	ret = snd_soc_component_update_bits(component,
> DA7219_PC_COUNT,
> +			DA7219_PC_FREERUN_MASK,
> DA7219_PC_FREERUN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Default gain ramping */
> -	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_MIXIN_L_CTRL,
>  			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK,
>  			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
> DA7219_ADC_L_RAMP_EN_MASK,
> -			    DA7219_ADC_L_RAMP_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_DAC_L_CTRL,
> DA7219_DAC_L_RAMP_EN_MASK,
> -			    DA7219_DAC_L_RAMP_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_DAC_R_CTRL,
> DA7219_DAC_R_RAMP_EN_MASK,
> -			    DA7219_DAC_R_RAMP_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component,
> DA7219_ADC_L_CTRL,
> +			DA7219_ADC_L_RAMP_EN_MASK,
> DA7219_ADC_L_RAMP_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAC_L_CTRL,
> +			DA7219_DAC_L_RAMP_EN_MASK,
> DA7219_DAC_L_RAMP_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component,
> DA7219_DAC_R_CTRL,
> +			DA7219_DAC_R_RAMP_EN_MASK,
> DA7219_DAC_R_RAMP_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component,
> DA7219_HP_L_CTRL,
>  			    DA7219_HP_L_AMP_RAMP_EN_MASK,
>  			    DA7219_HP_L_AMP_RAMP_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component,
> DA7219_HP_R_CTRL,
>  			    DA7219_HP_R_AMP_RAMP_EN_MASK,
>  			    DA7219_HP_R_AMP_RAMP_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Default minimum gain on HP to avoid pops during DAPM sequencing
> */
> -	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
> +	ret = snd_soc_component_update_bits(component,
> DA7219_HP_L_CTRL,
>  			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK,
>  			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK);
> -	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_component_update_bits(component,
> DA7219_HP_R_CTRL,
>  			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK,
>  			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Default infinite tone gen, start/stop by Kcontrol */
> -	snd_soc_component_write(component, DA7219_TONE_GEN_CYCLES,
> DA7219_BEEP_CYCLES_MASK);
> +	ret = snd_soc_component_write(component,
> DA7219_TONE_GEN_CYCLES,
> +			DA7219_BEEP_CYCLES_MASK);
> +	if (ret < 0)
> +		return ret;
> 
>  	/* Initialise AAD block */
>  	ret = da7219_aad_init(component);
> @@ -2221,16 +2372,28 @@ static int da7219_i2c_probe(struct i2c_client *i2c,
>  	regcache_cache_bypass(da7219->regmap, true);
> 
>  	/* Disable audio paths if still active from previous start */
> -	regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> &system_active);
> +	ret = regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> &system_active);
> +	if (ret < 0)
> +		return ret;
>  	if (system_active) {
> -		regmap_write(da7219->regmap, DA7219_GAIN_RAMP_CTRL,
> +		ret = regmap_write(da7219->regmap,
> DA7219_GAIN_RAMP_CTRL,
>  			     DA7219_GAIN_RAMP_RATE_NOMINAL);
> -		regmap_write(da7219->regmap,
> DA7219_SYSTEM_MODES_INPUT, 0x00);
> -		regmap_write(da7219->regmap,
> DA7219_SYSTEM_MODES_OUTPUT, 0x01);
> +		if (ret < 0)
> +			return ret;
> +		ret = regmap_write(da7219->regmap,
> DA7219_SYSTEM_MODES_INPUT,
> +				0x00);
> +		if (ret < 0)
> +			return ret;
> +		ret = regmap_write(da7219->regmap,
> DA7219_SYSTEM_MODES_OUTPUT,
> +				0x01);
> +		if (ret < 0)
> +			return ret;
> 
>  		for (i = 0; i < DA7219_SYS_STAT_CHECK_RETRIES; ++i) {
> -			regmap_read(da7219->regmap,
> DA7219_SYSTEM_STATUS,
> +			ret = regmap_read(da7219->regmap,
> DA7219_SYSTEM_STATUS,
>  				    &system_status);
> +			if (ret < 0)
> +				return ret;
>  			if (!system_status)
>  				break;
> 
> @@ -2239,13 +2402,19 @@ static int da7219_i2c_probe(struct i2c_client *i2c,
>  	}
> 
>  	/* Soft reset component */
> -	regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
> +	ret = regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
>  			  DA7219_ACCDET_EN_MASK, 0);
> -	regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
>  			  DA7219_CIF_REG_SOFT_RESET_MASK,
>  			  DA7219_CIF_REG_SOFT_RESET_MASK);
> -	regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
>  			  DA7219_SYSTEM_ACTIVE_MASK, 0);
> +	if (ret < 0)
> +		return ret;
> 
>  	regcache_cache_bypass(da7219->regmap, false);
> 
> diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h index
> 3a00686..0cf78a7 100644
> --- a/sound/soc/codecs/da7219.h
> +++ b/sound/soc/codecs/da7219.h
> @@ -832,7 +832,7 @@ struct da7219_priv {
>  	bool alc_en;
>  	bool micbias_on_event;
>  	unsigned int mic_pga_delay;
> -	u8 gain_ramp_ctrl;
> +	int gain_ramp_ctrl;
>  };
> 
>  int da7219_set_pll(struct snd_soc_component *component, int source,
> unsigned int fout);
> --
> 1.9.1

Akshu Agrawal Dec. 5, 2018, 4:26 a.m. UTC | #2

On 12/5/2018 2:46 AM, Adam Thomson wrote:
> On 04 December 2018 18:36, Akshu Agrawal wrote:
> 
>> Failed i2c transaction can lead to failure in reg read or write.
>> Need to have error check for each read write operation.
>>
> 
> I'm not really clear what this gains you. If I2C is broken this won't solve
> anything, and you have far deeper problems. Seems to me like a lot of code which
> adds almost nothing.
>  

I agree Adam that if I2C is broken, system is in bad state. But so is
the case with most of the error situations. Error handling is anyway needed.
We have seen case where read32 returned -1, unsigned int cast and the
write back of the error value ends up in updating the register with 0xff.
Need to avoid scenarios like this.

Thanks,
Akshu
>> Signed-off-by: Akshu Agrawal <[e-mail]>
>> ---
>>  sound/soc/codecs/da7219.c | 323 +++++++++++++++++++++++++++++++++++-
>> ----------
>>  sound/soc/codecs/da7219.h |   2 +-
>>  2 files changed, 247 insertions(+), 78 deletions(-)
>>
>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index
>> e46e9f4..6757129 100644
>> --- a/sound/soc/codecs/da7219.c
>> +++ b/sound/soc/codecs/da7219.c
>> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
>> snd_kcontrol *kcontrol,  }
>>
>>  /* ALC */
>> -static void da7219_alc_calib(struct snd_soc_component *component)
>> +static int da7219_alc_calib(struct snd_soc_component *component)
>>  {
>> -	u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
>> +	int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
>> +	int ret;
>>
>>  	/* Save current state of mic control register */
>>  	mic_ctrl = snd_soc_component_read32(component,
>> DA7219_MIC_1_CTRL);
>> +	if (mic_ctrl < 0)
>> +		return mic_ctrl;
>>
>>  	/* Save current state of input mixer control register */
>>  	mixin_ctrl = snd_soc_component_read32(component,
>> DA7219_MIXIN_L_CTRL);
>> +	if (mixin_ctrl < 0)
>> +		return mixin_ctrl;
>>
>>  	/* Save current state of input ADC control register */
>>  	adc_ctrl = snd_soc_component_read32(component,
>> DA7219_ADC_L_CTRL);
>> +	if (adc_ctrl < 0)
>> +		return adc_ctrl;
>>
>>  	/* Enable then Mute MIC PGAs */
>> -	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
>> DA7219_MIC_1_AMP_EN_MASK,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_MIC_1_CTRL,
>> +			    DA7219_MIC_1_AMP_EN_MASK,
>>  			    DA7219_MIC_1_AMP_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_MIC_1_CTRL,
>>  			    DA7219_MIC_1_AMP_MUTE_EN_MASK,
>>  			    DA7219_MIC_1_AMP_MUTE_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Enable input mixers unmuted */
>> -	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_MIXIN_L_CTRL,
>>  			    DA7219_MIXIN_L_AMP_EN_MASK |
>>  			    DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
>>  			    DA7219_MIXIN_L_AMP_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Enable input filters unmuted */
>> -	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_ADC_L_CTRL,
>>  			    DA7219_ADC_L_MUTE_EN_MASK |
>> DA7219_ADC_L_EN_MASK,
>>  			    DA7219_ADC_L_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Perform auto calibration */
>> -	snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>>  			    DA7219_ALC_AUTO_CALIB_EN_MASK,
>>  			    DA7219_ALC_AUTO_CALIB_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>  	do {
>>  		calib_ctrl = snd_soc_component_read32(component,
>> DA7219_ALC_CTRL1);
>> +		if (calib_ctrl < 0)
>> +			return calib_ctrl;
>>  	} while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
>>
>>  	/* If auto calibration fails, disable DC offset, hybrid ALC */
>>  	if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
>>  		dev_warn(component->dev,
>>  			 "ALC auto calibration failed with overflow\n");
>> -		snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>>  				    DA7219_ALC_OFFSET_EN_MASK |
>>  				    DA7219_ALC_SYNC_MODE_MASK, 0);
>> +		if (ret < 0)
>> +			return ret;
>>  	} else {
>>  		/* Enable DC offset cancellation, hybrid mode */
>> -		snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>>  				    DA7219_ALC_OFFSET_EN_MASK |
>>  				    DA7219_ALC_SYNC_MODE_MASK,
>>  				    DA7219_ALC_OFFSET_EN_MASK |
>>  				    DA7219_ALC_SYNC_MODE_MASK);
>> +		if (ret < 0)
>> +			return ret;
>>  	}
>>
>>  	/* Restore input filter control register to original state */
>> -	snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
>> +	ret = snd_soc_component_write(component, DA7219_ADC_L_CTRL,
>> adc_ctrl);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Restore input mixer control registers to original state */
>> -	snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
>> mixin_ctrl);
>> +	ret = snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
>> +				mixin_ctrl);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Restore MIC control registers to original states */
>> -	snd_soc_component_write(component, DA7219_MIC_1_CTRL, mic_ctrl);
>> +	ret = snd_soc_component_write(component, DA7219_MIC_1_CTRL,
>> mic_ctrl);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>>  }
>>
>>  static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol, @@ -391,7
>> +425,7 @@ static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol,
>>  	 * make sure calibrated offsets are updated.
>>  	 */
>>  	if ((ret == 1) && (da7219->alc_en))
>> -		da7219_alc_calib(component);
>> +		ret = da7219_alc_calib(component);
>>
>>  	return ret;
>>  }
>> @@ -401,11 +435,14 @@ static int da7219_alc_sw_put(struct snd_kcontrol
>> *kcontrol,  {
>>  	struct snd_soc_component *component =
>> snd_soc_kcontrol_component(kcontrol);
>>  	struct da7219_priv *da7219 =
>> snd_soc_component_get_drvdata(component);
>> +	int ret;
>>
>>
>>  	/* Force ALC offset calibration if enabling ALC */
>>  	if ((ucontrol->value.integer.value[0]) && (!da7219->alc_en)) {
>> -		da7219_alc_calib(component);
>> +		ret = da7219_alc_calib(component);
>> +		if (ret < 0)
>> +			return ret;
>>  		da7219->alc_en = true;
>>  	} else {
>>  		da7219->alc_en = false;
>> @@ -813,16 +850,20 @@ static int da7219_dai_event(struct
>> snd_soc_dapm_widget *w,
>>  					return ret;
>>  				}
>>  			} else {
>> -				snd_soc_component_update_bits(component,
>> +				ret =
>> snd_soc_component_update_bits(component,
>>
>> DA7219_DAI_CLK_MODE,
>>
>> DA7219_DAI_CLK_EN_MASK,
>>
>> DA7219_DAI_CLK_EN_MASK);
>> +				if (ret < 0)
>> +					return ret;
>>  			}
>>  		}
>>
>>  		/* PC synchronised to DAI */
>> -		snd_soc_component_update_bits(component,
>> DA7219_PC_COUNT,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_PC_COUNT,
>>  				    DA7219_PC_FREERUN_MASK, 0);
>> +		if (ret < 0)
>> +			return ret;
>>
>>  		/* Slave mode, if SRM not enabled no need for status checks */
>>  		pll_ctrl = snd_soc_component_read32(component,
>> DA7219_PLL_CTRL); @@ -846,19 +887,23 @@ static int da7219_dai_event(struct
>> snd_soc_dapm_widget *w,
>>  		return 0;
>>  	case SND_SOC_DAPM_POST_PMD:
>>  		/* PC free-running */
>> -		snd_soc_component_update_bits(component,
>> DA7219_PC_COUNT,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_PC_COUNT,
>>  				    DA7219_PC_FREERUN_MASK,
>>  				    DA7219_PC_FREERUN_MASK);
>> +		if (ret < 0)
>> +			return ret;
>>
>>  		/* Disable DAI clks if in master mode */
>>  		if (da7219->master) {
>>  			if (da7219->dai_clks)
>>  				clk_disable_unprepare(da7219->dai_clks);
>>  			else
>> -				snd_soc_component_update_bits(component,
>> +				ret =
>> snd_soc_component_update_bits(component,
>>
>> DA7219_DAI_CLK_MODE,
>>
>> DA7219_DAI_CLK_EN_MASK,
>>  							      0);
>> +				if (ret < 0)
>> +					return ret;
>>  		}
>>
>>  		return 0;
>> @@ -887,6 +932,7 @@ static int da7219_mixout_event(struct
>> snd_soc_dapm_widget *w,  {
>>  	struct snd_soc_component *component =
>> snd_soc_dapm_to_component(w->dapm);
>>  	u8 hp_ctrl, min_gain_mask;
>> +	int ret;
>>
>>  	switch (w->reg) {
>>  	case DA7219_MIXOUT_L_CTRL:
>> @@ -904,15 +950,20 @@ static int da7219_mixout_event(struct
>> snd_soc_dapm_widget *w,
>>  	switch (event) {
>>  	case SND_SOC_DAPM_PRE_PMD:
>>  		/* Enable minimum gain on HP to avoid pops */
>> -		snd_soc_component_update_bits(component, hp_ctrl,
>> min_gain_mask,
>> -				    min_gain_mask);
>> +		ret = snd_soc_component_update_bits(component, hp_ctrl,
>> +				    min_gain_mask, min_gain_mask);
>> +		if (ret < 0)
>> +			return ret;
>>
>>  		msleep(DA7219_MIN_GAIN_DELAY);
>>
>>  		break;
>>  	case SND_SOC_DAPM_POST_PMU:
>>  		/* Remove minimum gain on HP */
>> -		snd_soc_component_update_bits(component, hp_ctrl,
>> min_gain_mask, 0);
>> +		ret = snd_soc_component_update_bits(component, hp_ctrl,
>> +				    min_gain_mask, 0);
>> +		if (ret < 0)
>> +			return ret;
>>
>>  		break;
>>  	}
>> @@ -925,21 +976,29 @@ static int da7219_gain_ramp_event(struct
>> snd_soc_dapm_widget *w,  {
>>  	struct snd_soc_component *component =
>> snd_soc_dapm_to_component(w->dapm);
>>  	struct da7219_priv *da7219 =
>> snd_soc_component_get_drvdata(component);
>> +	int ret;
>>
>>  	switch (event) {
>>  	case SND_SOC_DAPM_PRE_PMU:
>>  	case SND_SOC_DAPM_PRE_PMD:
>>  		/* Ensure nominal gain ramping for DAPM sequence */
>> -		da7219->gain_ramp_ctrl =
>> -			snd_soc_component_read32(component,
>> DA7219_GAIN_RAMP_CTRL);
>> -		snd_soc_component_write(component,
>> DA7219_GAIN_RAMP_CTRL,
>> -			      DA7219_GAIN_RAMP_RATE_NOMINAL);
>> +		da7219->gain_ramp_ctrl =
>> snd_soc_component_read32(component,
>> +						DA7219_GAIN_RAMP_CTRL);
>> +		if (da7219->gain_ramp_ctrl < 0)
>> +			return da7219->gain_ramp_ctrl;
>> +		 ret = snd_soc_component_write(component,
>> +				DA7219_GAIN_RAMP_CTRL,
>> +				DA7219_GAIN_RAMP_RATE_NOMINAL);
>> +		if (ret < 0)
>> +			return ret;
>>  		break;
>>  	case SND_SOC_DAPM_POST_PMU:
>>  	case SND_SOC_DAPM_POST_PMD:
>>  		/* Restore previous gain ramp settings */
>> -		snd_soc_component_write(component,
>> DA7219_GAIN_RAMP_CTRL,
>> +		ret = snd_soc_component_write(component,
>> DA7219_GAIN_RAMP_CTRL,
>>  			      da7219->gain_ramp_ctrl);
>> +		if (ret < 0)
>> +			return ret;
>>  		break;
>>  	}
>>
>> @@ -1177,13 +1236,17 @@ static int da7219_set_dai_sysclk(struct snd_soc_dai
>> *codec_dai,
>>
>>  	switch (clk_id) {
>>  	case DA7219_CLKSRC_MCLK_SQR:
>> -		snd_soc_component_update_bits(component,
>> DA7219_PLL_CTRL,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_PLL_CTRL,
>>  				    DA7219_PLL_MCLK_SQR_EN_MASK,
>>  				    DA7219_PLL_MCLK_SQR_EN_MASK);
>> +		if (ret < 0)
>> +			return ret;
>>  		break;
>>  	case DA7219_CLKSRC_MCLK:
>> -		snd_soc_component_update_bits(component,
>> DA7219_PLL_CTRL,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_PLL_CTRL,
>>  				    DA7219_PLL_MCLK_SQR_EN_MASK, 0);
>> +		if (ret < 0)
>> +			return ret;
>>  		break;
>>  	default:
>>  		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
>> @@ -1219,6 +1282,7 @@ int da7219_set_pll(struct snd_soc_component
>> *component, int source, unsigned int
>>  	u8 pll_frac_top, pll_frac_bot, pll_integer;
>>  	u32 freq_ref;
>>  	u64 frac_div;
>> +	int ret;
>>
>>  	/* Verify 2MHz - 54MHz MCLK provided, and set input divider */
>>  	if (da7219->mclk_rate < 2000000) {
>> @@ -1252,9 +1316,11 @@ int da7219_set_pll(struct snd_soc_component
>> *component, int source, unsigned int
>>  	switch (source) {
>>  	case DA7219_SYSCLK_MCLK:
>>  		pll_ctrl |= DA7219_PLL_MODE_BYPASS;
>> -		snd_soc_component_update_bits(component,
>> DA7219_PLL_CTRL,
>> +		ret = snd_soc_component_update_bits(component,
>> DA7219_PLL_CTRL,
>>  				    DA7219_PLL_INDIV_MASK |
>>  				    DA7219_PLL_MODE_MASK, pll_ctrl);
>> +		if (ret < 0)
>> +			return ret;
>>  		return 0;
>>  	case DA7219_SYSCLK_PLL:
>>  		pll_ctrl |= DA7219_PLL_MODE_NORMAL;
>> @@ -1275,12 +1341,23 @@ int da7219_set_pll(struct snd_soc_component
>> *component, int source, unsigned int
>>  	pll_frac_bot = (frac_div) & DA7219_BYTE_MASK;
>>
>>  	/* Write PLL config & dividers */
>> -	snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
>> pll_frac_top);
>> -	snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
>> pll_frac_bot);
>> -	snd_soc_component_write(component, DA7219_PLL_INTEGER,
>> pll_integer);
>> -	snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
>> +	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
>> +				pll_frac_top);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
>> +				pll_frac_bot);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_write(component, DA7219_PLL_INTEGER,
>> +				pll_integer);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
>>  			    DA7219_PLL_INDIV_MASK |
>> DA7219_PLL_MODE_MASK,
>>  			    pll_ctrl);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	return 0;
>>  }
>> @@ -1304,6 +1381,7 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
>> *codec_dai, unsigned int fmt)
>>  	struct snd_soc_component *component = codec_dai->component;
>>  	struct da7219_priv *da7219 =
>> snd_soc_component_get_drvdata(component);
>>  	u8 dai_clk_mode = 0, dai_ctrl = 0;
>> +	int ret;
>>
>>  	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>>  	case SND_SOC_DAIFMT_CBM_CFM:
>> @@ -1379,12 +1457,16 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
>> *codec_dai, unsigned int fmt)
>>  	/* By default 64 BCLKs per WCLK is supported */
>>  	dai_clk_mode |= DA7219_DAI_BCLKS_PER_WCLK_64;
>>
>> -	snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>>  			    DA7219_DAI_BCLKS_PER_WCLK_MASK |
>>  			    DA7219_DAI_CLK_POL_MASK |
>> DA7219_DAI_WCLK_POL_MASK,
>>  			    dai_clk_mode);
>> -	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
>> DA7219_DAI_FORMAT_MASK,
>> -			    dai_ctrl);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
>> +			    DA7219_DAI_FORMAT_MASK, dai_ctrl);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	return 0;
>>  }
>> @@ -1398,15 +1480,22 @@ static int da7219_set_dai_tdm_slot(struct
>> snd_soc_dai *dai,
>>  	u8 dai_bclks_per_wclk;
>>  	u16 offset;
>>  	u32 frame_size;
>> +	int ret;
>>
>>  	/* No channels enabled so disable TDM, revert to 64-bit frames */
>>  	if (!tx_mask) {
>> -		snd_soc_component_update_bits(component,
>> DA7219_DAI_TDM_CTRL,
>> +		ret = snd_soc_component_update_bits(component,
>> +				    DA7219_DAI_TDM_CTRL,
>>  				    DA7219_DAI_TDM_CH_EN_MASK |
>>  				    DA7219_DAI_TDM_MODE_EN_MASK, 0);
>> -		snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>> +		if (ret < 0)
>> +			return ret;
>> +		ret = snd_soc_component_update_bits(component,
>> +				    DA7219_DAI_CLK_MODE,
>>  				    DA7219_DAI_BCLKS_PER_WCLK_MASK,
>>  				    DA7219_DAI_BCLKS_PER_WCLK_64);
>> +		if (ret < 0)
>> +			return ret;
>>  		return 0;
>>  	}
>>
>> @@ -1444,19 +1533,25 @@ static int da7219_set_dai_tdm_slot(struct
>> snd_soc_dai *dai,
>>  		return -EINVAL;
>>  	}
>>
>> -	snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>>  			    DA7219_DAI_BCLKS_PER_WCLK_MASK,
>>  			    dai_bclks_per_wclk);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	offset = cpu_to_le16(rx_mask);
>> -	regmap_bulk_write(da7219->regmap, DA7219_DAI_OFFSET_LOWER,
>> +	ret = regmap_bulk_write(da7219->regmap,
>> DA7219_DAI_OFFSET_LOWER,
>>  			  &offset, sizeof(offset));
>> +	if (ret < 0)
>> +		return ret;
>>
>> -	snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAI_TDM_CTRL,
>>  			    DA7219_DAI_TDM_CH_EN_MASK |
>>  			    DA7219_DAI_TDM_MODE_EN_MASK,
>>  			    (tx_mask << DA7219_DAI_TDM_CH_EN_SHIFT) |
>>  			    DA7219_DAI_TDM_MODE_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	return 0;
>>  }
>> @@ -1468,6 +1563,7 @@ static int da7219_hw_params(struct
>> snd_pcm_substream *substream,
>>  	struct snd_soc_component *component = dai->component;
>>  	u8 dai_ctrl = 0, fs;
>>  	unsigned int channels;
>> +	int ret;
>>
>>  	switch (params_width(params)) {
>>  	case 16:
>> @@ -1533,11 +1629,15 @@ static int da7219_hw_params(struct
>> snd_pcm_substream *substream,
>>  		return -EINVAL;
>>  	}
>>
>> -	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
>> +	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
>>  			    DA7219_DAI_WORD_LENGTH_MASK |
>>  			    DA7219_DAI_CH_NUM_MASK,
>>  			    dai_ctrl);
>> -	snd_soc_component_write(component, DA7219_SR, fs);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_write(component, DA7219_SR, fs);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	return 0;
>>  }
>> @@ -1692,9 +1792,12 @@ static int da7219_set_bias_level(struct
>> snd_soc_component *component,
>>  	case SND_SOC_BIAS_STANDBY:
>>  		if (snd_soc_component_get_bias_level(component) ==
>> SND_SOC_BIAS_OFF)
>>  			/* Master bias */
>> -			snd_soc_component_update_bits(component,
>> DA7219_REFERENCES,
>> +			ret = snd_soc_component_update_bits(component,
>> +					    DA7219_REFERENCES,
>>  					    DA7219_BIAS_EN_MASK,
>>  					    DA7219_BIAS_EN_MASK);
>> +			if (ret < 0)
>> +				return ret;
>>
>>  		if (snd_soc_component_get_bias_level(component) ==
>> SND_SOC_BIAS_PREPARE) {
>>  			/* Remove MCLK */
>> @@ -1705,8 +1808,11 @@ static int da7219_set_bias_level(struct
>> snd_soc_component *component,
>>  	case SND_SOC_BIAS_OFF:
>>  		/* Only disable master bias if we're not a wake-up source */
>>  		if (!da7219->wakeup_source)
>> -			snd_soc_component_update_bits(component,
>> DA7219_REFERENCES,
>> +			ret = snd_soc_component_update_bits(component,
>> +					    DA7219_REFERENCES,
>>  					    DA7219_BIAS_EN_MASK, 0);
>> +			if (ret < 0)
>> +				return ret;
>>
>>  		break;
>>  	}
>> @@ -1754,10 +1860,16 @@ static int da7219_handle_supplies(struct
>> snd_soc_component *component)
>>  	}
>>
>>  	/* Ensure device in active mode */
>> -	snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
>> DA7219_SYSTEM_ACTIVE_MASK);
>> +	ret = snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
>> +				DA7219_SYSTEM_ACTIVE_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Update IO voltage level range */
>> -	snd_soc_component_write(component, DA7219_IO_CTRL,
>> io_voltage_lvl);
>> +	ret = snd_soc_component_write(component, DA7219_IO_CTRL,
>> +				io_voltage_lvl);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	return 0;
>>  }
>> @@ -1768,10 +1880,13 @@ static int da7219_dai_clks_prepare(struct clk_hw
>> *hw)
>>  	struct da7219_priv *da7219 =
>>  		container_of(hw, struct da7219_priv, dai_clks_hw);
>>  	struct snd_soc_component *component = da7219->aad->component;
>> +	int ret;
>>
>> -	snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>>  				      DA7219_DAI_CLK_EN_MASK,
>>  				      DA7219_DAI_CLK_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	return 0;
>>  }
>> @@ -1781,9 +1896,12 @@ static void da7219_dai_clks_unprepare(struct clk_hw
>> *hw)
>>  	struct da7219_priv *da7219 =
>>  		container_of(hw, struct da7219_priv, dai_clks_hw);
>>  	struct snd_soc_component *component = da7219->aad->component;
>> +	int ret;
>>
>> -	snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAI_CLK_MODE,
>>  				      DA7219_DAI_CLK_EN_MASK, 0);
>> +	if (ret < 0)
>> +		dev_err(component->dev, "Failed to disable clk: %d\n", ret);
>>  }
>>
>>  static int da7219_dai_clks_is_prepared(struct clk_hw *hw) @@ -1791,9 +1909,11
>> @@ static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
>>  	struct da7219_priv *da7219 =
>>  		container_of(hw, struct da7219_priv, dai_clks_hw);
>>  	struct snd_soc_component *component = da7219->aad->component;
>> -	u8 clk_reg;
>> +	int clk_reg;
>>
>>  	clk_reg = snd_soc_component_read32(component,
>> DA7219_DAI_CLK_MODE);
>> +	if (clk_reg < 0)
>> +		return clk_reg;
>>
>>  	return !!(clk_reg & DA7219_DAI_CLK_EN_MASK);  } @@ -1844,10
>> +1964,11 @@ static void da7219_register_dai_clks(struct snd_soc_component
>> *component)  static inline void da7219_register_dai_clks(struct
>> snd_soc_component *component) {}  #endif /* CONFIG_COMMON_CLK */
>>
>> -static void da7219_handle_pdata(struct snd_soc_component *component)
>> +static int da7219_handle_pdata(struct snd_soc_component *component)
>>  {
>>  	struct da7219_priv *da7219 =
>> snd_soc_component_get_drvdata(component);
>>  	struct da7219_pdata *pdata = da7219->pdata;
>> +	int ret;
>>
>>  	if (pdata) {
>>  		u8 micbias_lvl = 0;
>> @@ -1869,7 +1990,10 @@ static void da7219_handle_pdata(struct
>> snd_soc_component *component)
>>  			break;
>>  		}
>>
>> -		snd_soc_component_write(component, DA7219_MICBIAS_CTRL,
>> micbias_lvl);
>> +		ret = snd_soc_component_write(component,
>> DA7219_MICBIAS_CTRL,
>> +					micbias_lvl);
>> +		if (ret < 0)
>> +			return ret;
>>
>>  		/*
>>  		 * Calculate delay required to compensate for DC offset in @@ -
>> 1884,11 +2008,15 @@ static void da7219_handle_pdata(struct
>> snd_soc_component *component)
>>  		case DA7219_MIC_AMP_IN_SEL_DIFF:
>>  		case DA7219_MIC_AMP_IN_SEL_SE_P:
>>  		case DA7219_MIC_AMP_IN_SEL_SE_N:
>> -			snd_soc_component_write(component,
>> DA7219_MIC_1_SELECT,
>> -				      pdata->mic_amp_in_sel);
>> +			ret = snd_soc_component_write(component,
>> +					DA7219_MIC_1_SELECT,
>> +					pdata->mic_amp_in_sel);
>> +			if (ret < 0)
>> +				return ret;
>>  			break;
>>  		}
>>  	}
>> +	return 0;
>>  }
>>
>>  static struct reg_sequence da7219_rev_aa_patch[] = { @@ -1934,7 +2062,9 @@
>> static int da7219_probe(struct snd_soc_component *component)
>>  	if (!da7219->pdata)
>>  		da7219->pdata = da7219_fw_to_pdata(component);
>>
>> -	da7219_handle_pdata(component);
>> +	ret = da7219_handle_pdata(component);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Check if MCLK provided */
>>  	da7219->mclk = devm_clk_get(component->dev, "mclk"); @@ -1948,36
>> +2078,57 @@ static int da7219_probe(struct snd_soc_component *component)
>>  	}
>>
>>  	/* Default PC counter to free-running */
>> -	snd_soc_component_update_bits(component, DA7219_PC_COUNT,
>> DA7219_PC_FREERUN_MASK,
>> -			    DA7219_PC_FREERUN_MASK);
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_PC_COUNT,
>> +			DA7219_PC_FREERUN_MASK,
>> DA7219_PC_FREERUN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Default gain ramping */
>> -	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_MIXIN_L_CTRL,
>>  			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK,
>>  			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
>> DA7219_ADC_L_RAMP_EN_MASK,
>> -			    DA7219_ADC_L_RAMP_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_DAC_L_CTRL,
>> DA7219_DAC_L_RAMP_EN_MASK,
>> -			    DA7219_DAC_L_RAMP_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_DAC_R_CTRL,
>> DA7219_DAC_R_RAMP_EN_MASK,
>> -			    DA7219_DAC_R_RAMP_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_ADC_L_CTRL,
>> +			DA7219_ADC_L_RAMP_EN_MASK,
>> DA7219_ADC_L_RAMP_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAC_L_CTRL,
>> +			DA7219_DAC_L_RAMP_EN_MASK,
>> DA7219_DAC_L_RAMP_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_DAC_R_CTRL,
>> +			DA7219_DAC_R_RAMP_EN_MASK,
>> DA7219_DAC_R_RAMP_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_HP_L_CTRL,
>>  			    DA7219_HP_L_AMP_RAMP_EN_MASK,
>>  			    DA7219_HP_L_AMP_RAMP_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_HP_R_CTRL,
>>  			    DA7219_HP_R_AMP_RAMP_EN_MASK,
>>  			    DA7219_HP_R_AMP_RAMP_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Default minimum gain on HP to avoid pops during DAPM sequencing
>> */
>> -	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_HP_L_CTRL,
>>  			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK,
>>  			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK);
>> -	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = snd_soc_component_update_bits(component,
>> DA7219_HP_R_CTRL,
>>  			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK,
>>  			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Default infinite tone gen, start/stop by Kcontrol */
>> -	snd_soc_component_write(component, DA7219_TONE_GEN_CYCLES,
>> DA7219_BEEP_CYCLES_MASK);
>> +	ret = snd_soc_component_write(component,
>> DA7219_TONE_GEN_CYCLES,
>> +			DA7219_BEEP_CYCLES_MASK);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	/* Initialise AAD block */
>>  	ret = da7219_aad_init(component);
>> @@ -2221,16 +2372,28 @@ static int da7219_i2c_probe(struct i2c_client *i2c,
>>  	regcache_cache_bypass(da7219->regmap, true);
>>
>>  	/* Disable audio paths if still active from previous start */
>> -	regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE,
>> &system_active);
>> +	ret = regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE,
>> &system_active);
>> +	if (ret < 0)
>> +		return ret;
>>  	if (system_active) {
>> -		regmap_write(da7219->regmap, DA7219_GAIN_RAMP_CTRL,
>> +		ret = regmap_write(da7219->regmap,
>> DA7219_GAIN_RAMP_CTRL,
>>  			     DA7219_GAIN_RAMP_RATE_NOMINAL);
>> -		regmap_write(da7219->regmap,
>> DA7219_SYSTEM_MODES_INPUT, 0x00);
>> -		regmap_write(da7219->regmap,
>> DA7219_SYSTEM_MODES_OUTPUT, 0x01);
>> +		if (ret < 0)
>> +			return ret;
>> +		ret = regmap_write(da7219->regmap,
>> DA7219_SYSTEM_MODES_INPUT,
>> +				0x00);
>> +		if (ret < 0)
>> +			return ret;
>> +		ret = regmap_write(da7219->regmap,
>> DA7219_SYSTEM_MODES_OUTPUT,
>> +				0x01);
>> +		if (ret < 0)
>> +			return ret;
>>
>>  		for (i = 0; i < DA7219_SYS_STAT_CHECK_RETRIES; ++i) {
>> -			regmap_read(da7219->regmap,
>> DA7219_SYSTEM_STATUS,
>> +			ret = regmap_read(da7219->regmap,
>> DA7219_SYSTEM_STATUS,
>>  				    &system_status);
>> +			if (ret < 0)
>> +				return ret;
>>  			if (!system_status)
>>  				break;
>>
>> @@ -2239,13 +2402,19 @@ static int da7219_i2c_probe(struct i2c_client *i2c,
>>  	}
>>
>>  	/* Soft reset component */
>> -	regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
>> +	ret = regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
>>  			  DA7219_ACCDET_EN_MASK, 0);
>> -	regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
>>  			  DA7219_CIF_REG_SOFT_RESET_MASK,
>>  			  DA7219_CIF_REG_SOFT_RESET_MASK);
>> -	regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
>>  			  DA7219_SYSTEM_ACTIVE_MASK, 0);
>> +	if (ret < 0)
>> +		return ret;
>>
>>  	regcache_cache_bypass(da7219->regmap, false);
>>
>> diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h index
>> 3a00686..0cf78a7 100644
>> --- a/sound/soc/codecs/da7219.h
>> +++ b/sound/soc/codecs/da7219.h
>> @@ -832,7 +832,7 @@ struct da7219_priv {
>>  	bool alc_en;
>>  	bool micbias_on_event;
>>  	unsigned int mic_pga_delay;
>> -	u8 gain_ramp_ctrl;
>> +	int gain_ramp_ctrl;
>>  };
>>
>>  int da7219_set_pll(struct snd_soc_component *component, int source,
>> unsigned int fout);
>> --
>> 1.9.1

Adam Thomson Dec. 5, 2018, 10:21 a.m. UTC | #3

On 05 December 2018 04:27, Akshu Agrawal wrote:

> On 12/5/2018 2:46 AM, Adam Thomson wrote:
> > On 04 December 2018 18:36, Akshu Agrawal wrote:
> >
> >> Failed i2c transaction can lead to failure in reg read or write.
> >> Need to have error check for each read write operation.
> >>
> >
> > I'm not really clear what this gains you. If I2C is broken this won't
> > solve anything, and you have far deeper problems. Seems to me like a
> > lot of code which adds almost nothing.
> >
> 
> I agree Adam that if I2C is broken, system is in bad state. But so is the case with
> most of the error situations. Error handling is anyway needed.
> We have seen case where read32 returned -1, unsigned int cast and the write
> back of the error value ends up in updating the register with 0xff.
> Need to avoid scenarios like this.

If the previous I2C access failed, how can we be sure that the write back to HW
of 0xFF even succeeds? More importantly these error returns won't necessarily
stop subsequent calls to controls within the Codec I believe, so you could still
see unwanted writes to HW via I2C, if I2C is sporadically operational. Again I
don't see this update resolving that. The key thing is to resolve why even just
one I2C transaction fails.

I'd like to hear input from others on this as if this was something
fundamentally required, all other I2C based codecs would be doing this and
that's just not the case right now.

One last thing, if a change like this is somehow deemed to be required, you will
need to update the AAD part of the driver as well.

> 
> Thanks,
> Akshu
> >> Signed-off-by: Akshu Agrawal <[e-mail]>
> >> ---
> >>  sound/soc/codecs/da7219.c | 323
> +++++++++++++++++++++++++++++++++++-
> >> ----------
> >>  sound/soc/codecs/da7219.h |   2 +-
> >>  2 files changed, 247 insertions(+), 78 deletions(-)
> >>
> >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> >> index
> >> e46e9f4..6757129 100644
> >> --- a/sound/soc/codecs/da7219.c
> >> +++ b/sound/soc/codecs/da7219.c
> >> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
> >> snd_kcontrol *kcontrol,  }
> >>
> >>  /* ALC */
> >> -static void da7219_alc_calib(struct snd_soc_component *component)
> >> +static int da7219_alc_calib(struct snd_soc_component *component)
> >>  {
> >> -	u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> >> +	int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> >> +	int ret;
> >>
> >>  	/* Save current state of mic control register */
> >>  	mic_ctrl = snd_soc_component_read32(component,
> >> DA7219_MIC_1_CTRL);
> >> +	if (mic_ctrl < 0)
> >> +		return mic_ctrl;
> >>
> >>  	/* Save current state of input mixer control register */
> >>  	mixin_ctrl = snd_soc_component_read32(component,
> >> DA7219_MIXIN_L_CTRL);
> >> +	if (mixin_ctrl < 0)
> >> +		return mixin_ctrl;
> >>
> >>  	/* Save current state of input ADC control register */
> >>  	adc_ctrl = snd_soc_component_read32(component,
> >> DA7219_ADC_L_CTRL);
> >> +	if (adc_ctrl < 0)
> >> +		return adc_ctrl;
> >>
> >>  	/* Enable then Mute MIC PGAs */
> >> -	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> >> DA7219_MIC_1_AMP_EN_MASK,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_MIC_1_CTRL,
> >> +			    DA7219_MIC_1_AMP_EN_MASK,
> >>  			    DA7219_MIC_1_AMP_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_MIC_1_CTRL,
> >>  			    DA7219_MIC_1_AMP_MUTE_EN_MASK,
> >>  			    DA7219_MIC_1_AMP_MUTE_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Enable input mixers unmuted */
> >> -	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_MIXIN_L_CTRL,
> >>  			    DA7219_MIXIN_L_AMP_EN_MASK |
> >>  			    DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
> >>  			    DA7219_MIXIN_L_AMP_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Enable input filters unmuted */
> >> -	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_ADC_L_CTRL,
> >>  			    DA7219_ADC_L_MUTE_EN_MASK |
> >> DA7219_ADC_L_EN_MASK,
> >>  			    DA7219_ADC_L_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Perform auto calibration */
> >> -	snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_ALC_CTRL1,
> >>  			    DA7219_ALC_AUTO_CALIB_EN_MASK,
> >>  			    DA7219_ALC_AUTO_CALIB_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>  	do {
> >>  		calib_ctrl = snd_soc_component_read32(component,
> >> DA7219_ALC_CTRL1);
> >> +		if (calib_ctrl < 0)
> >> +			return calib_ctrl;
> >>  	} while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
> >>
> >>  	/* If auto calibration fails, disable DC offset, hybrid ALC */
> >>  	if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
> >>  		dev_warn(component->dev,
> >>  			 "ALC auto calibration failed with overflow\n");
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_ALC_CTRL1,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_ALC_CTRL1,
> >>  				    DA7219_ALC_OFFSET_EN_MASK |
> >>  				    DA7219_ALC_SYNC_MODE_MASK, 0);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  	} else {
> >>  		/* Enable DC offset cancellation, hybrid mode */
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_ALC_CTRL1,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_ALC_CTRL1,
> >>  				    DA7219_ALC_OFFSET_EN_MASK |
> >>  				    DA7219_ALC_SYNC_MODE_MASK,
> >>  				    DA7219_ALC_OFFSET_EN_MASK |
> >>  				    DA7219_ALC_SYNC_MODE_MASK);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  	}
> >>
> >>  	/* Restore input filter control register to original state */
> >> -	snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
> >> +	ret = snd_soc_component_write(component, DA7219_ADC_L_CTRL,
> >> adc_ctrl);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Restore input mixer control registers to original state */
> >> -	snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
> >> mixin_ctrl);
> >> +	ret = snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
> >> +				mixin_ctrl);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Restore MIC control registers to original states */
> >> -	snd_soc_component_write(component, DA7219_MIC_1_CTRL, mic_ctrl);
> >> +	ret = snd_soc_component_write(component, DA7219_MIC_1_CTRL,
> >> mic_ctrl);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	return 0;
> >>  }
> >>
> >>  static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol, @@
> >> -391,7
> >> +425,7 @@ static int da7219_mixin_gain_put(struct snd_kcontrol
> >> +*kcontrol,
> >>  	 * make sure calibrated offsets are updated.
> >>  	 */
> >>  	if ((ret == 1) && (da7219->alc_en))
> >> -		da7219_alc_calib(component);
> >> +		ret = da7219_alc_calib(component);
> >>
> >>  	return ret;
> >>  }
> >> @@ -401,11 +435,14 @@ static int da7219_alc_sw_put(struct
> >> snd_kcontrol *kcontrol,  {
> >>  	struct snd_soc_component *component =
> >> snd_soc_kcontrol_component(kcontrol);
> >>  	struct da7219_priv *da7219 =
> >> snd_soc_component_get_drvdata(component);
> >> +	int ret;
> >>
> >>
> >>  	/* Force ALC offset calibration if enabling ALC */
> >>  	if ((ucontrol->value.integer.value[0]) && (!da7219->alc_en)) {
> >> -		da7219_alc_calib(component);
> >> +		ret = da7219_alc_calib(component);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		da7219->alc_en = true;
> >>  	} else {
> >>  		da7219->alc_en = false;
> >> @@ -813,16 +850,20 @@ static int da7219_dai_event(struct
> >> snd_soc_dapm_widget *w,
> >>  					return ret;
> >>  				}
> >>  			} else {
> >> -				snd_soc_component_update_bits(component,
> >> +				ret =
> >> snd_soc_component_update_bits(component,
> >>
> >> DA7219_DAI_CLK_MODE,
> >>
> >> DA7219_DAI_CLK_EN_MASK,
> >>
> >> DA7219_DAI_CLK_EN_MASK);
> >> +				if (ret < 0)
> >> +					return ret;
> >>  			}
> >>  		}
> >>
> >>  		/* PC synchronised to DAI */
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_PC_COUNT,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_PC_COUNT,
> >>  				    DA7219_PC_FREERUN_MASK, 0);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >>  		/* Slave mode, if SRM not enabled no need for status checks */
> >>  		pll_ctrl = snd_soc_component_read32(component,
> >> DA7219_PLL_CTRL); @@ -846,19 +887,23 @@ static int
> >> da7219_dai_event(struct snd_soc_dapm_widget *w,
> >>  		return 0;
> >>  	case SND_SOC_DAPM_POST_PMD:
> >>  		/* PC free-running */
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_PC_COUNT,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_PC_COUNT,
> >>  				    DA7219_PC_FREERUN_MASK,
> >>  				    DA7219_PC_FREERUN_MASK);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >>  		/* Disable DAI clks if in master mode */
> >>  		if (da7219->master) {
> >>  			if (da7219->dai_clks)
> >>  				clk_disable_unprepare(da7219->dai_clks);
> >>  			else
> >> -				snd_soc_component_update_bits(component,
> >> +				ret =
> >> snd_soc_component_update_bits(component,
> >>
> >> DA7219_DAI_CLK_MODE,
> >>
> >> DA7219_DAI_CLK_EN_MASK,
> >>  							      0);
> >> +				if (ret < 0)
> >> +					return ret;
> >>  		}
> >>
> >>  		return 0;
> >> @@ -887,6 +932,7 @@ static int da7219_mixout_event(struct
> >> snd_soc_dapm_widget *w,  {
> >>  	struct snd_soc_component *component =
> >> snd_soc_dapm_to_component(w->dapm);
> >>  	u8 hp_ctrl, min_gain_mask;
> >> +	int ret;
> >>
> >>  	switch (w->reg) {
> >>  	case DA7219_MIXOUT_L_CTRL:
> >> @@ -904,15 +950,20 @@ static int da7219_mixout_event(struct
> >> snd_soc_dapm_widget *w,
> >>  	switch (event) {
> >>  	case SND_SOC_DAPM_PRE_PMD:
> >>  		/* Enable minimum gain on HP to avoid pops */
> >> -		snd_soc_component_update_bits(component, hp_ctrl,
> >> min_gain_mask,
> >> -				    min_gain_mask);
> >> +		ret = snd_soc_component_update_bits(component, hp_ctrl,
> >> +				    min_gain_mask, min_gain_mask);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >>  		msleep(DA7219_MIN_GAIN_DELAY);
> >>
> >>  		break;
> >>  	case SND_SOC_DAPM_POST_PMU:
> >>  		/* Remove minimum gain on HP */
> >> -		snd_soc_component_update_bits(component, hp_ctrl,
> >> min_gain_mask, 0);
> >> +		ret = snd_soc_component_update_bits(component, hp_ctrl,
> >> +				    min_gain_mask, 0);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >>  		break;
> >>  	}
> >> @@ -925,21 +976,29 @@ static int da7219_gain_ramp_event(struct
> >> snd_soc_dapm_widget *w,  {
> >>  	struct snd_soc_component *component =
> >> snd_soc_dapm_to_component(w->dapm);
> >>  	struct da7219_priv *da7219 =
> >> snd_soc_component_get_drvdata(component);
> >> +	int ret;
> >>
> >>  	switch (event) {
> >>  	case SND_SOC_DAPM_PRE_PMU:
> >>  	case SND_SOC_DAPM_PRE_PMD:
> >>  		/* Ensure nominal gain ramping for DAPM sequence */
> >> -		da7219->gain_ramp_ctrl =
> >> -			snd_soc_component_read32(component,
> >> DA7219_GAIN_RAMP_CTRL);
> >> -		snd_soc_component_write(component,
> >> DA7219_GAIN_RAMP_CTRL,
> >> -			      DA7219_GAIN_RAMP_RATE_NOMINAL);
> >> +		da7219->gain_ramp_ctrl =
> >> snd_soc_component_read32(component,
> >> +						DA7219_GAIN_RAMP_CTRL);
> >> +		if (da7219->gain_ramp_ctrl < 0)
> >> +			return da7219->gain_ramp_ctrl;
> >> +		 ret = snd_soc_component_write(component,
> >> +				DA7219_GAIN_RAMP_CTRL,
> >> +				DA7219_GAIN_RAMP_RATE_NOMINAL);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		break;
> >>  	case SND_SOC_DAPM_POST_PMU:
> >>  	case SND_SOC_DAPM_POST_PMD:
> >>  		/* Restore previous gain ramp settings */
> >> -		snd_soc_component_write(component,
> >> DA7219_GAIN_RAMP_CTRL,
> >> +		ret = snd_soc_component_write(component,
> >> DA7219_GAIN_RAMP_CTRL,
> >>  			      da7219->gain_ramp_ctrl);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		break;
> >>  	}
> >>
> >> @@ -1177,13 +1236,17 @@ static int da7219_set_dai_sysclk(struct
> >> snd_soc_dai *codec_dai,
> >>
> >>  	switch (clk_id) {
> >>  	case DA7219_CLKSRC_MCLK_SQR:
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_PLL_CTRL,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_PLL_CTRL,
> >>  				    DA7219_PLL_MCLK_SQR_EN_MASK,
> >>  				    DA7219_PLL_MCLK_SQR_EN_MASK);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		break;
> >>  	case DA7219_CLKSRC_MCLK:
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_PLL_CTRL,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_PLL_CTRL,
> >>  				    DA7219_PLL_MCLK_SQR_EN_MASK, 0);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		break;
> >>  	default:
> >>  		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
> @@
> >> -1219,6 +1282,7 @@ int da7219_set_pll(struct snd_soc_component
> >> *component, int source, unsigned int
> >>  	u8 pll_frac_top, pll_frac_bot, pll_integer;
> >>  	u32 freq_ref;
> >>  	u64 frac_div;
> >> +	int ret;
> >>
> >>  	/* Verify 2MHz - 54MHz MCLK provided, and set input divider */
> >>  	if (da7219->mclk_rate < 2000000) {
> >> @@ -1252,9 +1316,11 @@ int da7219_set_pll(struct snd_soc_component
> >> *component, int source, unsigned int
> >>  	switch (source) {
> >>  	case DA7219_SYSCLK_MCLK:
> >>  		pll_ctrl |= DA7219_PLL_MODE_BYPASS;
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_PLL_CTRL,
> >> +		ret = snd_soc_component_update_bits(component,
> >> DA7219_PLL_CTRL,
> >>  				    DA7219_PLL_INDIV_MASK |
> >>  				    DA7219_PLL_MODE_MASK, pll_ctrl);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		return 0;
> >>  	case DA7219_SYSCLK_PLL:
> >>  		pll_ctrl |= DA7219_PLL_MODE_NORMAL; @@ -1275,12 +1341,23
> @@ int
> >> da7219_set_pll(struct snd_soc_component *component, int source,
> >> unsigned int
> >>  	pll_frac_bot = (frac_div) & DA7219_BYTE_MASK;
> >>
> >>  	/* Write PLL config & dividers */
> >> -	snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
> >> pll_frac_top);
> >> -	snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
> >> pll_frac_bot);
> >> -	snd_soc_component_write(component, DA7219_PLL_INTEGER,
> >> pll_integer);
> >> -	snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
> >> +	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
> >> +				pll_frac_top);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
> >> +				pll_frac_bot);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_write(component, DA7219_PLL_INTEGER,
> >> +				pll_integer);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
> >>  			    DA7219_PLL_INDIV_MASK |
> >> DA7219_PLL_MODE_MASK,
> >>  			    pll_ctrl);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	return 0;
> >>  }
> >> @@ -1304,6 +1381,7 @@ static int da7219_set_dai_fmt(struct
> >> snd_soc_dai *codec_dai, unsigned int fmt)
> >>  	struct snd_soc_component *component = codec_dai->component;
> >>  	struct da7219_priv *da7219 =
> >> snd_soc_component_get_drvdata(component);
> >>  	u8 dai_clk_mode = 0, dai_ctrl = 0;
> >> +	int ret;
> >>
> >>  	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> >>  	case SND_SOC_DAIFMT_CBM_CFM:
> >> @@ -1379,12 +1457,16 @@ static int da7219_set_dai_fmt(struct
> >> snd_soc_dai *codec_dai, unsigned int fmt)
> >>  	/* By default 64 BCLKs per WCLK is supported */
> >>  	dai_clk_mode |= DA7219_DAI_BCLKS_PER_WCLK_64;
> >>
> >> -	snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >>  			    DA7219_DAI_BCLKS_PER_WCLK_MASK |
> >>  			    DA7219_DAI_CLK_POL_MASK |
> >> DA7219_DAI_WCLK_POL_MASK,
> >>  			    dai_clk_mode);
> >> -	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> >> DA7219_DAI_FORMAT_MASK,
> >> -			    dai_ctrl);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> >> +			    DA7219_DAI_FORMAT_MASK, dai_ctrl);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	return 0;
> >>  }
> >> @@ -1398,15 +1480,22 @@ static int da7219_set_dai_tdm_slot(struct
> >> snd_soc_dai *dai,
> >>  	u8 dai_bclks_per_wclk;
> >>  	u16 offset;
> >>  	u32 frame_size;
> >> +	int ret;
> >>
> >>  	/* No channels enabled so disable TDM, revert to 64-bit frames */
> >>  	if (!tx_mask) {
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_DAI_TDM_CTRL,
> >> +		ret = snd_soc_component_update_bits(component,
> >> +				    DA7219_DAI_TDM_CTRL,
> >>  				    DA7219_DAI_TDM_CH_EN_MASK |
> >>  				    DA7219_DAI_TDM_MODE_EN_MASK, 0);
> >> -		snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		ret = snd_soc_component_update_bits(component,
> >> +				    DA7219_DAI_CLK_MODE,
> >>  				    DA7219_DAI_BCLKS_PER_WCLK_MASK,
> >>  				    DA7219_DAI_BCLKS_PER_WCLK_64);
> >> +		if (ret < 0)
> >> +			return ret;
> >>  		return 0;
> >>  	}
> >>
> >> @@ -1444,19 +1533,25 @@ static int da7219_set_dai_tdm_slot(struct
> >> snd_soc_dai *dai,
> >>  		return -EINVAL;
> >>  	}
> >>
> >> -	snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >>  			    DA7219_DAI_BCLKS_PER_WCLK_MASK,
> >>  			    dai_bclks_per_wclk);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	offset = cpu_to_le16(rx_mask);
> >> -	regmap_bulk_write(da7219->regmap, DA7219_DAI_OFFSET_LOWER,
> >> +	ret = regmap_bulk_write(da7219->regmap,
> >> DA7219_DAI_OFFSET_LOWER,
> >>  			  &offset, sizeof(offset));
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >> -	snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAI_TDM_CTRL,
> >>  			    DA7219_DAI_TDM_CH_EN_MASK |
> >>  			    DA7219_DAI_TDM_MODE_EN_MASK,
> >>  			    (tx_mask << DA7219_DAI_TDM_CH_EN_SHIFT) |
> >>  			    DA7219_DAI_TDM_MODE_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	return 0;
> >>  }
> >> @@ -1468,6 +1563,7 @@ static int da7219_hw_params(struct
> >> snd_pcm_substream *substream,
> >>  	struct snd_soc_component *component = dai->component;
> >>  	u8 dai_ctrl = 0, fs;
> >>  	unsigned int channels;
> >> +	int ret;
> >>
> >>  	switch (params_width(params)) {
> >>  	case 16:
> >> @@ -1533,11 +1629,15 @@ static int da7219_hw_params(struct
> >> snd_pcm_substream *substream,
> >>  		return -EINVAL;
> >>  	}
> >>
> >> -	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> >> +	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
> >>  			    DA7219_DAI_WORD_LENGTH_MASK |
> >>  			    DA7219_DAI_CH_NUM_MASK,
> >>  			    dai_ctrl);
> >> -	snd_soc_component_write(component, DA7219_SR, fs);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_write(component, DA7219_SR, fs);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	return 0;
> >>  }
> >> @@ -1692,9 +1792,12 @@ static int da7219_set_bias_level(struct
> >> snd_soc_component *component,
> >>  	case SND_SOC_BIAS_STANDBY:
> >>  		if (snd_soc_component_get_bias_level(component) ==
> >> SND_SOC_BIAS_OFF)
> >>  			/* Master bias */
> >> -			snd_soc_component_update_bits(component,
> >> DA7219_REFERENCES,
> >> +			ret = snd_soc_component_update_bits(component,
> >> +					    DA7219_REFERENCES,
> >>  					    DA7219_BIAS_EN_MASK,
> >>  					    DA7219_BIAS_EN_MASK);
> >> +			if (ret < 0)
> >> +				return ret;
> >>
> >>  		if (snd_soc_component_get_bias_level(component) ==
> >> SND_SOC_BIAS_PREPARE) {
> >>  			/* Remove MCLK */
> >> @@ -1705,8 +1808,11 @@ static int da7219_set_bias_level(struct
> >> snd_soc_component *component,
> >>  	case SND_SOC_BIAS_OFF:
> >>  		/* Only disable master bias if we're not a wake-up source */
> >>  		if (!da7219->wakeup_source)
> >> -			snd_soc_component_update_bits(component,
> >> DA7219_REFERENCES,
> >> +			ret = snd_soc_component_update_bits(component,
> >> +					    DA7219_REFERENCES,
> >>  					    DA7219_BIAS_EN_MASK, 0);
> >> +			if (ret < 0)
> >> +				return ret;
> >>
> >>  		break;
> >>  	}
> >> @@ -1754,10 +1860,16 @@ static int da7219_handle_supplies(struct
> >> snd_soc_component *component)
> >>  	}
> >>
> >>  	/* Ensure device in active mode */
> >> -	snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
> >> DA7219_SYSTEM_ACTIVE_MASK);
> >> +	ret = snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
> >> +				DA7219_SYSTEM_ACTIVE_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Update IO voltage level range */
> >> -	snd_soc_component_write(component, DA7219_IO_CTRL,
> >> io_voltage_lvl);
> >> +	ret = snd_soc_component_write(component, DA7219_IO_CTRL,
> >> +				io_voltage_lvl);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	return 0;
> >>  }
> >> @@ -1768,10 +1880,13 @@ static int da7219_dai_clks_prepare(struct
> >> clk_hw
> >> *hw)
> >>  	struct da7219_priv *da7219 =
> >>  		container_of(hw, struct da7219_priv, dai_clks_hw);
> >>  	struct snd_soc_component *component = da7219->aad->component;
> >> +	int ret;
> >>
> >> -	snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >>  				      DA7219_DAI_CLK_EN_MASK,
> >>  				      DA7219_DAI_CLK_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	return 0;
> >>  }
> >> @@ -1781,9 +1896,12 @@ static void da7219_dai_clks_unprepare(struct
> >> clk_hw
> >> *hw)
> >>  	struct da7219_priv *da7219 =
> >>  		container_of(hw, struct da7219_priv, dai_clks_hw);
> >>  	struct snd_soc_component *component = da7219->aad->component;
> >> +	int ret;
> >>
> >> -	snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAI_CLK_MODE,
> >>  				      DA7219_DAI_CLK_EN_MASK, 0);
> >> +	if (ret < 0)
> >> +		dev_err(component->dev, "Failed to disable clk: %d\n", ret);
> >>  }
> >>
> >>  static int da7219_dai_clks_is_prepared(struct clk_hw *hw) @@ -1791,9
> >> +1909,11 @@ static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
> >>  	struct da7219_priv *da7219 =
> >>  		container_of(hw, struct da7219_priv, dai_clks_hw);
> >>  	struct snd_soc_component *component = da7219->aad->component;
> >> -	u8 clk_reg;
> >> +	int clk_reg;
> >>
> >>  	clk_reg = snd_soc_component_read32(component,
> >> DA7219_DAI_CLK_MODE);
> >> +	if (clk_reg < 0)
> >> +		return clk_reg;
> >>
> >>  	return !!(clk_reg & DA7219_DAI_CLK_EN_MASK);  } @@ -1844,10
> >> +1964,11 @@ static void da7219_register_dai_clks(struct
> >> +snd_soc_component
> >> *component)  static inline void da7219_register_dai_clks(struct
> >> snd_soc_component *component) {}  #endif /* CONFIG_COMMON_CLK */
> >>
> >> -static void da7219_handle_pdata(struct snd_soc_component *component)
> >> +static int da7219_handle_pdata(struct snd_soc_component *component)
> >>  {
> >>  	struct da7219_priv *da7219 =
> >> snd_soc_component_get_drvdata(component);
> >>  	struct da7219_pdata *pdata = da7219->pdata;
> >> +	int ret;
> >>
> >>  	if (pdata) {
> >>  		u8 micbias_lvl = 0;
> >> @@ -1869,7 +1990,10 @@ static void da7219_handle_pdata(struct
> >> snd_soc_component *component)
> >>  			break;
> >>  		}
> >>
> >> -		snd_soc_component_write(component, DA7219_MICBIAS_CTRL,
> >> micbias_lvl);
> >> +		ret = snd_soc_component_write(component,
> >> DA7219_MICBIAS_CTRL,
> >> +					micbias_lvl);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >>  		/*
> >>  		 * Calculate delay required to compensate for DC offset in @@ -
> >> 1884,11 +2008,15 @@ static void da7219_handle_pdata(struct
> >> snd_soc_component *component)
> >>  		case DA7219_MIC_AMP_IN_SEL_DIFF:
> >>  		case DA7219_MIC_AMP_IN_SEL_SE_P:
> >>  		case DA7219_MIC_AMP_IN_SEL_SE_N:
> >> -			snd_soc_component_write(component,
> >> DA7219_MIC_1_SELECT,
> >> -				      pdata->mic_amp_in_sel);
> >> +			ret = snd_soc_component_write(component,
> >> +					DA7219_MIC_1_SELECT,
> >> +					pdata->mic_amp_in_sel);
> >> +			if (ret < 0)
> >> +				return ret;
> >>  			break;
> >>  		}
> >>  	}
> >> +	return 0;
> >>  }
> >>
> >>  static struct reg_sequence da7219_rev_aa_patch[] = { @@ -1934,7
> >> +2062,9 @@ static int da7219_probe(struct snd_soc_component
> *component)
> >>  	if (!da7219->pdata)
> >>  		da7219->pdata = da7219_fw_to_pdata(component);
> >>
> >> -	da7219_handle_pdata(component);
> >> +	ret = da7219_handle_pdata(component);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Check if MCLK provided */
> >>  	da7219->mclk = devm_clk_get(component->dev, "mclk"); @@ -1948,36
> >> +2078,57 @@ static int da7219_probe(struct snd_soc_component
> >> +*component)
> >>  	}
> >>
> >>  	/* Default PC counter to free-running */
> >> -	snd_soc_component_update_bits(component, DA7219_PC_COUNT,
> >> DA7219_PC_FREERUN_MASK,
> >> -			    DA7219_PC_FREERUN_MASK);
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_PC_COUNT,
> >> +			DA7219_PC_FREERUN_MASK,
> >> DA7219_PC_FREERUN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Default gain ramping */
> >> -	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_MIXIN_L_CTRL,
> >>  			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK,
> >>  			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
> >> DA7219_ADC_L_RAMP_EN_MASK,
> >> -			    DA7219_ADC_L_RAMP_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_DAC_L_CTRL,
> >> DA7219_DAC_L_RAMP_EN_MASK,
> >> -			    DA7219_DAC_L_RAMP_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_DAC_R_CTRL,
> >> DA7219_DAC_R_RAMP_EN_MASK,
> >> -			    DA7219_DAC_R_RAMP_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_ADC_L_CTRL,
> >> +			DA7219_ADC_L_RAMP_EN_MASK,
> >> DA7219_ADC_L_RAMP_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAC_L_CTRL,
> >> +			DA7219_DAC_L_RAMP_EN_MASK,
> >> DA7219_DAC_L_RAMP_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_DAC_R_CTRL,
> >> +			DA7219_DAC_R_RAMP_EN_MASK,
> >> DA7219_DAC_R_RAMP_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_HP_L_CTRL,
> >>  			    DA7219_HP_L_AMP_RAMP_EN_MASK,
> >>  			    DA7219_HP_L_AMP_RAMP_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_HP_R_CTRL,
> >>  			    DA7219_HP_R_AMP_RAMP_EN_MASK,
> >>  			    DA7219_HP_R_AMP_RAMP_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Default minimum gain on HP to avoid pops during DAPM sequencing
> >> */
> >> -	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_HP_L_CTRL,
> >>  			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK,
> >>  			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK);
> >> -	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = snd_soc_component_update_bits(component,
> >> DA7219_HP_R_CTRL,
> >>  			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK,
> >>  			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Default infinite tone gen, start/stop by Kcontrol */
> >> -	snd_soc_component_write(component, DA7219_TONE_GEN_CYCLES,
> >> DA7219_BEEP_CYCLES_MASK);
> >> +	ret = snd_soc_component_write(component,
> >> DA7219_TONE_GEN_CYCLES,
> >> +			DA7219_BEEP_CYCLES_MASK);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	/* Initialise AAD block */
> >>  	ret = da7219_aad_init(component);
> >> @@ -2221,16 +2372,28 @@ static int da7219_i2c_probe(struct i2c_client *i2c,
> >>  	regcache_cache_bypass(da7219->regmap, true);
> >>
> >>  	/* Disable audio paths if still active from previous start */
> >> -	regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> >> &system_active);
> >> +	ret = regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> >> &system_active);
> >> +	if (ret < 0)
> >> +		return ret;
> >>  	if (system_active) {
> >> -		regmap_write(da7219->regmap, DA7219_GAIN_RAMP_CTRL,
> >> +		ret = regmap_write(da7219->regmap,
> >> DA7219_GAIN_RAMP_CTRL,
> >>  			     DA7219_GAIN_RAMP_RATE_NOMINAL);
> >> -		regmap_write(da7219->regmap,
> >> DA7219_SYSTEM_MODES_INPUT, 0x00);
> >> -		regmap_write(da7219->regmap,
> >> DA7219_SYSTEM_MODES_OUTPUT, 0x01);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		ret = regmap_write(da7219->regmap,
> >> DA7219_SYSTEM_MODES_INPUT,
> >> +				0x00);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		ret = regmap_write(da7219->regmap,
> >> DA7219_SYSTEM_MODES_OUTPUT,
> >> +				0x01);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >>  		for (i = 0; i < DA7219_SYS_STAT_CHECK_RETRIES; ++i) {
> >> -			regmap_read(da7219->regmap,
> >> DA7219_SYSTEM_STATUS,
> >> +			ret = regmap_read(da7219->regmap,
> >> DA7219_SYSTEM_STATUS,
> >>  				    &system_status);
> >> +			if (ret < 0)
> >> +				return ret;
> >>  			if (!system_status)
> >>  				break;
> >>
> >> @@ -2239,13 +2402,19 @@ static int da7219_i2c_probe(struct i2c_client *i2c,
> >>  	}
> >>
> >>  	/* Soft reset component */
> >> -	regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
> >> +	ret = regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
> >>  			  DA7219_ACCDET_EN_MASK, 0);
> >> -	regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
> >>  			  DA7219_CIF_REG_SOFT_RESET_MASK,
> >>  			  DA7219_CIF_REG_SOFT_RESET_MASK);
> >> -	regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	ret = regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
> >>  			  DA7219_SYSTEM_ACTIVE_MASK, 0);
> >> +	if (ret < 0)
> >> +		return ret;
> >>
> >>  	regcache_cache_bypass(da7219->regmap, false);
> >>
> >> diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
> >> index
> >> 3a00686..0cf78a7 100644
> >> --- a/sound/soc/codecs/da7219.h
> >> +++ b/sound/soc/codecs/da7219.h
> >> @@ -832,7 +832,7 @@ struct da7219_priv {
> >>  	bool alc_en;
> >>  	bool micbias_on_event;
> >>  	unsigned int mic_pga_delay;
> >> -	u8 gain_ramp_ctrl;
> >> +	int gain_ramp_ctrl;
> >>  };
> >>
> >>  int da7219_set_pll(struct snd_soc_component *component, int source,
> >> unsigned int fout);
> >> --
> >> 1.9.1

Mark Brown Dec. 5, 2018, 11:28 a.m. UTC | #4

On Wed, Dec 05, 2018 at 10:21:04AM +0000, Adam Thomson wrote:

> If the previous I2C access failed, how can we be sure that the write back to HW
> of 0xFF even succeeds? More importantly these error returns won't necessarily
> stop subsequent calls to controls within the Codec I believe, so you could still
> see unwanted writes to HW via I2C, if I2C is sporadically operational. Again I
> don't see this update resolving that. The key thing is to resolve why even just
> one I2C transaction fails.

Right, it's just not clear what we can constructively do if the I2C bus
falls to bits other than log things and the I2C controllers will
generally do that themselves.  There's no guarantee what made it
through to the device or what will in future make it through to the
device.

Daniel Kurtz Dec. 5, 2018, 5:50 p.m. UTC | #5

On Wed, Dec 5, 2018 at 4:28 AM Mark Brown <[e-mail]> wrote:
>
> On Wed, Dec 05, 2018 at 10:21:04AM +0000, Adam Thomson wrote:
>
> > If the previous I2C access failed, how can we be sure that the write back to HW
> > of 0xFF even succeeds? More importantly these error returns won't necessarily
> > stop subsequent calls to controls within the Codec I believe, so you could still
> > see unwanted writes to HW via I2C, if I2C is sporadically operational. Again I
> > don't see this update resolving that. The key thing is to resolve why even just
> > one I2C transaction fails.
>
> Right, it's just not clear what we can constructively do if the I2C bus
> falls to bits other than log things and the I2C controllers will
> generally do that themselves.  There's no guarantee what made it
> through to the device or what will in future make it through to the
> device.

I agree, there is no guarantee here once things have gone wrong, and
the concerns above are reasonable.  However, in the real world, I2C
transactions do sometimes fail for various reasons.  The I2C (and
other bus) APIs have ways of reporting up their errors so callers can
take appropriate action.  This codec driver can run on all kinds of
hardware that can experience transient I2C errors, thus it sounds like
a reasonable idea to have the driver do some error checking on the
APIs it calls and take whatever action it can.  Just ignoring the
errors and proceeding like nothing is wrong is one option, but we can
probably do a little better by at least checking for errors, abort the
current operation, and pass up errors to higher layers when an i2c
transaction failure is detected.  If nothing else, this would enable
higher policy layers to take appropriate corrective action - for
example, if there is an i2c error when configuring a codec, it seems
advisable to report this up so that a machine driver would know to
abort and not turn on downstream amplifiers [I am assuming here that
something like this would happen, I don't know if the sound stack
really works this way].

Once the default "check, abort and report" error checking is in place,
we could perhaps think about actions that the driver could take to
recover from various failures, such as resetting the device or
unwinding previous transactions before aborting, or retrying....  but
those are all policy, and this patch is more mechanism that enables
policy.

As for this patch itself, I would recommend using
snd_soc_component_read rather than snd_soc_component_read32() which is
fundamentally broken and afaict should be removed, since there is no
way to distinguish between its error return "(u32)-1" and the valid
register value 0xffffffff.  (Edit: I notice that you did this in v2,
so please ignore, but still replying here since this seems to be where
the active discussion is).

-Dan

Mark Brown Dec. 6, 2018, 7:56 p.m. UTC | #6

On Wed, Dec 05, 2018 at 10:50:29AM -0700, Daniel Kurtz wrote:

> I agree, there is no guarantee here once things have gone wrong, and
> the concerns above are reasonable.  However, in the real world, I2C
> transactions do sometimes fail for various reasons.  The I2C (and

It's *vanishingly* rare and like I say we don't have any constructive
recovery plans - I've never seen a practical system that had any better
idea than hoping the user noticed a problem and reboots.  Probably the
best we can really do is try to reset the device, or at least resync the
register map.  That would probably do something useful in the
vanishingly rare case where it were a singular glitch, at the cost of
obvious and painful user visible issues (which would probably be
happening anyway).  The nearest I've seen to that is one of the CODEC
drivers which has watchdog code to monitor the device in case it
spontaneously reboots and will resync the register cache if that happened.

I'm gathering that there are Chromebooks that do have I/O problems here?

> other bus) APIs have ways of reporting up their errors so callers can
> take appropriate action.  This codec driver can run on all kinds of
> hardware that can experience transient I2C errors, thus it sounds like
> a reasonable idea to have the driver do some error checking on the
> APIs it calls and take whatever action it can.  Just ignoring the
> errors and proceeding like nothing is wrong is one option, but we can
> probably do a little better by at least checking for errors, abort the
> current operation, and pass up errors to higher layers when an i2c
> transaction failure is detected.  If nothing else, this would enable
> higher policy layers to take appropriate corrective action - for
> example, if there is an i2c error when configuring a codec, it seems
> advisable to report this up so that a machine driver would know to
> abort and not turn on downstream amplifiers [I am assuming here that
> something like this would happen, I don't know if the sound stack
> really works this way].

Yeah, so this is partly why I'm not super thrilled here - none of the
layers currently have any especially bright ideas about how to handle
things and giving up part way through an operation and returning an
error without any attempt to unwind or recover feels like it's just
trying to give a false sense of security.  

> Once the default "check, abort and report" error checking is in place,
> we could perhaps think about actions that the driver could take to
> recover from various failures, such as resetting the device or
> unwinding previous transactions before aborting, or retrying....  but
> those are all policy, and this patch is more mechanism that enables
> policy.

But is this going to be a useful way of handling such policy and is any
work on that from whoever has these unreliable systems likely to be
forthcoming?  We're going to end up with partially done reconfigurations
in the register cache which is a potential issue for recovery strategies
based around resyncing that, it's not so bad on things like starting a
stream but bias level reconfigurations could be fun.

Patch

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index e46e9f4..6757129 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -312,69 +312,103 @@  static int da7219_enum_locked_put(struct snd_kcontrol *kcontrol,
 }
 
 /* ALC */
-static void da7219_alc_calib(struct snd_soc_component *component)
+static int da7219_alc_calib(struct snd_soc_component *component)
 {
-	u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
+	int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
+	int ret;
 
 	/* Save current state of mic control register */
 	mic_ctrl = snd_soc_component_read32(component, DA7219_MIC_1_CTRL);
+	if (mic_ctrl < 0)
+		return mic_ctrl;
 
 	/* Save current state of input mixer control register */
 	mixin_ctrl = snd_soc_component_read32(component, DA7219_MIXIN_L_CTRL);
+	if (mixin_ctrl < 0)
+		return mixin_ctrl;
 
 	/* Save current state of input ADC control register */
 	adc_ctrl = snd_soc_component_read32(component, DA7219_ADC_L_CTRL);
+	if (adc_ctrl < 0)
+		return adc_ctrl;
 
 	/* Enable then Mute MIC PGAs */
-	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL, DA7219_MIC_1_AMP_EN_MASK,
+	ret = snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
+			    DA7219_MIC_1_AMP_EN_MASK,
 			    DA7219_MIC_1_AMP_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
+	if (ret < 0)
+		return ret;
+
+	ret = snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
 			    DA7219_MIC_1_AMP_MUTE_EN_MASK,
 			    DA7219_MIC_1_AMP_MUTE_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Enable input mixers unmuted */
-	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
+	ret = snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
 			    DA7219_MIXIN_L_AMP_EN_MASK |
 			    DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
 			    DA7219_MIXIN_L_AMP_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Enable input filters unmuted */
-	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
+	ret = snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
 			    DA7219_ADC_L_MUTE_EN_MASK | DA7219_ADC_L_EN_MASK,
 			    DA7219_ADC_L_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Perform auto calibration */
-	snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+	ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
 			    DA7219_ALC_AUTO_CALIB_EN_MASK,
 			    DA7219_ALC_AUTO_CALIB_EN_MASK);
+	if (ret < 0)
+		return ret;
 	do {
 		calib_ctrl = snd_soc_component_read32(component, DA7219_ALC_CTRL1);
+		if (calib_ctrl < 0)
+			return calib_ctrl;
 	} while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
 
 	/* If auto calibration fails, disable DC offset, hybrid ALC */
 	if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
 		dev_warn(component->dev,
 			 "ALC auto calibration failed with overflow\n");
-		snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+		ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
 				    DA7219_ALC_OFFSET_EN_MASK |
 				    DA7219_ALC_SYNC_MODE_MASK, 0);
+		if (ret < 0)
+			return ret;
 	} else {
 		/* Enable DC offset cancellation, hybrid mode */
-		snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+		ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
 				    DA7219_ALC_OFFSET_EN_MASK |
 				    DA7219_ALC_SYNC_MODE_MASK,
 				    DA7219_ALC_OFFSET_EN_MASK |
 				    DA7219_ALC_SYNC_MODE_MASK);
+		if (ret < 0)
+			return ret;
 	}
 
 	/* Restore input filter control register to original state */
-	snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
+	ret = snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
+	if (ret < 0)
+		return ret;
 
 	/* Restore input mixer control registers to original state */
-	snd_soc_component_write(component, DA7219_MIXIN_L_CTRL, mixin_ctrl);
+	ret = snd_soc_component_write(component, DA7219_MIXIN_L_CTRL,
+				mixin_ctrl);
+	if (ret < 0)
+		return ret;
 
 	/* Restore MIC control registers to original states */
-	snd_soc_component_write(component, DA7219_MIC_1_CTRL, mic_ctrl);
+	ret = snd_soc_component_write(component, DA7219_MIC_1_CTRL, mic_ctrl);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol,
@@ -391,7 +425,7 @@  static int da7219_mixin_gain_put(struct snd_kcontrol *kcontrol,
 	 * make sure calibrated offsets are updated.
 	 */
 	if ((ret == 1) && (da7219->alc_en))
-		da7219_alc_calib(component);
+		ret = da7219_alc_calib(component);
 
 	return ret;
 }
@@ -401,11 +435,14 @@  static int da7219_alc_sw_put(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
 	struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 
 	/* Force ALC offset calibration if enabling ALC */
 	if ((ucontrol->value.integer.value[0]) && (!da7219->alc_en)) {
-		da7219_alc_calib(component);
+		ret = da7219_alc_calib(component);
+		if (ret < 0)
+			return ret;
 		da7219->alc_en = true;
 	} else {
 		da7219->alc_en = false;
@@ -813,16 +850,20 @@  static int da7219_dai_event(struct snd_soc_dapm_widget *w,
 					return ret;
 				}
 			} else {
-				snd_soc_component_update_bits(component,
+				ret = snd_soc_component_update_bits(component,
 							      DA7219_DAI_CLK_MODE,
 							      DA7219_DAI_CLK_EN_MASK,
 							      DA7219_DAI_CLK_EN_MASK);
+				if (ret < 0)
+					return ret;
 			}
 		}
 
 		/* PC synchronised to DAI */
-		snd_soc_component_update_bits(component, DA7219_PC_COUNT,
+		ret = snd_soc_component_update_bits(component, DA7219_PC_COUNT,
 				    DA7219_PC_FREERUN_MASK, 0);
+		if (ret < 0)
+			return ret;
 
 		/* Slave mode, if SRM not enabled no need for status checks */
 		pll_ctrl = snd_soc_component_read32(component, DA7219_PLL_CTRL);
@@ -846,19 +887,23 @@  static int da7219_dai_event(struct snd_soc_dapm_widget *w,
 		return 0;
 	case SND_SOC_DAPM_POST_PMD:
 		/* PC free-running */
-		snd_soc_component_update_bits(component, DA7219_PC_COUNT,
+		ret = snd_soc_component_update_bits(component, DA7219_PC_COUNT,
 				    DA7219_PC_FREERUN_MASK,
 				    DA7219_PC_FREERUN_MASK);
+		if (ret < 0)
+			return ret;
 
 		/* Disable DAI clks if in master mode */
 		if (da7219->master) {
 			if (da7219->dai_clks)
 				clk_disable_unprepare(da7219->dai_clks);
 			else
-				snd_soc_component_update_bits(component,
+				ret = snd_soc_component_update_bits(component,
 							      DA7219_DAI_CLK_MODE,
 							      DA7219_DAI_CLK_EN_MASK,
 							      0);
+				if (ret < 0)
+					return ret;
 		}
 
 		return 0;
@@ -887,6 +932,7 @@  static int da7219_mixout_event(struct snd_soc_dapm_widget *w,
 {
 	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	u8 hp_ctrl, min_gain_mask;
+	int ret;
 
 	switch (w->reg) {
 	case DA7219_MIXOUT_L_CTRL:
@@ -904,15 +950,20 @@  static int da7219_mixout_event(struct snd_soc_dapm_widget *w,
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMD:
 		/* Enable minimum gain on HP to avoid pops */
-		snd_soc_component_update_bits(component, hp_ctrl, min_gain_mask,
-				    min_gain_mask);
+		ret = snd_soc_component_update_bits(component, hp_ctrl,
+				    min_gain_mask, min_gain_mask);
+		if (ret < 0)
+			return ret;
 
 		msleep(DA7219_MIN_GAIN_DELAY);
 
 		break;
 	case SND_SOC_DAPM_POST_PMU:
 		/* Remove minimum gain on HP */
-		snd_soc_component_update_bits(component, hp_ctrl, min_gain_mask, 0);
+		ret = snd_soc_component_update_bits(component, hp_ctrl,
+				    min_gain_mask, 0);
+		if (ret < 0)
+			return ret;
 
 		break;
 	}
@@ -925,21 +976,29 @@  static int da7219_gain_ramp_event(struct snd_soc_dapm_widget *w,
 {
 	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
 	case SND_SOC_DAPM_PRE_PMD:
 		/* Ensure nominal gain ramping for DAPM sequence */
-		da7219->gain_ramp_ctrl =
-			snd_soc_component_read32(component, DA7219_GAIN_RAMP_CTRL);
-		snd_soc_component_write(component, DA7219_GAIN_RAMP_CTRL,
-			      DA7219_GAIN_RAMP_RATE_NOMINAL);
+		da7219->gain_ramp_ctrl = snd_soc_component_read32(component,
+						DA7219_GAIN_RAMP_CTRL);
+		if (da7219->gain_ramp_ctrl < 0)
+			return da7219->gain_ramp_ctrl;
+		 ret = snd_soc_component_write(component,
+				DA7219_GAIN_RAMP_CTRL,
+				DA7219_GAIN_RAMP_RATE_NOMINAL);
+		if (ret < 0)
+			return ret;
 		break;
 	case SND_SOC_DAPM_POST_PMU:
 	case SND_SOC_DAPM_POST_PMD:
 		/* Restore previous gain ramp settings */
-		snd_soc_component_write(component, DA7219_GAIN_RAMP_CTRL,
+		ret = snd_soc_component_write(component, DA7219_GAIN_RAMP_CTRL,
 			      da7219->gain_ramp_ctrl);
+		if (ret < 0)
+			return ret;
 		break;
 	}
 
@@ -1177,13 +1236,17 @@  static int da7219_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 
 	switch (clk_id) {
 	case DA7219_CLKSRC_MCLK_SQR:
-		snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
+		ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
 				    DA7219_PLL_MCLK_SQR_EN_MASK,
 				    DA7219_PLL_MCLK_SQR_EN_MASK);
+		if (ret < 0)
+			return ret;
 		break;
 	case DA7219_CLKSRC_MCLK:
-		snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
+		ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
 				    DA7219_PLL_MCLK_SQR_EN_MASK, 0);
+		if (ret < 0)
+			return ret;
 		break;
 	default:
 		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
@@ -1219,6 +1282,7 @@  int da7219_set_pll(struct snd_soc_component *component, int source, unsigned int
 	u8 pll_frac_top, pll_frac_bot, pll_integer;
 	u32 freq_ref;
 	u64 frac_div;
+	int ret;
 
 	/* Verify 2MHz - 54MHz MCLK provided, and set input divider */
 	if (da7219->mclk_rate < 2000000) {
@@ -1252,9 +1316,11 @@  int da7219_set_pll(struct snd_soc_component *component, int source, unsigned int
 	switch (source) {
 	case DA7219_SYSCLK_MCLK:
 		pll_ctrl |= DA7219_PLL_MODE_BYPASS;
-		snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
+		ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
 				    DA7219_PLL_INDIV_MASK |
 				    DA7219_PLL_MODE_MASK, pll_ctrl);
+		if (ret < 0)
+			return ret;
 		return 0;
 	case DA7219_SYSCLK_PLL:
 		pll_ctrl |= DA7219_PLL_MODE_NORMAL;
@@ -1275,12 +1341,23 @@  int da7219_set_pll(struct snd_soc_component *component, int source, unsigned int
 	pll_frac_bot = (frac_div) & DA7219_BYTE_MASK;
 
 	/* Write PLL config & dividers */
-	snd_soc_component_write(component, DA7219_PLL_FRAC_TOP, pll_frac_top);
-	snd_soc_component_write(component, DA7219_PLL_FRAC_BOT, pll_frac_bot);
-	snd_soc_component_write(component, DA7219_PLL_INTEGER, pll_integer);
-	snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
+	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_TOP,
+				pll_frac_top);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_write(component, DA7219_PLL_FRAC_BOT,
+				pll_frac_bot);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_write(component, DA7219_PLL_INTEGER,
+				pll_integer);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_PLL_CTRL,
 			    DA7219_PLL_INDIV_MASK | DA7219_PLL_MODE_MASK,
 			    pll_ctrl);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -1304,6 +1381,7 @@  static int da7219_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
 	struct snd_soc_component *component = codec_dai->component;
 	struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
 	u8 dai_clk_mode = 0, dai_ctrl = 0;
+	int ret;
 
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBM_CFM:
@@ -1379,12 +1457,16 @@  static int da7219_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
 	/* By default 64 BCLKs per WCLK is supported */
 	dai_clk_mode |= DA7219_DAI_BCLKS_PER_WCLK_64;
 
-	snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
 			    DA7219_DAI_BCLKS_PER_WCLK_MASK |
 			    DA7219_DAI_CLK_POL_MASK | DA7219_DAI_WCLK_POL_MASK,
 			    dai_clk_mode);
-	snd_soc_component_update_bits(component, DA7219_DAI_CTRL, DA7219_DAI_FORMAT_MASK,
-			    dai_ctrl);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
+			    DA7219_DAI_FORMAT_MASK, dai_ctrl);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -1398,15 +1480,22 @@  static int da7219_set_dai_tdm_slot(struct snd_soc_dai *dai,
 	u8 dai_bclks_per_wclk;
 	u16 offset;
 	u32 frame_size;
+	int ret;
 
 	/* No channels enabled so disable TDM, revert to 64-bit frames */
 	if (!tx_mask) {
-		snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
+		ret = snd_soc_component_update_bits(component,
+				    DA7219_DAI_TDM_CTRL,
 				    DA7219_DAI_TDM_CH_EN_MASK |
 				    DA7219_DAI_TDM_MODE_EN_MASK, 0);
-		snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+		if (ret < 0)
+			return ret;
+		ret = snd_soc_component_update_bits(component,
+				    DA7219_DAI_CLK_MODE,
 				    DA7219_DAI_BCLKS_PER_WCLK_MASK,
 				    DA7219_DAI_BCLKS_PER_WCLK_64);
+		if (ret < 0)
+			return ret;
 		return 0;
 	}
 
@@ -1444,19 +1533,25 @@  static int da7219_set_dai_tdm_slot(struct snd_soc_dai *dai,
 		return -EINVAL;
 	}
 
-	snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
 			    DA7219_DAI_BCLKS_PER_WCLK_MASK,
 			    dai_bclks_per_wclk);
+	if (ret < 0)
+		return ret;
 
 	offset = cpu_to_le16(rx_mask);
-	regmap_bulk_write(da7219->regmap, DA7219_DAI_OFFSET_LOWER,
+	ret = regmap_bulk_write(da7219->regmap, DA7219_DAI_OFFSET_LOWER,
 			  &offset, sizeof(offset));
+	if (ret < 0)
+		return ret;
 
-	snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
 			    DA7219_DAI_TDM_CH_EN_MASK |
 			    DA7219_DAI_TDM_MODE_EN_MASK,
 			    (tx_mask << DA7219_DAI_TDM_CH_EN_SHIFT) |
 			    DA7219_DAI_TDM_MODE_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -1468,6 +1563,7 @@  static int da7219_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_component *component = dai->component;
 	u8 dai_ctrl = 0, fs;
 	unsigned int channels;
+	int ret;
 
 	switch (params_width(params)) {
 	case 16:
@@ -1533,11 +1629,15 @@  static int da7219_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-	snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
 			    DA7219_DAI_WORD_LENGTH_MASK |
 			    DA7219_DAI_CH_NUM_MASK,
 			    dai_ctrl);
-	snd_soc_component_write(component, DA7219_SR, fs);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_write(component, DA7219_SR, fs);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -1692,9 +1792,12 @@  static int da7219_set_bias_level(struct snd_soc_component *component,
 	case SND_SOC_BIAS_STANDBY:
 		if (snd_soc_component_get_bias_level(component) == SND_SOC_BIAS_OFF)
 			/* Master bias */
-			snd_soc_component_update_bits(component, DA7219_REFERENCES,
+			ret = snd_soc_component_update_bits(component,
+					    DA7219_REFERENCES,
 					    DA7219_BIAS_EN_MASK,
 					    DA7219_BIAS_EN_MASK);
+			if (ret < 0)
+				return ret;
 
 		if (snd_soc_component_get_bias_level(component) == SND_SOC_BIAS_PREPARE) {
 			/* Remove MCLK */
@@ -1705,8 +1808,11 @@  static int da7219_set_bias_level(struct snd_soc_component *component,
 	case SND_SOC_BIAS_OFF:
 		/* Only disable master bias if we're not a wake-up source */
 		if (!da7219->wakeup_source)
-			snd_soc_component_update_bits(component, DA7219_REFERENCES,
+			ret = snd_soc_component_update_bits(component,
+					    DA7219_REFERENCES,
 					    DA7219_BIAS_EN_MASK, 0);
+			if (ret < 0)
+				return ret;
 
 		break;
 	}
@@ -1754,10 +1860,16 @@  static int da7219_handle_supplies(struct snd_soc_component *component)
 	}
 
 	/* Ensure device in active mode */
-	snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE, DA7219_SYSTEM_ACTIVE_MASK);
+	ret = snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
+				DA7219_SYSTEM_ACTIVE_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Update IO voltage level range */
-	snd_soc_component_write(component, DA7219_IO_CTRL, io_voltage_lvl);
+	ret = snd_soc_component_write(component, DA7219_IO_CTRL,
+				io_voltage_lvl);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -1768,10 +1880,13 @@  static int da7219_dai_clks_prepare(struct clk_hw *hw)
 	struct da7219_priv *da7219 =
 		container_of(hw, struct da7219_priv, dai_clks_hw);
 	struct snd_soc_component *component = da7219->aad->component;
+	int ret;
 
-	snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
 				      DA7219_DAI_CLK_EN_MASK,
 				      DA7219_DAI_CLK_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -1781,9 +1896,12 @@  static void da7219_dai_clks_unprepare(struct clk_hw *hw)
 	struct da7219_priv *da7219 =
 		container_of(hw, struct da7219_priv, dai_clks_hw);
 	struct snd_soc_component *component = da7219->aad->component;
+	int ret;
 
-	snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+	ret = snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
 				      DA7219_DAI_CLK_EN_MASK, 0);
+	if (ret < 0)
+		dev_err(component->dev, "Failed to disable clk: %d\n", ret);
 }
 
 static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
@@ -1791,9 +1909,11 @@  static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
 	struct da7219_priv *da7219 =
 		container_of(hw, struct da7219_priv, dai_clks_hw);
 	struct snd_soc_component *component = da7219->aad->component;
-	u8 clk_reg;
+	int clk_reg;
 
 	clk_reg = snd_soc_component_read32(component, DA7219_DAI_CLK_MODE);
+	if (clk_reg < 0)
+		return clk_reg;
 
 	return !!(clk_reg & DA7219_DAI_CLK_EN_MASK);
 }
@@ -1844,10 +1964,11 @@  static void da7219_register_dai_clks(struct snd_soc_component *component)
 static inline void da7219_register_dai_clks(struct snd_soc_component *component) {}
 #endif /* CONFIG_COMMON_CLK */
 
-static void da7219_handle_pdata(struct snd_soc_component *component)
+static int da7219_handle_pdata(struct snd_soc_component *component)
 {
 	struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
 	struct da7219_pdata *pdata = da7219->pdata;
+	int ret;
 
 	if (pdata) {
 		u8 micbias_lvl = 0;
@@ -1869,7 +1990,10 @@  static void da7219_handle_pdata(struct snd_soc_component *component)
 			break;
 		}
 
-		snd_soc_component_write(component, DA7219_MICBIAS_CTRL, micbias_lvl);
+		ret = snd_soc_component_write(component, DA7219_MICBIAS_CTRL,
+					micbias_lvl);
+		if (ret < 0)
+			return ret;
 
 		/*
 		 * Calculate delay required to compensate for DC offset in
@@ -1884,11 +2008,15 @@  static void da7219_handle_pdata(struct snd_soc_component *component)
 		case DA7219_MIC_AMP_IN_SEL_DIFF:
 		case DA7219_MIC_AMP_IN_SEL_SE_P:
 		case DA7219_MIC_AMP_IN_SEL_SE_N:
-			snd_soc_component_write(component, DA7219_MIC_1_SELECT,
-				      pdata->mic_amp_in_sel);
+			ret = snd_soc_component_write(component,
+					DA7219_MIC_1_SELECT,
+					pdata->mic_amp_in_sel);
+			if (ret < 0)
+				return ret;
 			break;
 		}
 	}
+	return 0;
 }
 
 static struct reg_sequence da7219_rev_aa_patch[] = {
@@ -1934,7 +2062,9 @@  static int da7219_probe(struct snd_soc_component *component)
 	if (!da7219->pdata)
 		da7219->pdata = da7219_fw_to_pdata(component);
 
-	da7219_handle_pdata(component);
+	ret = da7219_handle_pdata(component);
+	if (ret < 0)
+		return ret;
 
 	/* Check if MCLK provided */
 	da7219->mclk = devm_clk_get(component->dev, "mclk");
@@ -1948,36 +2078,57 @@  static int da7219_probe(struct snd_soc_component *component)
 	}
 
 	/* Default PC counter to free-running */
-	snd_soc_component_update_bits(component, DA7219_PC_COUNT, DA7219_PC_FREERUN_MASK,
-			    DA7219_PC_FREERUN_MASK);
+	ret = snd_soc_component_update_bits(component, DA7219_PC_COUNT,
+			DA7219_PC_FREERUN_MASK, DA7219_PC_FREERUN_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Default gain ramping */
-	snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
+	ret = snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
 			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK,
 			    DA7219_MIXIN_L_AMP_RAMP_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL, DA7219_ADC_L_RAMP_EN_MASK,
-			    DA7219_ADC_L_RAMP_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_DAC_L_CTRL, DA7219_DAC_L_RAMP_EN_MASK,
-			    DA7219_DAC_L_RAMP_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_DAC_R_CTRL, DA7219_DAC_R_RAMP_EN_MASK,
-			    DA7219_DAC_R_RAMP_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
+			DA7219_ADC_L_RAMP_EN_MASK, DA7219_ADC_L_RAMP_EN_MASK);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_DAC_L_CTRL,
+			DA7219_DAC_L_RAMP_EN_MASK, DA7219_DAC_L_RAMP_EN_MASK);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_DAC_R_CTRL,
+			DA7219_DAC_R_RAMP_EN_MASK, DA7219_DAC_R_RAMP_EN_MASK);
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
 			    DA7219_HP_L_AMP_RAMP_EN_MASK,
 			    DA7219_HP_L_AMP_RAMP_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
 			    DA7219_HP_R_AMP_RAMP_EN_MASK,
 			    DA7219_HP_R_AMP_RAMP_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Default minimum gain on HP to avoid pops during DAPM sequencing */
-	snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
+	ret = snd_soc_component_update_bits(component, DA7219_HP_L_CTRL,
 			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK,
 			    DA7219_HP_L_AMP_MIN_GAIN_EN_MASK);
-	snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
+	if (ret < 0)
+		return ret;
+	ret = snd_soc_component_update_bits(component, DA7219_HP_R_CTRL,
 			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK,
 			    DA7219_HP_R_AMP_MIN_GAIN_EN_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Default infinite tone gen, start/stop by Kcontrol */
-	snd_soc_component_write(component, DA7219_TONE_GEN_CYCLES, DA7219_BEEP_CYCLES_MASK);
+	ret = snd_soc_component_write(component, DA7219_TONE_GEN_CYCLES,
+			DA7219_BEEP_CYCLES_MASK);
+	if (ret < 0)
+		return ret;
 
 	/* Initialise AAD block */
 	ret = da7219_aad_init(component);
@@ -2221,16 +2372,28 @@  static int da7219_i2c_probe(struct i2c_client *i2c,
 	regcache_cache_bypass(da7219->regmap, true);
 
 	/* Disable audio paths if still active from previous start */
-	regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE, &system_active);
+	ret = regmap_read(da7219->regmap, DA7219_SYSTEM_ACTIVE, &system_active);
+	if (ret < 0)
+		return ret;
 	if (system_active) {
-		regmap_write(da7219->regmap, DA7219_GAIN_RAMP_CTRL,
+		ret = regmap_write(da7219->regmap, DA7219_GAIN_RAMP_CTRL,
 			     DA7219_GAIN_RAMP_RATE_NOMINAL);
-		regmap_write(da7219->regmap, DA7219_SYSTEM_MODES_INPUT, 0x00);
-		regmap_write(da7219->regmap, DA7219_SYSTEM_MODES_OUTPUT, 0x01);
+		if (ret < 0)
+			return ret;
+		ret = regmap_write(da7219->regmap, DA7219_SYSTEM_MODES_INPUT,
+				0x00);
+		if (ret < 0)
+			return ret;
+		ret = regmap_write(da7219->regmap, DA7219_SYSTEM_MODES_OUTPUT,
+				0x01);
+		if (ret < 0)
+			return ret;
 
 		for (i = 0; i < DA7219_SYS_STAT_CHECK_RETRIES; ++i) {
-			regmap_read(da7219->regmap, DA7219_SYSTEM_STATUS,
+			ret = regmap_read(da7219->regmap, DA7219_SYSTEM_STATUS,
 				    &system_status);
+			if (ret < 0)
+				return ret;
 			if (!system_status)
 				break;
 
@@ -2239,13 +2402,19 @@  static int da7219_i2c_probe(struct i2c_client *i2c,
 	}
 
 	/* Soft reset component */
-	regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
+	ret = regmap_write_bits(da7219->regmap, DA7219_ACCDET_CONFIG_1,
 			  DA7219_ACCDET_EN_MASK, 0);
-	regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
+	if (ret < 0)
+		return ret;
+	ret = regmap_write_bits(da7219->regmap, DA7219_CIF_CTRL,
 			  DA7219_CIF_REG_SOFT_RESET_MASK,
 			  DA7219_CIF_REG_SOFT_RESET_MASK);
-	regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
+	if (ret < 0)
+		return ret;
+	ret = regmap_write_bits(da7219->regmap, DA7219_SYSTEM_ACTIVE,
 			  DA7219_SYSTEM_ACTIVE_MASK, 0);
+	if (ret < 0)
+		return ret;
 
 	regcache_cache_bypass(da7219->regmap, false);
 
diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
index 3a00686..0cf78a7 100644
--- a/sound/soc/codecs/da7219.h
+++ b/sound/soc/codecs/da7219.h
@@ -832,7 +832,7 @@  struct da7219_priv {
 	bool alc_en;
 	bool micbias_on_event;
 	unsigned int mic_pga_delay;
-	u8 gain_ramp_ctrl;
+	int gain_ramp_ctrl;
 };
 
 int da7219_set_pll(struct snd_soc_component *component, int source, unsigned int fout);