[PATCH v3 0/6] ASoC: codecs: lpass-rx-macro: Few code cleanups
Hi,
Changes in v3: - New patch #1 to fix clang jump warning ("ASoC: codecs: lpass-rx-macro: Simplify PDS cleanup with devm") - Link to v2: https://lore.kernel.org/r/20240628-b4-qcom-audio-lpass-codec-cleanups-v2-0-e...
Changes in v2: - Use cleanup.h instead of devm(), therefore not adding Dmitry's review. - New patch #5. - Link to v1: https://lore.kernel.org/r/20240627-b4-qcom-audio-lpass-codec-cleanups-v1-0-e...
Improve a bit the Qualcomm LPASS RX macro driver and align similar parts of code with LPASS WSA macro driver for consistency.
No external dependencies.
Best regards, Krzysztof
--- Krzysztof Kozlowski (6): ASoC: codecs: lpass-rx-macro: Simplify PDS cleanup with devm ASoC: codecs: lpass-rx-macro: Simplify with cleanup.h ASoC: codecs: lpass-rx-macro: Keep static regmap_config as const ASoC: dapm: Use unsigned for number of widgets in snd_soc_dapm_new_controls() ASoC: codecs: lpass-rx-macro: Use unsigned for number of widgets ASoC: codecs: lpass-wsa-macro: Simplify with cleanup.h
include/sound/soc-dapm.h | 2 +- sound/soc/codecs/lpass-macro-common.h | 5 +++ sound/soc/codecs/lpass-rx-macro.c | 63 +++++++++++++++++------------------ sound/soc/codecs/lpass-wsa-macro.c | 22 ++++++------ sound/soc/soc-dapm.c | 2 +- 5 files changed, 47 insertions(+), 47 deletions(-) --- base-commit: feca1ff0cd5ab7bc3990ec5a387d81d4dff88068 change-id: 20240627-b4-qcom-audio-lpass-codec-cleanups-27175f1d069f
Best regards,
Eliminate PDS cleanup by using devm_add_action_or_reset() which results in one less error path and smaller cleanup in remove().
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
---
Changes in v3: 1. New patch --- sound/soc/codecs/lpass-macro-common.h | 5 +++++ sound/soc/codecs/lpass-rx-macro.c | 30 ++++++++++++------------------ 2 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/sound/soc/codecs/lpass-macro-common.h b/sound/soc/codecs/lpass-macro-common.h index 3aa9737f2737..21cb30ab706d 100644 --- a/sound/soc/codecs/lpass-macro-common.h +++ b/sound/soc/codecs/lpass-macro-common.h @@ -41,6 +41,11 @@ void lpass_macro_pds_exit(struct lpass_macro *pds); void lpass_macro_set_codec_version(enum lpass_codec_version version); enum lpass_codec_version lpass_macro_get_codec_version(void);
+static inline void lpass_macro_pds_exit_action(void *pds) +{ + lpass_macro_pds_exit(pds); +} + static inline const char *lpass_macro_get_codec_version_string(int version) { switch (version) { diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index d47c49c90de3..77e734ad1885 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -3803,11 +3803,14 @@ static int rx_macro_probe(struct platform_device *pdev) if (IS_ERR(rx->pds)) return PTR_ERR(rx->pds);
+ ret = devm_add_action_or_reset(dev, lpass_macro_pds_exit_action, rx->pds); + if (ret) + return ret; + base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(base)) { - ret = PTR_ERR(base); - goto err; - } + if (IS_ERR(base)) + return PTR_ERR(base); + rx->codec_version = lpass_macro_get_codec_version(); switch (rx->codec_version) { case LPASS_CODEC_VERSION_1_0: @@ -3818,10 +3821,8 @@ static int rx_macro_probe(struct platform_device *pdev) rx->rxn_reg_stride = 0x80; def_count = ARRAY_SIZE(rx_defaults) + ARRAY_SIZE(rx_pre_2_5_defaults); reg_defaults = kmalloc_array(def_count, sizeof(struct reg_default), GFP_KERNEL); - if (!reg_defaults) { - ret = -ENOMEM; - goto err; - } + if (!reg_defaults) + return -ENOMEM; memcpy(®_defaults[0], rx_defaults, sizeof(rx_defaults)); memcpy(®_defaults[ARRAY_SIZE(rx_defaults)], rx_pre_2_5_defaults, sizeof(rx_pre_2_5_defaults)); @@ -3833,18 +3834,15 @@ static int rx_macro_probe(struct platform_device *pdev) rx->rxn_reg_stride = 0xc0; def_count = ARRAY_SIZE(rx_defaults) + ARRAY_SIZE(rx_2_5_defaults); reg_defaults = kmalloc_array(def_count, sizeof(struct reg_default), GFP_KERNEL); - if (!reg_defaults) { - ret = -ENOMEM; - goto err; - } + if (!reg_defaults) + return -ENOMEM; memcpy(®_defaults[0], rx_defaults, sizeof(rx_defaults)); memcpy(®_defaults[ARRAY_SIZE(rx_defaults)], rx_2_5_defaults, sizeof(rx_2_5_defaults)); break; default: dev_err(dev, "Unsupported Codec version (%d)\n", rx->codec_version); - ret = -EINVAL; - goto err; + return -EINVAL; }
rx_regmap_config.reg_defaults = reg_defaults; @@ -3927,8 +3925,6 @@ static int rx_macro_probe(struct platform_device *pdev) clk_disable_unprepare(rx->macro); err_ver: kfree(reg_defaults); -err: - lpass_macro_pds_exit(rx->pds);
return ret; } @@ -3942,8 +3938,6 @@ static void rx_macro_remove(struct platform_device *pdev) clk_disable_unprepare(rx->fsgen); clk_disable_unprepare(rx->macro); clk_disable_unprepare(rx->dcodec); - - lpass_macro_pds_exit(rx->pds); }
static const struct of_device_id rx_macro_dt_match[] = {
Allocate the default register values array with scoped/cleanup.h to reduce number of error paths and make code a bit simpler.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
---
Not adding Dmitry's Rb tag, because of major change devm->cleanup.h. --- sound/soc/codecs/lpass-rx-macro.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 77e734ad1885..638f3995a364 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
+#include <linux/cleanup.h> #include <linux/module.h> #include <linux/init.h> #include <linux/io.h> @@ -3764,7 +3765,6 @@ static const struct snd_soc_component_driver rx_macro_component_drv = {
static int rx_macro_probe(struct platform_device *pdev) { - struct reg_default *reg_defaults; struct device *dev = &pdev->dev; kernel_ulong_t flags; struct rx_macro *rx; @@ -3812,6 +3812,8 @@ static int rx_macro_probe(struct platform_device *pdev) return PTR_ERR(base);
rx->codec_version = lpass_macro_get_codec_version(); + struct reg_default *reg_defaults __free(kfree) = NULL; + switch (rx->codec_version) { case LPASS_CODEC_VERSION_1_0: case LPASS_CODEC_VERSION_1_1: @@ -3849,10 +3851,8 @@ static int rx_macro_probe(struct platform_device *pdev) rx_regmap_config.num_reg_defaults = def_count;
rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config); - if (IS_ERR(rx->regmap)) { - ret = PTR_ERR(rx->regmap); - goto err_ver; - } + if (IS_ERR(rx->regmap)) + return PTR_ERR(rx->regmap);
dev_set_drvdata(dev, rx);
@@ -3864,7 +3864,7 @@ static int rx_macro_probe(struct platform_device *pdev)
ret = clk_prepare_enable(rx->macro); if (ret) - goto err_ver; + return ret;
ret = clk_prepare_enable(rx->dcodec); if (ret) @@ -3910,7 +3910,6 @@ static int rx_macro_probe(struct platform_device *pdev) if (ret) goto err_clkout;
- kfree(reg_defaults); return 0;
err_clkout: @@ -3923,8 +3922,6 @@ static int rx_macro_probe(struct platform_device *pdev) clk_disable_unprepare(rx->dcodec); err_dcodec: clk_disable_unprepare(rx->macro); -err_ver: - kfree(reg_defaults);
return ret; }
The driver has static 'struct regmap_config', which is then customized depending on device version. This works fine, because there should not be two devices in a system simultaneously and even less likely that such two devices would have different versions, thus different regmap config. However code is cleaner and more obvious when static data in the driver is also const - it serves as a template.
Mark the 'struct regmap_config' as const and duplicate it in the probe() with kmemdup to allow customizing per detected device variant.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org ---
Not adding Dmitry's Rb tag, because of major change devm->cleanup.h. --- sound/soc/codecs/lpass-rx-macro.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 638f3995a364..d4d7e02db83f 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1663,7 +1663,7 @@ static bool rx_is_readable_register(struct device *dev, unsigned int reg) return rx_is_rw_register(dev, reg); }
-static struct regmap_config rx_regmap_config = { +static const struct regmap_config rx_regmap_config = { .name = "rx_macro", .reg_bits = 16, .val_bits = 32, /* 8 but with 32 bit read/write */ @@ -3847,10 +3847,16 @@ static int rx_macro_probe(struct platform_device *pdev) return -EINVAL; }
- rx_regmap_config.reg_defaults = reg_defaults; - rx_regmap_config.num_reg_defaults = def_count; + struct regmap_config *reg_config __free(kfree) = kmemdup(&rx_regmap_config, + sizeof(*reg_config), + GFP_KERNEL); + if (!reg_config) + return -ENOMEM;
- rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config); + reg_config->reg_defaults = reg_defaults; + reg_config->num_reg_defaults = def_count; + + rx->regmap = devm_regmap_init_mmio(dev, base, reg_config); if (IS_ERR(rx->regmap)) return PTR_ERR(rx->regmap);
Number of widgets in array passed to snd_soc_dapm_new_controls() cannot be negative, so make it explicit by using 'unsigned int', just like snd_soc_add_component_controls() is doing.
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- include/sound/soc-dapm.h | 2 +- sound/soc/soc-dapm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 667ecd4daa68..12cd7b5a2202 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -457,7 +457,7 @@ int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol, int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *uncontrol); int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, - const struct snd_soc_dapm_widget *widget, int num); + const struct snd_soc_dapm_widget *widget, unsigned int num); struct snd_soc_dapm_widget *snd_soc_dapm_new_control(struct snd_soc_dapm_context *dapm, const struct snd_soc_dapm_widget *widget); struct snd_soc_dapm_widget *snd_soc_dapm_new_control_unlocked(struct snd_soc_dapm_context *dapm, diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 16dad4a45443..32cc90d09bc2 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3857,7 +3857,7 @@ EXPORT_SYMBOL_GPL(snd_soc_dapm_new_control); */ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, const struct snd_soc_dapm_widget *widget, - int num) + unsigned int num) { int i; int ret = 0;
Driver uses ARRAY_SIZE() to get number of widgets later passed to snd_soc_dapm_new_controls(), which is an 'unsigned int'.
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/codecs/lpass-rx-macro.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index d4d7e02db83f..ce42749660c8 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -3612,8 +3612,8 @@ static int rx_macro_component_probe(struct snd_soc_component *component) struct rx_macro *rx = snd_soc_component_get_drvdata(component); const struct snd_soc_dapm_widget *widgets; const struct snd_kcontrol_new *controls; - unsigned int num_controls; - int ret, num_widgets; + unsigned int num_controls, num_widgets; + int ret;
snd_soc_component_init_regmap(component, rx->regmap);
Driver's probe() has two allocations which are needed only within the probe() itself - for devm_regmap_init_mmio().
Usage of devm interface is a bit misleading here, because these can be freed right after each scope finishes.
This makes the code a bit more obvious and self documenting.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
---
Changes in v2: 1. New patch --- sound/soc/codecs/lpass-wsa-macro.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index b4e7139bac61..73a588289408 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
+#include <linux/cleanup.h> #include <linux/module.h> #include <linux/init.h> #include <linux/io.h> @@ -2725,8 +2726,6 @@ static const struct snd_soc_component_driver wsa_macro_component_drv = { static int wsa_macro_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct reg_default *reg_defaults; - struct regmap_config *reg_config; struct wsa_macro *wsa; kernel_ulong_t flags; void __iomem *base; @@ -2765,6 +2764,8 @@ static int wsa_macro_probe(struct platform_device *pdev) return PTR_ERR(base);
wsa->codec_version = lpass_macro_get_codec_version(); + struct reg_default *reg_defaults __free(kfree) = NULL; + switch (wsa->codec_version) { case LPASS_CODEC_VERSION_1_0: case LPASS_CODEC_VERSION_1_1: @@ -2773,9 +2774,8 @@ static int wsa_macro_probe(struct platform_device *pdev) case LPASS_CODEC_VERSION_2_1: wsa->reg_layout = &wsa_codec_v2_1; def_count = ARRAY_SIZE(wsa_defaults) + ARRAY_SIZE(wsa_defaults_v2_1); - reg_defaults = devm_kmalloc_array(dev, def_count, - sizeof(*reg_defaults), - GFP_KERNEL); + reg_defaults = kmalloc_array(def_count, sizeof(*reg_defaults), + GFP_KERNEL); if (!reg_defaults) return -ENOMEM; memcpy(®_defaults[0], wsa_defaults, sizeof(wsa_defaults)); @@ -2789,9 +2789,8 @@ static int wsa_macro_probe(struct platform_device *pdev) case LPASS_CODEC_VERSION_2_8: wsa->reg_layout = &wsa_codec_v2_5; def_count = ARRAY_SIZE(wsa_defaults) + ARRAY_SIZE(wsa_defaults_v2_5); - reg_defaults = devm_kmalloc_array(dev, def_count, - sizeof(*reg_defaults), - GFP_KERNEL); + reg_defaults = kmalloc_array(def_count, sizeof(*reg_defaults), + GFP_KERNEL); if (!reg_defaults) return -ENOMEM; memcpy(®_defaults[0], wsa_defaults, sizeof(wsa_defaults)); @@ -2804,8 +2803,9 @@ static int wsa_macro_probe(struct platform_device *pdev) return -EINVAL; }
- reg_config = devm_kmemdup(dev, &wsa_regmap_config, - sizeof(*reg_config), GFP_KERNEL); + struct regmap_config *reg_config __free(kfree) = kmemdup(&wsa_regmap_config, + sizeof(*reg_config), + GFP_KERNEL); if (!reg_config) return -ENOMEM;
@@ -2816,8 +2816,6 @@ static int wsa_macro_probe(struct platform_device *pdev) if (IS_ERR(wsa->regmap)) return PTR_ERR(wsa->regmap);
- devm_kfree(dev, reg_config); - devm_kfree(dev, reg_defaults); dev_set_drvdata(dev, wsa);
wsa->dev = dev;
On Mon, 01 Jul 2024 09:39:32 +0200, Krzysztof Kozlowski wrote:
Changes in v3:
- New patch #1 to fix clang jump warning ("ASoC: codecs: lpass-rx-macro: Simplify PDS cleanup with devm")
- Link to v2: https://lore.kernel.org/r/20240628-b4-qcom-audio-lpass-codec-cleanups-v2-0-e...
Changes in v2:
- Use cleanup.h instead of devm(), therefore not adding Dmitry's review.
- New patch #5.
- Link to v1: https://lore.kernel.org/r/20240627-b4-qcom-audio-lpass-codec-cleanups-v1-0-e...
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: codecs: lpass-rx-macro: Simplify PDS cleanup with devm commit: 891168dc4a6c637ca76c64e7bde6917b96b9cd54 [2/6] ASoC: codecs: lpass-rx-macro: Simplify with cleanup.h commit: ee5e13b2c92324938c2bffc44b36b5a29fc28087 [3/6] ASoC: codecs: lpass-rx-macro: Keep static regmap_config as const commit: 0c02cacf62fd90bf9f0c6c33e9a4862cfc50aab4 [4/6] ASoC: dapm: Use unsigned for number of widgets in snd_soc_dapm_new_controls() commit: bf95919fe1917efa8f5da83057ff9fc11130aa55 [5/6] ASoC: codecs: lpass-rx-macro: Use unsigned for number of widgets commit: c72585d79249fb07ca3e3c91022e312d21f20f40 [6/6] ASoC: codecs: lpass-wsa-macro: Simplify with cleanup.h commit: 67820eb9f4895791da46df42ff7942dfc1353bb2
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 (2)
-
Krzysztof Kozlowski -
Mark Brown