[PATCH] ASoC: soc-component: improve error reporting for register access

[Pierre-Louis Bossart]

On 10/14/21 11:13 AM, Srinivas Kandagatla wrote:
> Currently errors on register read/write/update are reported with
> an error code and the corresponding function but does not provide
> any details on the which register number did it actually fail.
> 
> register number can give better clue and it should be easy to
> locate the code and fix.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
> ---
> 
> Personally I found this patch very useful while developing and debugging.
> 
> Ex: below error 
> "ASoC: error at soc_component_read_no_lock on soc at 0:codec: -16"
> did not provide much info except that it failed to read,
> but after this patch error message looks like:
> "ASoC: error at soc_component_read_no_lock on soc at 0:codec for register: [0x00003125] -16"
> which was easy to locate and fix.

LGTM

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>

BTW while looking at the code, I started to wonder if it's intentional
that we cannot check any error code on component->driver->read(). We do
for the write and on regmap read, which suggests this API is problematic?

static unsigned int soc_component_read_no_lock(
	struct snd_soc_component *component,
	unsigned int reg)
{
	int ret;
	unsigned int val = 0;

	if (component->regmap)
		ret = regmap_read(component->regmap, reg, &val);
	else if (component->driver->read) {
		ret = 0;
		val = component->driver->read(component, reg); <<< NO ERROR CHECKS?
	}
	else
		ret = -EIO;

	if (ret < 0)
		return soc_component_ret(component, ret);

	return val;
}


> 
>  sound/soc/soc-component.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
> index a08a897c5230..c76ff9c59dfb 100644
> --- a/sound/soc/soc-component.c
> +++ b/sound/soc/soc-component.c
> @@ -13,9 +13,10 @@
>  #include <sound/soc.h>
>  #include <linux/bitops.h>
>  
> -#define soc_component_ret(dai, ret) _soc_component_ret(dai, __func__, ret)
> +#define soc_component_ret(dai, ret) _soc_component_ret(dai, __func__, ret, -1)
> +#define soc_component_ret_reg_rw(dai, ret, reg) _soc_component_ret(dai, __func__, ret, reg)
>  static inline int _soc_component_ret(struct snd_soc_component *component,
> -				     const char *func, int ret)
> +				     const char *func, int ret, int reg)
>  {
>  	/* Positive/Zero values are not errors */
>  	if (ret >= 0)
> @@ -27,9 +28,14 @@ static inline int _soc_component_ret(struct snd_soc_component *component,
>  	case -ENOTSUPP:
>  		break;
>  	default:
> -		dev_err(component->dev,
> -			"ASoC: error at %s on %s: %d\n",
> -			func, component->name, ret);
> +		if (reg == -1)
> +			dev_err(component->dev,
> +				"ASoC: error at %s on %s: %d\n",
> +				func, component->name, ret);
> +		else
> +			dev_err(component->dev,
> +				"ASoC: error at %s on %s for register: [0x%08x] %d\n",
> +				func, component->name, reg, ret);
>  	}
>  
>  	return ret;
> @@ -687,7 +693,7 @@ static unsigned int soc_component_read_no_lock(
>  		ret = -EIO;
>  
>  	if (ret < 0)
> -		return soc_component_ret(component, ret);
> +		return soc_component_ret_reg_rw(component, ret, reg);
>  
>  	return val;
>  }
> @@ -723,7 +729,7 @@ static int soc_component_write_no_lock(
>  	else if (component->driver->write)
>  		ret = component->driver->write(component, reg, val);
>  
> -	return soc_component_ret(component, ret);
> +	return soc_component_ret_reg_rw(component, ret, reg);
>  }
>  
>  /**
> @@ -765,7 +771,7 @@ static int snd_soc_component_update_bits_legacy(
>  
>  	mutex_unlock(&component->io_mutex);
>  
> -	return soc_component_ret(component, ret);
> +	return soc_component_ret_reg_rw(component, ret, reg);
>  }
>  
>  /**
> @@ -793,7 +799,7 @@ int snd_soc_component_update_bits(struct snd_soc_component *component,
>  							   mask, val, &change);
>  
>  	if (ret < 0)
> -		return soc_component_ret(component, ret);
> +		return soc_component_ret_reg_rw(component, ret, reg);
>  	return change;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_component_update_bits);
> @@ -829,7 +835,7 @@ int snd_soc_component_update_bits_async(struct snd_soc_component *component,
>  							   mask, val, &change);
>  
>  	if (ret < 0)
> -		return soc_component_ret(component, ret);
> +		return soc_component_ret_reg_rw(component, ret, reg);
>  	return change;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_component_update_bits_async);
>