[PATCH] ASoC: soc-component: improve error reporting for register access
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@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@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.
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);
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@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@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@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);elsedev_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);} 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);} EXPORT_SYMBOL_GPL(snd_soc_component_update_bits_async);
On Thu, Oct 14, 2021 at 11:31:24AM -0500, Pierre-Louis Bossart wrote:
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?
I dunno about intentional but it's always been that way since ASoC was originally merged, it's just that nobody's ever changed that as part of one of the refactorings. I did add error checking to the regmap read interface when I wrote that but I wasn't about to go fixing up all the ASoC drivers for a new API.
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?
I dunno about intentional but it's always been that way since ASoC was originally merged, it's just that nobody's ever changed that as part of one of the refactorings. I did add error checking to the regmap read interface when I wrote that but I wasn't about to go fixing up all the ASoC drivers for a new API.
I started looking, there aren't that many users of that callback, but some of the tlv320dac33 and twl6040 drivers would need a lot of changes. that's probably going to be legacy code at this point.
On Thu, 14 Oct 2021 17:13:30 +0100, 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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: soc-component: improve error reporting for register access commit: b296997cf539976c62f81cdd367924809fdcc14e
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Srinivas Kandagatla