[PATCH 0/4] ASoC: codecs: lpass-rx-macro: Few code cleanups
Hi,
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 (4): ASoC: codecs: lpass-rx-macro: Simplify with devm allocations 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
include/sound/soc-dapm.h | 2 +- sound/soc/codecs/lpass-rx-macro.c | 37 ++++++++++++++++++++++++------------- sound/soc/soc-dapm.c | 2 +- 3 files changed, 26 insertions(+), 15 deletions(-) --- base-commit: 47ec7f026f5db6a0fe26c6215554d043d04368eb change-id: 20240627-b4-qcom-audio-lpass-codec-cleanups-27175f1d069f
Best regards,
Allocate the default register values array with devm interface to reduce number of error paths. The array is not used after initializing regmap, so move the freeing to place right after devm_regmap_init_mmio() to make it obvious.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/codecs/lpass-rx-macro.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 9c4f0763675d..59fe76b13cdb 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -3817,7 +3817,9 @@ static int rx_macro_probe(struct platform_device *pdev) case LPASS_CODEC_VERSION_2_1: 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); + reg_defaults = devm_kmalloc_array(dev, def_count, + sizeof(struct reg_default), + GFP_KERNEL); if (!reg_defaults) { ret = -ENOMEM; goto err; @@ -3832,7 +3834,9 @@ static int rx_macro_probe(struct platform_device *pdev) case LPASS_CODEC_VERSION_2_8: 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); + reg_defaults = devm_kmalloc_array(dev, def_count, + sizeof(struct reg_default), + GFP_KERNEL); if (!reg_defaults) { ret = -ENOMEM; goto err; @@ -3853,8 +3857,9 @@ static int rx_macro_probe(struct platform_device *pdev) rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config); if (IS_ERR(rx->regmap)) { ret = PTR_ERR(rx->regmap); - goto err_ver; + goto err; } + devm_kfree(dev, reg_defaults);
dev_set_drvdata(dev, rx);
@@ -3866,7 +3871,7 @@ static int rx_macro_probe(struct platform_device *pdev)
ret = clk_prepare_enable(rx->macro); if (ret) - goto err_ver; + goto err;
ret = clk_prepare_enable(rx->dcodec); if (ret) @@ -3912,7 +3917,6 @@ static int rx_macro_probe(struct platform_device *pdev) if (ret) goto err_clkout;
- kfree(reg_defaults); return 0;
err_clkout: @@ -3925,8 +3929,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); err: lpass_macro_pds_exit(rx->pds);
On Thu, Jun 27, 2024 at 05:23:43PM GMT, Krzysztof Kozlowski wrote:
Allocate the default register values array with devm interface to reduce number of error paths. The array is not used after initializing regmap, so move the freeing to place right after devm_regmap_init_mmio() to make it obvious.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/lpass-rx-macro.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
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 devm_kmemdup to allow customizing per detected device variant.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 59fe76b13cdb..3d8149665439 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1662,7 +1662,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 */ @@ -3765,6 +3765,7 @@ 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 regmap_config *reg_config; struct device *dev = &pdev->dev; kernel_ulong_t flags; struct rx_macro *rx; @@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev) goto err; }
- rx_regmap_config.reg_defaults = reg_defaults; - rx_regmap_config.num_reg_defaults = def_count; + reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config), + GFP_KERNEL); + if (!reg_config) { + ret = -ENOMEM; + goto err; + }
- 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)) { ret = PTR_ERR(rx->regmap); goto err; } + devm_kfree(dev, reg_config); devm_kfree(dev, reg_defaults);
dev_set_drvdata(dev, rx);
On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
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 devm_kmemdup to allow customizing per detected device variant.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 59fe76b13cdb..3d8149665439 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1662,7 +1662,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 */ @@ -3765,6 +3765,7 @@ 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 regmap_config *reg_config; struct device *dev = &pdev->dev; kernel_ulong_t flags; struct rx_macro *rx;
@@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev) goto err; }
- rx_regmap_config.reg_defaults = reg_defaults;
- rx_regmap_config.num_reg_defaults = def_count;
- reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
GFP_KERNEL);- if (!reg_config) {
ret = -ENOMEM;goto err;- }
- 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)) { ret = PTR_ERR(rx->regmap); goto err; }
- devm_kfree(dev, reg_config); devm_kfree(dev, reg_defaults);
Seeing devm_kfree in the non-error path makes me feel strange. Maybe it's one of the rare occasions when I can say that __free is suitable here.
dev_set_drvdata(dev, rx);
-- 2.43.0
On 28/06/2024 10:34, Dmitry Baryshkov wrote:
On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
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 devm_kmemdup to allow customizing per detected device variant.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 59fe76b13cdb..3d8149665439 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1662,7 +1662,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 */ @@ -3765,6 +3765,7 @@ 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 regmap_config *reg_config; struct device *dev = &pdev->dev; kernel_ulong_t flags; struct rx_macro *rx;
@@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev) goto err; }
- rx_regmap_config.reg_defaults = reg_defaults;
- rx_regmap_config.num_reg_defaults = def_count;
- reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
GFP_KERNEL);- if (!reg_config) {
ret = -ENOMEM;goto err;- }
- 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)) { ret = PTR_ERR(rx->regmap); goto err; }
- devm_kfree(dev, reg_config); devm_kfree(dev, reg_defaults);
Seeing devm_kfree in the non-error path makes me feel strange. Maybe it's one of the rare occasions when I can say that __free is suitable here.
These would have a bit different meaning in such case. The __free would not clean it in this spot, but on exit from the scope. I wanted to kfree() here, because the config (and reg_defaults) are not used by past regmap_init. I mentioned it briefly in previous patch msg.
To me this code is readable and obvious - past this point nothing uses that allocation. However maybe instead of devm(), the code would be easier to read if non-devm-malloc + __free()?
Best regards, Krzysztof
On Fri, 28 Jun 2024 at 11:39, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
On 28/06/2024 10:34, Dmitry Baryshkov wrote:
On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
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 devm_kmemdup to allow customizing per detected device variant.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index 59fe76b13cdb..3d8149665439 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1662,7 +1662,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 */ @@ -3765,6 +3765,7 @@ 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 regmap_config *reg_config; struct device *dev = &pdev->dev; kernel_ulong_t flags; struct rx_macro *rx;
@@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev) goto err; }
- rx_regmap_config.reg_defaults = reg_defaults;
- rx_regmap_config.num_reg_defaults = def_count;
- reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
GFP_KERNEL);- if (!reg_config) {
ret = -ENOMEM;goto err;- }
- 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)) { ret = PTR_ERR(rx->regmap); goto err; }
- devm_kfree(dev, reg_config); devm_kfree(dev, reg_defaults);
Seeing devm_kfree in the non-error path makes me feel strange. Maybe it's one of the rare occasions when I can say that __free is suitable here.
These would have a bit different meaning in such case. The __free would not clean it in this spot, but on exit from the scope. I wanted to kfree() here, because the config (and reg_defaults) are not used by past regmap_init. I mentioned it briefly in previous patch msg.
To me this code is readable and obvious - past this point nothing uses that allocation. However maybe instead of devm(), the code would be easier to read if non-devm-malloc + __free()?
Yes, that's what I was thinking about. But it's definitely an optional topic. Your code is correct.
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.
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;
On Thu, Jun 27, 2024 at 05:23:45PM GMT, Krzysztof Kozlowski wrote:
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.
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(-)
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
Driver uses ARRAY_SIZE() to get number of widgets later passed to snd_soc_dapm_new_controls(), which is an 'unsigned int'.
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 3d8149665439..bfcbfbe8b086 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -3611,8 +3611,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);
On Thu, Jun 27, 2024 at 05:23:46PM GMT, Krzysztof Kozlowski wrote:
Driver uses ARRAY_SIZE() to get number of widgets later passed to snd_soc_dapm_new_controls(), which is an 'unsigned int'.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/lpass-rx-macro.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
participants (2)
-
Dmitry Baryshkov -
Krzysztof Kozlowski