[PATCH 00/11] ASoC: topology: Fixes and cleanups
Following is series of fixes and cleanups for core topology code. Few patches fixing various problems all around and few fixing function names. Amadeusz Sławiński (11): ASoC: topology: Properly access value coming from topology file ASoC: topology: Remove unused SOC_TPLG_PASS_PINS constant ASoC: topology: Fix typo in functions name ASoC: topology: Fix function name ASoC: topology: Rename remove_ handlers ASoC: topology: Remove unnecessary forward declarations ASoC: topology: Pass correct pointer instead of casting ASoC: topology: Return an error on complete() failure ASoC: Topology: Remove unnecessary check for EOF ASoC: topology: Use unload() op directly ASoC: topology: Unify kcontrol removal code include/sound/soc-topology.h | 2 +- sound/soc/soc-topology.c | 183 ++++++++++++++--------------------- 2 files changed, 74 insertions(+), 111 deletions(-) -- 2.25.1
When accessing values coming from topology, le32_to_cpu should be used. One of recent commits missed that. Fixes: 86e2d14b6d1a ("ASoC: topology: Add header payload_size verification") Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index c3be24b2fac5..b9addbab2b3d 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2404,7 +2404,7 @@ static int soc_valid_header(struct soc_tplg *tplg, return -EINVAL; } - if (soc_tplg_get_hdr_offset(tplg) + hdr->payload_size >= tplg->fw->size) { + if (soc_tplg_get_hdr_offset(tplg) + le32_to_cpu(hdr->payload_size) >= tplg->fw->size) { dev_err(tplg->dev, "ASoC: invalid header of type %d at offset %ld payload_size %d\n", le32_to_cpu(hdr->type), soc_tplg_get_hdr_offset(tplg), -- 2.25.1
The constant is unused, so it can be safely removed. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index b9addbab2b3d..e9138ec4df8f 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -44,9 +44,8 @@ #define SOC_TPLG_PASS_WIDGET 3 #define SOC_TPLG_PASS_PCM_DAI 4 #define SOC_TPLG_PASS_GRAPH 5 -#define SOC_TPLG_PASS_PINS 6 -#define SOC_TPLG_PASS_BE_DAI 7 -#define SOC_TPLG_PASS_LINK 8 +#define SOC_TPLG_PASS_BE_DAI 6 +#define SOC_TPLG_PASS_LINK 7 #define SOC_TPLG_PASS_START SOC_TPLG_PASS_MANIFEST #define SOC_TPLG_PASS_END SOC_TPLG_PASS_LINK -- 2.25.1
Topology is being abbreviated to "tplg", not "tplc", however, few functions have typo in name, fix it. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index e9138ec4df8f..b9c29effeb60 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -185,7 +185,7 @@ static const struct soc_tplg_map dapm_map[] = { {SND_SOC_TPLG_DAPM_DECODER, snd_soc_dapm_decoder}, }; -static int tplc_chan_get_reg(struct soc_tplg *tplg, +static int tplg_chan_get_reg(struct soc_tplg *tplg, struct snd_soc_tplg_channel *chan, int map) { int i; @@ -198,7 +198,7 @@ static int tplc_chan_get_reg(struct soc_tplg *tplg, return -EINVAL; } -static int tplc_chan_get_shift(struct soc_tplg *tplg, +static int tplg_chan_get_shift(struct soc_tplg *tplg, struct snd_soc_tplg_channel *chan, int map) { int i; @@ -779,10 +779,10 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, size_t size) kc.access = le32_to_cpu(mc->hdr.access); /* we only support FL/FR channel mapping atm */ - sm->reg = tplc_chan_get_reg(tplg, mc->channel, SNDRV_CHMAP_FL); - sm->rreg = tplc_chan_get_reg(tplg, mc->channel, SNDRV_CHMAP_FR); - sm->shift = tplc_chan_get_shift(tplg, mc->channel, SNDRV_CHMAP_FL); - sm->rshift = tplc_chan_get_shift(tplg, mc->channel, SNDRV_CHMAP_FR); + sm->reg = tplg_chan_get_reg(tplg, mc->channel, SNDRV_CHMAP_FL); + sm->rreg = tplg_chan_get_reg(tplg, mc->channel, SNDRV_CHMAP_FR); + sm->shift = tplg_chan_get_shift(tplg, mc->channel, SNDRV_CHMAP_FL); + sm->rshift = tplg_chan_get_shift(tplg, mc->channel, SNDRV_CHMAP_FR); sm->max = le32_to_cpu(mc->max); sm->min = le32_to_cpu(mc->min); @@ -926,10 +926,10 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, size_t size) kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER; kc.access = le32_to_cpu(ec->hdr.access); - se->reg = tplc_chan_get_reg(tplg, ec->channel, SNDRV_CHMAP_FL); - se->shift_l = tplc_chan_get_shift(tplg, ec->channel, + se->reg = tplg_chan_get_reg(tplg, ec->channel, SNDRV_CHMAP_FL); + se->shift_l = tplg_chan_get_shift(tplg, ec->channel, SNDRV_CHMAP_FL); - se->shift_r = tplc_chan_get_shift(tplg, ec->channel, + se->shift_r = tplg_chan_get_shift(tplg, ec->channel, SNDRV_CHMAP_FL); se->mask = le32_to_cpu(ec->mask); @@ -1160,13 +1160,13 @@ static int soc_tplg_dapm_widget_dmixer_create(struct soc_tplg *tplg, struct snd_ kc->access = le32_to_cpu(mc->hdr.access); /* we only support FL/FR channel mapping atm */ - sm->reg = tplc_chan_get_reg(tplg, mc->channel, + sm->reg = tplg_chan_get_reg(tplg, mc->channel, SNDRV_CHMAP_FL); - sm->rreg = tplc_chan_get_reg(tplg, mc->channel, + sm->rreg = tplg_chan_get_reg(tplg, mc->channel, SNDRV_CHMAP_FR); - sm->shift = tplc_chan_get_shift(tplg, mc->channel, + sm->shift = tplg_chan_get_shift(tplg, mc->channel, SNDRV_CHMAP_FL); - sm->rshift = tplc_chan_get_shift(tplg, mc->channel, + sm->rshift = tplg_chan_get_shift(tplg, mc->channel, SNDRV_CHMAP_FR); sm->max = le32_to_cpu(mc->max); @@ -1232,10 +1232,10 @@ static int soc_tplg_dapm_widget_denum_create(struct soc_tplg *tplg, struct snd_k kc->access = le32_to_cpu(ec->hdr.access); /* we only support FL/FR channel mapping atm */ - se->reg = tplc_chan_get_reg(tplg, ec->channel, SNDRV_CHMAP_FL); - se->shift_l = tplc_chan_get_shift(tplg, ec->channel, + se->reg = tplg_chan_get_reg(tplg, ec->channel, SNDRV_CHMAP_FL); + se->shift_l = tplg_chan_get_shift(tplg, ec->channel, SNDRV_CHMAP_FL); - se->shift_r = tplc_chan_get_shift(tplg, ec->channel, + se->shift_r = tplg_chan_get_shift(tplg, ec->channel, SNDRV_CHMAP_FR); se->items = le32_to_cpu(ec->items); -- 2.25.1
Functions other than soc_valid_header have soc_tplg_ prefix. Rename function to follow convention in file. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index b9c29effeb60..1fe4fa82fa2f 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2389,7 +2389,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, } /* validate header magic, size and type */ -static int soc_valid_header(struct soc_tplg *tplg, +static int soc_tplg_valid_header(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { if (soc_tplg_get_hdr_offset(tplg) >= tplg->fw->size) @@ -2526,7 +2526,7 @@ static int soc_tplg_process_headers(struct soc_tplg *tplg) while (!soc_tplg_is_eof(tplg)) { /* make sure header is valid before loading */ - ret = soc_valid_header(tplg, hdr); + ret = soc_tplg_valid_header(tplg, hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: topology: invalid header: %d\n", ret); -- 2.25.1
Those are the only functions missing soc_tplg_ prefix, add it for consistency. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 1fe4fa82fa2f..a952f7eaa646 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -354,7 +354,7 @@ static int soc_tplg_add_kcontrol(struct soc_tplg *tplg, } /* remove a mixer kcontrol */ -static void remove_mixer(struct snd_soc_component *comp, +static void soc_tplg_remove_mixer(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { struct snd_card *card = comp->card->snd_card; @@ -370,7 +370,7 @@ static void remove_mixer(struct snd_soc_component *comp, } /* remove an enum kcontrol */ -static void remove_enum(struct snd_soc_component *comp, +static void soc_tplg_remove_enum(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { struct snd_card *card = comp->card->snd_card; @@ -386,7 +386,7 @@ static void remove_enum(struct snd_soc_component *comp, } /* remove a byte kcontrol */ -static void remove_bytes(struct snd_soc_component *comp, +static void soc_tplg_remove_bytes(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { struct snd_card *card = comp->card->snd_card; @@ -402,7 +402,7 @@ static void remove_bytes(struct snd_soc_component *comp, } /* remove a route */ -static void remove_route(struct snd_soc_component *comp, +static void soc_tplg_remove_route(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { if (pass != SOC_TPLG_PASS_GRAPH) @@ -415,7 +415,7 @@ static void remove_route(struct snd_soc_component *comp, } /* remove a widget and it's kcontrols - routes must be removed first */ -static void remove_widget(struct snd_soc_component *comp, +static void soc_tplg_remove_widget(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { struct snd_card *card = comp->card->snd_card; @@ -443,7 +443,7 @@ static void remove_widget(struct snd_soc_component *comp, } /* remove DAI configurations */ -static void remove_dai(struct snd_soc_component *comp, +static void soc_tplg_remove_dai(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { struct snd_soc_dai_driver *dai_drv = @@ -464,7 +464,7 @@ static void remove_dai(struct snd_soc_component *comp, } /* remove link configurations */ -static void remove_link(struct snd_soc_component *comp, +static void soc_tplg_remove_link(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, int pass) { struct snd_soc_dai_link *link = @@ -492,7 +492,7 @@ static void remove_backend_link(struct snd_soc_component *comp, dobj->ops->link_unload(comp, dobj); /* - * We don't free the link here as what remove_link() do since BE + * We don't free the link here as what soc_tplg_remove_link() do since BE * links are not allocated by topology. * We however need to reset the dobj type to its initial values */ @@ -1492,7 +1492,7 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg, return 0; ready_err: - remove_widget(widget->dapm->component, &widget->dobj, SOC_TPLG_PASS_WIDGET); + soc_tplg_remove_widget(widget->dapm->component, &widget->dobj, SOC_TPLG_PASS_WIDGET); snd_soc_dapm_free_widget(widget); hdr_err: kfree(template.sname); @@ -2627,25 +2627,25 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp) switch (dobj->type) { case SND_SOC_DOBJ_MIXER: - remove_mixer(comp, dobj, pass); + soc_tplg_remove_mixer(comp, dobj, pass); break; case SND_SOC_DOBJ_ENUM: - remove_enum(comp, dobj, pass); + soc_tplg_remove_enum(comp, dobj, pass); break; case SND_SOC_DOBJ_BYTES: - remove_bytes(comp, dobj, pass); + soc_tplg_remove_bytes(comp, dobj, pass); break; case SND_SOC_DOBJ_GRAPH: - remove_route(comp, dobj, pass); + soc_tplg_remove_route(comp, dobj, pass); break; case SND_SOC_DOBJ_WIDGET: - remove_widget(comp, dobj, pass); + soc_tplg_remove_widget(comp, dobj, pass); break; case SND_SOC_DOBJ_PCM: - remove_dai(comp, dobj, pass); + soc_tplg_remove_dai(comp, dobj, pass); break; case SND_SOC_DOBJ_DAI_LINK: - remove_link(comp, dobj, pass); + soc_tplg_remove_link(comp, dobj, pass); break; case SND_SOC_DOBJ_BACKEND_LINK: /* -- 2.25.1
There is no need to forward declare functions if their use is after their definition. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index a952f7eaa646..766de161aa4a 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -76,9 +76,6 @@ struct soc_tplg { struct snd_soc_tplg_ops *ops; }; -static int soc_tplg_process_headers(struct soc_tplg *tplg); -static int soc_tplg_complete(struct soc_tplg *tplg); - /* check we dont overflow the data for this control chunk */ static int soc_tplg_check_elem_count(struct soc_tplg *tplg, size_t elem_size, unsigned int count, size_t bytes, const char *elem_type) -- 2.25.1
Instead of passing address of structure, the containg structure is casted to target structure. While it works - the expected structure is the first field of containing one - it is bad practice, fix this by passing pointer to structure field. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 766de161aa4a..c35c0c53a184 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -721,7 +721,7 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, size_t size) } /* pass control to driver for optional further init */ - ret = soc_tplg_control_load(tplg, &kc, (struct snd_soc_tplg_ctl_hdr *)be); + ret = soc_tplg_control_load(tplg, &kc, &be->hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", be->hdr.name); goto err; @@ -805,7 +805,7 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, size_t size) } /* pass control to driver for optional further init */ - ret = soc_tplg_control_load(tplg, &kc, (struct snd_soc_tplg_ctl_hdr *)mc); + ret = soc_tplg_control_load(tplg, &kc, &mc->hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", mc->hdr.name); goto err; @@ -973,7 +973,7 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, size_t size) } /* pass control to driver for optional further init */ - ret = soc_tplg_control_load(tplg, &kc, (struct snd_soc_tplg_ctl_hdr *)ec); + ret = soc_tplg_control_load(tplg, &kc, &ec->hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", ec->hdr.name); goto err; @@ -1189,7 +1189,7 @@ static int soc_tplg_dapm_widget_dmixer_create(struct soc_tplg *tplg, struct snd_ } /* pass control to driver for optional further init */ - err = soc_tplg_control_load(tplg, kc, (struct snd_soc_tplg_ctl_hdr *)mc); + err = soc_tplg_control_load(tplg, kc, &mc->hdr); if (err < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", mc->hdr.name); @@ -1273,7 +1273,7 @@ static int soc_tplg_dapm_widget_denum_create(struct soc_tplg *tplg, struct snd_k } /* pass control to driver for optional further init */ - err = soc_tplg_control_load(tplg, kc, (struct snd_soc_tplg_ctl_hdr *)ec); + err = soc_tplg_control_load(tplg, kc, &ec->hdr); if (err < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", ec->hdr.name); @@ -1325,7 +1325,7 @@ static int soc_tplg_dapm_widget_dbytes_create(struct soc_tplg *tplg, struct snd_ } /* pass control to driver for optional further init */ - err = soc_tplg_control_load(tplg, kc, (struct snd_soc_tplg_ctl_hdr *)be); + err = soc_tplg_control_load(tplg, kc, &be->hdr); if (err < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", be->hdr.name); -- 2.25.1
On 1/25/23 13:46, Amadeusz Sławiński wrote:
Instead of passing address of structure, the containg structure is
containing?
casted to target structure. While it works - the expected structure is
cast?
the first field of containing one - it is bad practice, fix this by passing pointer to structure field.
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 766de161aa4a..c35c0c53a184 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -721,7 +721,7 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, size_t size) }
/* pass control to driver for optional further init */ - ret = soc_tplg_control_load(tplg, &kc, (struct snd_soc_tplg_ctl_hdr *)be); + ret = soc_tplg_control_load(tplg, &kc, &be->hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", be->hdr.name); goto err; @@ -805,7 +805,7 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, size_t size) }
/* pass control to driver for optional further init */ - ret = soc_tplg_control_load(tplg, &kc, (struct snd_soc_tplg_ctl_hdr *)mc); + ret = soc_tplg_control_load(tplg, &kc, &mc->hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", mc->hdr.name); goto err; @@ -973,7 +973,7 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, size_t size) }
/* pass control to driver for optional further init */ - ret = soc_tplg_control_load(tplg, &kc, (struct snd_soc_tplg_ctl_hdr *)ec); + ret = soc_tplg_control_load(tplg, &kc, &ec->hdr); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", ec->hdr.name); goto err; @@ -1189,7 +1189,7 @@ static int soc_tplg_dapm_widget_dmixer_create(struct soc_tplg *tplg, struct snd_ }
/* pass control to driver for optional further init */ - err = soc_tplg_control_load(tplg, kc, (struct snd_soc_tplg_ctl_hdr *)mc); + err = soc_tplg_control_load(tplg, kc, &mc->hdr); if (err < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", mc->hdr.name); @@ -1273,7 +1273,7 @@ static int soc_tplg_dapm_widget_denum_create(struct soc_tplg *tplg, struct snd_k }
/* pass control to driver for optional further init */ - err = soc_tplg_control_load(tplg, kc, (struct snd_soc_tplg_ctl_hdr *)ec); + err = soc_tplg_control_load(tplg, kc, &ec->hdr); if (err < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", ec->hdr.name); @@ -1325,7 +1325,7 @@ static int soc_tplg_dapm_widget_dbytes_create(struct soc_tplg *tplg, struct snd_ }
/* pass control to driver for optional further init */ - err = soc_tplg_control_load(tplg, kc, (struct snd_soc_tplg_ctl_hdr *)be); + err = soc_tplg_control_load(tplg, kc, &be->hdr); if (err < 0) { dev_err(tplg->dev, "ASoC: failed to init %s\n", be->hdr.name);
Function soc_tplg_dapm_complete() detects an error and logs it, but doesn't return failure to the caller, fix it by returning the error. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index c35c0c53a184..08dd55f94584 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1563,7 +1563,7 @@ static int soc_tplg_dapm_complete(struct soc_tplg *tplg) dev_err(tplg->dev, "ASoC: failed to create new widgets %d\n", ret); - return 0; + return ret; } static int set_stream_info(struct soc_tplg *tplg, struct snd_soc_pcm_stream *stream, -- 2.25.1
Caller already checks if hdr_pos is behind EOF, before calling soc_tplg_valid_header(), so there is no need to recheck it again. This also allows to remove behaviour of return 0 - forcing the caller to break out of while loop. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 08dd55f94584..6689cf44464c 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2389,9 +2389,6 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, static int soc_tplg_valid_header(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { - if (soc_tplg_get_hdr_offset(tplg) >= tplg->fw->size) - return 0; - if (le32_to_cpu(hdr->size) != sizeof(*hdr)) { dev_err(tplg->dev, "ASoC: invalid header size for type %d at offset 0x%lx size 0x%zx.\n", @@ -2442,7 +2439,7 @@ static int soc_tplg_valid_header(struct soc_tplg *tplg, return -EINVAL; } - return 1; + return 0; } /* check header type and call appropriate handler */ @@ -2528,8 +2525,6 @@ static int soc_tplg_process_headers(struct soc_tplg *tplg) dev_err(tplg->dev, "ASoC: topology: invalid header: %d\n", ret); return ret; - } else if (ret == 0) { - break; } /* load the header object */ -- 2.25.1
Generic dynamic object (struct snd_soc_dobj) needs pointer to unload function, however, instead of using function pointer to point at it directly it points to all topology operations. Change code to use the function pointer instead. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- include/sound/soc-topology.h | 2 +- sound/soc/soc-topology.c | 56 ++++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h index b4b896f83b94..f055c6917f6c 100644 --- a/include/sound/soc-topology.h +++ b/include/sound/soc-topology.h @@ -62,7 +62,7 @@ struct snd_soc_dobj { enum snd_soc_dobj_type type; unsigned int index; /* objects can belong in different groups */ struct list_head list; - struct snd_soc_tplg_ops *ops; + int (*unload)(struct snd_soc_component *comp, struct snd_soc_dobj *dobj); union { struct snd_soc_dobj_control control; struct snd_soc_dobj_widget widget; diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 6689cf44464c..eb49037d86ae 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -359,8 +359,8 @@ static void soc_tplg_remove_mixer(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_CONTROL) return; - if (dobj->ops && dobj->ops->control_unload) - dobj->ops->control_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); snd_ctl_remove(card, dobj->control.kcontrol); list_del(&dobj->list); @@ -375,8 +375,8 @@ static void soc_tplg_remove_enum(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_CONTROL) return; - if (dobj->ops && dobj->ops->control_unload) - dobj->ops->control_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); snd_ctl_remove(card, dobj->control.kcontrol); list_del(&dobj->list); @@ -391,8 +391,8 @@ static void soc_tplg_remove_bytes(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_CONTROL) return; - if (dobj->ops && dobj->ops->control_unload) - dobj->ops->control_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); snd_ctl_remove(card, dobj->control.kcontrol); list_del(&dobj->list); @@ -405,8 +405,8 @@ static void soc_tplg_remove_route(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_GRAPH) return; - if (dobj->ops && dobj->ops->dapm_route_unload) - dobj->ops->dapm_route_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); list_del(&dobj->list); } @@ -423,8 +423,8 @@ static void soc_tplg_remove_widget(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_WIDGET) return; - if (dobj->ops && dobj->ops->widget_unload) - dobj->ops->widget_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); if (!w->kcontrols) goto free_news; @@ -450,8 +450,8 @@ static void soc_tplg_remove_dai(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_PCM_DAI) return; - if (dobj->ops && dobj->ops->dai_unload) - dobj->ops->dai_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); for_each_component_dais_safe(comp, dai, _dai) if (dai->driver == dai_drv) @@ -470,8 +470,8 @@ static void soc_tplg_remove_link(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_PCM_DAI) return; - if (dobj->ops && dobj->ops->link_unload) - dobj->ops->link_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); list_del(&dobj->list); snd_soc_remove_pcm_runtime(comp->card, @@ -485,8 +485,8 @@ static void remove_backend_link(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_LINK) return; - if (dobj->ops && dobj->ops->link_unload) - dobj->ops->link_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj); /* * We don't free the link here as what soc_tplg_remove_link() do since BE @@ -710,7 +710,8 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, size_t size) sbe->max = le32_to_cpu(be->max); sbe->dobj.type = SND_SOC_DOBJ_BYTES; - sbe->dobj.ops = tplg->ops; + if (tplg->ops) + sbe->dobj.unload = tplg->ops->control_unload; INIT_LIST_HEAD(&sbe->dobj.list); /* map io handlers */ @@ -786,8 +787,9 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, size_t size) sm->invert = le32_to_cpu(mc->invert); sm->platform_max = le32_to_cpu(mc->platform_max); sm->dobj.index = tplg->index; - sm->dobj.ops = tplg->ops; sm->dobj.type = SND_SOC_DOBJ_MIXER; + if (tplg->ops) + sm->dobj.unload = tplg->ops->control_unload; INIT_LIST_HEAD(&sm->dobj.list); /* map io handlers */ @@ -932,7 +934,8 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, size_t size) se->mask = le32_to_cpu(ec->mask); se->dobj.index = tplg->index; se->dobj.type = SND_SOC_DOBJ_ENUM; - se->dobj.ops = tplg->ops; + if (tplg->ops) + se->dobj.unload = tplg->ops->control_unload; INIT_LIST_HEAD(&se->dobj.list); switch (le32_to_cpu(ec->hdr.ops.info)) { @@ -1109,7 +1112,8 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg, /* add route dobj to dobj_list */ route->dobj.type = SND_SOC_DOBJ_GRAPH; - route->dobj.ops = tplg->ops; + if (tplg->ops) + route->dobj.unload = tplg->ops->control_unload; route->dobj.index = tplg->index; list_add(&route->dobj.list, &tplg->comp->dobj_list); @@ -1475,7 +1479,8 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg, widget->dobj.type = SND_SOC_DOBJ_WIDGET; widget->dobj.widget.kcontrol_type = kcontrol_type; - widget->dobj.ops = tplg->ops; + if (tplg->ops) + widget->dobj.unload = tplg->ops->widget_unload; widget->dobj.index = tplg->index; list_add(&widget->dobj.list, &tplg->comp->dobj_list); @@ -1653,8 +1658,9 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, } dai_drv->dobj.index = tplg->index; - dai_drv->dobj.ops = tplg->ops; dai_drv->dobj.type = SND_SOC_DOBJ_PCM; + if (tplg->ops) + dai_drv->dobj.unload = tplg->ops->dai_unload; list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list); /* register the DAI to the component */ @@ -1723,8 +1729,9 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, link->num_platforms = 1; link->dobj.index = tplg->index; - link->dobj.ops = tplg->ops; link->dobj.type = SND_SOC_DOBJ_DAI_LINK; + if (tplg->ops) + link->dobj.unload = tplg->ops->link_unload; if (strlen(pcm->pcm_name)) { link->name = devm_kstrdup(tplg->dev, pcm->pcm_name, GFP_KERNEL); @@ -2131,8 +2138,9 @@ static int soc_tplg_link_config(struct soc_tplg *tplg, /* for unloading it in snd_soc_tplg_component_remove */ link->dobj.index = tplg->index; - link->dobj.ops = tplg->ops; link->dobj.type = SND_SOC_DOBJ_BACKEND_LINK; + if (tplg->ops) + link->dobj.unload = tplg->ops->link_unload; list_add(&link->dobj.list, &tplg->comp->dobj_list); return 0; -- 2.25.1
On 1/25/23 13:46, Amadeusz Sławiński wrote:
Generic dynamic object (struct snd_soc_dobj) needs pointer to unload function, however, instead of using function pointer to point at it directly it points to all topology operations. Change code to use the function pointer instead.
This is a convoluted explanation, and the code does not support this last sentence. There is no existing dobj 'function pointer', it's added by this patch ....
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- include/sound/soc-topology.h | 2 +- sound/soc/soc-topology.c | 56 ++++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h index b4b896f83b94..f055c6917f6c 100644 --- a/include/sound/soc-topology.h +++ b/include/sound/soc-topology.h @@ -62,7 +62,7 @@ struct snd_soc_dobj { enum snd_soc_dobj_type type; unsigned int index; /* objects can belong in different groups */ struct list_head list; - struct snd_soc_tplg_ops *ops; + int (*unload)(struct snd_soc_component *comp, struct snd_soc_dobj *dobj);
.... here So what this changes is that instead of following the indirections ...
union { struct snd_soc_dobj_control control; struct snd_soc_dobj_widget widget; diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 6689cf44464c..eb49037d86ae 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -359,8 +359,8 @@ static void soc_tplg_remove_mixer(struct snd_soc_component *comp, if (pass != SOC_TPLG_PASS_CONTROL) return;
- if (dobj->ops && dobj->ops->control_unload) - dobj->ops->control_unload(comp, dobj); + if (dobj->unload) + dobj->unload(comp, dobj);
... here, you first need to set that pointer ....
sbe->max = le32_to_cpu(be->max); sbe->dobj.type = SND_SOC_DOBJ_BYTES; - sbe->dobj.ops = tplg->ops; + if (tplg->ops) + sbe->dobj.unload = tplg->ops->control_unload;
... here. I don't see the gain, sorry. Edit: This removal only makes sense with the patch 11 added, where the same function can be used to remove multiple types of control. Please revisit the commit message here and explain the intent, otherwise this change in isolation isn't really useful.
Functions removing bytes, enum and mixer kcontrols are identical. Unify them under one function and use it to free associated kcontrols. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 48 +++++----------------------------------- 1 file changed, 6 insertions(+), 42 deletions(-) diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index eb49037d86ae..e66b0d9e387a 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -350,41 +350,9 @@ static int soc_tplg_add_kcontrol(struct soc_tplg *tplg, tplg->dev, k, comp->name_prefix, comp, kcontrol); } -/* remove a mixer kcontrol */ -static void soc_tplg_remove_mixer(struct snd_soc_component *comp, - struct snd_soc_dobj *dobj, int pass) -{ - struct snd_card *card = comp->card->snd_card; - - if (pass != SOC_TPLG_PASS_CONTROL) - return; - - if (dobj->unload) - dobj->unload(comp, dobj); - - snd_ctl_remove(card, dobj->control.kcontrol); - list_del(&dobj->list); -} - -/* remove an enum kcontrol */ -static void soc_tplg_remove_enum(struct snd_soc_component *comp, - struct snd_soc_dobj *dobj, int pass) -{ - struct snd_card *card = comp->card->snd_card; - - if (pass != SOC_TPLG_PASS_CONTROL) - return; - - if (dobj->unload) - dobj->unload(comp, dobj); - - snd_ctl_remove(card, dobj->control.kcontrol); - list_del(&dobj->list); -} - -/* remove a byte kcontrol */ -static void soc_tplg_remove_bytes(struct snd_soc_component *comp, - struct snd_soc_dobj *dobj, int pass) +/* remove kcontrol */ +static void soc_tplg_remove_kcontrol(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, + int pass) { struct snd_card *card = comp->card->snd_card; @@ -2626,14 +2594,10 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp) list) { switch (dobj->type) { - case SND_SOC_DOBJ_MIXER: - soc_tplg_remove_mixer(comp, dobj, pass); - break; - case SND_SOC_DOBJ_ENUM: - soc_tplg_remove_enum(comp, dobj, pass); - break; case SND_SOC_DOBJ_BYTES: - soc_tplg_remove_bytes(comp, dobj, pass); + case SND_SOC_DOBJ_ENUM: + case SND_SOC_DOBJ_MIXER: + soc_tplg_remove_kcontrol(comp, dobj, pass); break; case SND_SOC_DOBJ_GRAPH: soc_tplg_remove_route(comp, dobj, pass); -- 2.25.1
On 1/25/23 13:46, Amadeusz Sławiński wrote:
Functions removing bytes, enum and mixer kcontrols are identical. Unify
they are identical because of the change in patch10. Please clarify that this is not a cleanup removing duplicated code that's been there forever, it's become useless as a result of the previous patch.
them under one function and use it to free associated kcontrols.
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/soc-topology.c | 48 +++++----------------------------------- 1 file changed, 6 insertions(+), 42 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index eb49037d86ae..e66b0d9e387a 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -350,41 +350,9 @@ static int soc_tplg_add_kcontrol(struct soc_tplg *tplg, tplg->dev, k, comp->name_prefix, comp, kcontrol); }
-/* remove a mixer kcontrol */ -static void soc_tplg_remove_mixer(struct snd_soc_component *comp, - struct snd_soc_dobj *dobj, int pass) -{ - struct snd_card *card = comp->card->snd_card; - - if (pass != SOC_TPLG_PASS_CONTROL) - return; - - if (dobj->unload) - dobj->unload(comp, dobj); - - snd_ctl_remove(card, dobj->control.kcontrol); - list_del(&dobj->list); -} - -/* remove an enum kcontrol */ -static void soc_tplg_remove_enum(struct snd_soc_component *comp, - struct snd_soc_dobj *dobj, int pass) -{ - struct snd_card *card = comp->card->snd_card; - - if (pass != SOC_TPLG_PASS_CONTROL) - return; - - if (dobj->unload) - dobj->unload(comp, dobj); - - snd_ctl_remove(card, dobj->control.kcontrol); - list_del(&dobj->list); -} - -/* remove a byte kcontrol */ -static void soc_tplg_remove_bytes(struct snd_soc_component *comp, - struct snd_soc_dobj *dobj, int pass) +/* remove kcontrol */ +static void soc_tplg_remove_kcontrol(struct snd_soc_component *comp, struct snd_soc_dobj *dobj, + int pass) { struct snd_card *card = comp->card->snd_card;
@@ -2626,14 +2594,10 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp) list) {
switch (dobj->type) { - case SND_SOC_DOBJ_MIXER: - soc_tplg_remove_mixer(comp, dobj, pass); - break; - case SND_SOC_DOBJ_ENUM: - soc_tplg_remove_enum(comp, dobj, pass); - break; case SND_SOC_DOBJ_BYTES: - soc_tplg_remove_bytes(comp, dobj, pass); + case SND_SOC_DOBJ_ENUM: + case SND_SOC_DOBJ_MIXER: + soc_tplg_remove_kcontrol(comp, dobj, pass); break; case SND_SOC_DOBJ_GRAPH: soc_tplg_remove_route(comp, dobj, pass);
On 1/25/2023 4:15 PM, Pierre-Louis Bossart wrote:
On 1/25/23 13:46, Amadeusz Sławiński wrote:
Functions removing bytes, enum and mixer kcontrols are identical. Unify
they are identical because of the change in patch10.
Please clarify that this is not a cleanup removing duplicated code that's been there forever, it's become useless as a result of the previous patch.
There is no dependency on previous patch - it is just order I've send them in - those functions have same implementation in current code.
On 1/27/23 05:12, Amadeusz Sławiński wrote:
On 1/25/2023 4:15 PM, Pierre-Louis Bossart wrote:
On 1/25/23 13:46, Amadeusz Sławiński wrote:
Functions removing bytes, enum and mixer kcontrols are identical. Unify
they are identical because of the change in patch10.
Please clarify that this is not a cleanup removing duplicated code that's been there forever, it's become useless as a result of the previous patch.
There is no dependency on previous patch - it is just order I've send them in - those functions have same implementation in current code.
Not following, sorry. What is this addition in patch 10? diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h index b4b896f83b94..f055c6917f6c 100644 --- a/include/sound/soc-topology.h +++ b/include/sound/soc-topology.h @@ -62,7 +62,7 @@ struct snd_soc_dobj { enum snd_soc_dobj_type type; unsigned int index; /* objects can belong in different groups */ struct list_head list; - struct snd_soc_tplg_ops *ops; + int (*unload)(struct snd_soc_component *comp, struct snd_soc_dobj *dobj); That's not in 'current code', is it? How is this not a dependency?
On 1/27/2023 2:38 PM, Pierre-Louis Bossart wrote:
On 1/27/23 05:12, Amadeusz Sławiński wrote:
On 1/25/2023 4:15 PM, Pierre-Louis Bossart wrote:
On 1/25/23 13:46, Amadeusz Sławiński wrote:
Functions removing bytes, enum and mixer kcontrols are identical. Unify
they are identical because of the change in patch10.
Please clarify that this is not a cleanup removing duplicated code that's been there forever, it's become useless as a result of the previous patch.
There is no dependency on previous patch - it is just order I've send them in - those functions have same implementation in current code.
Not following, sorry. What is this addition in patch 10?
diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h index b4b896f83b94..f055c6917f6c 100644 --- a/include/sound/soc-topology.h +++ b/include/sound/soc-topology.h @@ -62,7 +62,7 @@ struct snd_soc_dobj { enum snd_soc_dobj_type type; unsigned int index; /* objects can belong in different groups */ struct list_head list; - struct snd_soc_tplg_ops *ops; + int (*unload)(struct snd_soc_component *comp, struct snd_soc_dobj *dobj);
That's not in 'current code', is it? How is this not a dependency?
It only depends on it because of order I did changes in, but there is really no dependency here. I will send v2 with reversed order.
On Wed, 2023-01-25 at 20:46 +0100, Amadeusz Sławiński wrote:
Following is series of fixes and cleanups for core topology code. Few patches fixing various problems all around and few fixing function names.
Amadeusz Sławiński (11): ASoC: topology: Properly access value coming from topology file ASoC: topology: Remove unused SOC_TPLG_PASS_PINS constant ASoC: topology: Fix typo in functions name ASoC: topology: Fix function name ASoC: topology: Rename remove_ handlers ASoC: topology: Remove unnecessary forward declarations ASoC: topology: Pass correct pointer instead of casting ASoC: topology: Return an error on complete() failure ASoC: Topology: Remove unnecessary check for EOF ASoC: topology: Use unload() op directly ASoC: topology: Unify kcontrol removal code
LGTM, thanks, Amadeusz! Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
participants (3)
-
Amadeusz Sławiński -
Pierre-Louis Bossart -
Ranjani Sridharan