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@xxxxxxxxxx>
> ---
>
> Personally I found this patch very useful while developing and debugging.
>
> Ex: below error
> "ASoC: error at soc_component_read_no_lock on soc@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@0:codec for register: [0x00003125] -16"
> which was easy to locate and fix.
LGTM
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
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);
>
