No wname in kcontrol name #4445
Conversation
The existing soc-dapm code may add a prefix to control names, which in some cases is useful but in others leads to long and confusing kcontrol names such as "gain 2.1 Main Playback Volume". This patch suggests an added flag to prevent the widget name prefix from being added. That flag will be set in the topology file on a per-widget basis. The flag no_wname_in_kcontrol_name is added to struct snd_soc_dapm_widget, and the logic in dapm_create_or_share_kcontrol() is changed to not to add widget name if the flag is set. Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
jsarha
requested review from
ajitkupandey,
bardliao,
dbaluta,
juimonen,
kuanhsuncheng,
kv2019i,
lgirdwood,
lyakh,
plbossart,
ranj063,
singalsu,
ujfalusi and
yaochunhung
as code owners
include/uapi/sound/sof/tokens.h
Outdated
| #define SOF_TKN_COMP_NUM_AUDIO_FORMATS 410 | ||
| #define SOF_TKN_COMP_NUM_INPUT_PINS 411 | ||
| #define SOF_TKN_COMP_NUM_OUTPUT_PINS 412 | ||
|
|
sound/soc/sof/topology.c
Outdated
| type == SND_SOC_TPLG_TUPLE_TYPE_BYTE || | ||
| type == SND_SOC_TPLG_TUPLE_TYPE_BOOL)) { | ||
| dev_err(scomp->dev, "Bad tuple type %d\n", type); | ||
| return -EINVAL; |
There was a problem hiding this comment.
I don't understand what this error check does?
There was a problem hiding this comment.
If the tuple data type is not something that can easily be interpreted as boolean, then there is reason to believe that the topology is otherwise broken too. But sure, I can change this to a warning only.
sound/soc/sof/topology.c
Outdated
| break; | ||
| } | ||
| if (ret != 0) | ||
| return ret; |
There was a problem hiding this comment.
and do we really want to stop the topology parsing here? This seems like a warning at best to me, this doesn't break functionality, does it?
There was a problem hiding this comment.
Yep, maybe not. I'll remove it and make sof_w_no_wname_in_long_name() void.
jsarha
force-pushed
the
no_wname_in_kcontrol_name
branch
3 times, most recently
from
64b366f to
cb4b59e
Compare
|
@plbossart I think have your comments covered. |
sound/soc/sof/topology.c
Outdated
There was a problem hiding this comment.
@jsarha why do we need to duplicate the parsing code when we can simply do this:
static const struct sof_topology_token widget_kcontrol_name_token[] = {
{SOF_TKN_COMP_NO_WNAME_IN_KCONTROL_NAME, SND_SOC_TPLG_TUPLE_TYPE_BOOL, get_token_u16,
offsetof(struct snd_soc_dapm_widget, no_wname_in_kcontrol_name)},
};
ret = sof_parse_tokens(scomp, w, widget_kcontrol_name_token,
ARRAY_SIZE(widget_kcontrol_name_token), private->array,
le32_to_cpu(private->size));
if (ret) {
dev_err(scomp->dev, "error: parse stream tokens failed %d\n",
le32_to_cpu(private->size));
return ret;
}
There was a problem hiding this comment.
@ranj063 , I am pretty sure that does not work with bit-fields. I would have to turn no_wname_in_kcontrol_name from 1 bit wide to one byte wide. offsetof only has one byte granularity, hasn't it?
There was a problem hiding this comment.
@jsarha any reason why we can't make it a bool or a byte instead of a 1bit value?
There was a problem hiding this comment.
Only that all the other flags in the struct are bit-fields, so it would look a bit out of place there, and the header is generic ASoC code, not sof only. Of course we have @lgirdwood in the team so we can ask directly, what do you say? Can I change this just unsigned char: 44de36b
The other option would be adding a bit-mask to struct sof_topology_token and implementing get_token_u8_bitfield().
There was a problem hiding this comment.
@jsarha I dont think there's any restriction that this can to be a bit field in the struct. I'd modify it to a bool to keep it simple.
There was a problem hiding this comment.
Simple me... its not that hard really. I can of course do it like this:
static int get_w_no_wname_in_long_name(void *elem, void *object, u32 offset)
{
struct snd_soc_tplg_vendor_value_elem *velem = elem;
struct snd_soc_dapm_widget *w = object;
w->no_wname_in_kcontrol_name = !!le32_to_cpu(velem->value);
return 0;
}
static const struct sof_topology_token dapm_widget_token[] = {
{SOF_TKN_COMP_NO_WNAME_IN_KCONTROL_NAME, SND_SOC_TPLG_TUPLE_TYPE_BOOL,
get_w_no_wname_in_long_name, 0 }
};
That would be fine, would't it, @ranj063 ?
… name Adds SOF_TKN_COMP_NO_WNAME_IN_KCONTROL_NAME token, and copies the token's tuple value to the no_wname_in_kcontrol_name flag in struct snd_soc_dapm_widget. If the tuple value for the token in the topology is true, then the widget name is not added to the mixer name. In practice "gain.2.1 Post Mixer Analog Playback Volume" becomes just "Post Mixer Analog Playback Volume". Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
jsarha
force-pushed
the
no_wname_in_kcontrol_name
branch
from
cb4b59e to
be19cd0
Compare
4 participants
Ok, this PR has evolved enough so that the branch name does not anymore match the content, so I guess its time to turn the page and create a new one. The predecessor can be found as draft here: #4226