This patch series include following cleanup changes in GB Audio driver. - Avoid unnecessary checks for le32 variables - Initialize sig_bits before configuring hw_params - Remove junk codec register mapping - Ensure proper byte ordering
Next task is to enable build for GB codec driver. However this requires pushing some changes in ASoC framework. Possibly in another two weeks (based on my freetime), I'll try to submit those changes to ASoC mailing list. And once the same are accepted there, I'll share relevant patches for GB Audio codec driver as well.
Changes from v1: - Include review comments from Dan
Vaibhav Agarwal (4): staging: greybus: audio: Avoid less than zero check for le32 variable staging: greybus: audio: Initialize sig_bits before configuring hwparams staging: greybus: audio: Cleanup junk codec registers staging: greybus: audio: Ensure proper byte order
drivers/staging/greybus/audio_codec.c | 46 +++----------- drivers/staging/greybus/audio_module.c | 2 +- drivers/staging/greybus/audio_topology.c | 100 ++++++++++++++++--------------- 3 files changed, 61 insertions(+), 87 deletions(-)
mixer control->info call back function checks for -ve values to rebase min and max values. However, le32 variable is used to fetch values from GB module FW. Thus -ve value checking is not required. Fix this!!
Signed-off-by: Vaibhav Agarwal vaibhav.sr@gmail.com --- drivers/staging/greybus/audio_topology.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 3001a4971c06..a8b63a8b2bb0 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -361,8 +361,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct snd_kcontrol *kcontrol, info = (struct gb_audio_ctl_elem_info *)data->info;
/* update uinfo */ - platform_max = info->value.integer.max; - platform_min = info->value.integer.min; + platform_max = le32_to_cpu(info->value.integer.max); + platform_min = le32_to_cpu(info->value.integer.min);
if (platform_max == 1 && !strnstr(kcontrol->id.name, " Volume", NAME_SIZE)) @@ -371,12 +371,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct snd_kcontrol *kcontrol, uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
uinfo->count = data->vcount; - uinfo->value.integer.min = 0; - if (info->value.integer.min < 0 && - (uinfo->type == SNDRV_CTL_ELEM_TYPE_INTEGER)) - uinfo->value.integer.max = platform_max - platform_min; - else - uinfo->value.integer.max = platform_max; + uinfo->value.integer.min = platform_min; + uinfo->value.integer.max = platform_max;
return 0; }
Hi Vaibhav.
On Tue, Jan 17, 2017 at 08:19:27PM +0530, Vaibhav Agarwal wrote:
nit: Please spell out "negative" so it is as easy as possible for the casual observer to understand what you're saying.
Should this piece be in patch 4/4? It seems out of place in this patch.
This part looks good.
Thanks,
Mark --
On Tue, Jan 17, 2017 at 10:56 PM, Mark Greer mgreer@animalcreek.com wrote:
Will modify in v3.
Yes. It should be part of 4/4 only. Will update in v3
-- vaibhav
From: Vaibhav Agarwal vaibhav.agarwal@linaro.org
Uninitialized variable sig_bits was used while configuring stream params for codec module. These params are used to configure PCM settings for APBridgeA.
Usually, this is dependent on codec capability and thus populated via codec dai_driver definition. In our case, it is fixed to 16 based on the data format, container supported.
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- drivers/staging/greybus/audio_codec.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c index f8862c6d7102..b9d66278ff87 100644 --- a/drivers/staging/greybus/audio_codec.c +++ b/drivers/staging/greybus/audio_codec.c @@ -496,6 +496,11 @@ static int gbcodec_hw_params(struct snd_pcm_substream *substream,
gb_pm_runtime_put_noidle(bundle);
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + sig_bits = dai->driver->playback.sig_bits; + else + sig_bits = dai->driver->capture.sig_bits; + params->state = GBAUDIO_CODEC_HWPARAMS; params->format = format; params->rate = rate; @@ -689,6 +694,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = { .rate_min = 48000, .channels_min = 1, .channels_max = 2, + .sig_bits = 16, }, .capture = { .stream_name = "I2S 0 Capture", @@ -698,6 +704,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = { .rate_min = 48000, .channels_min = 1, .channels_max = 2, + .sig_bits = 16, }, .ops = &gbcodec_dai_ops, },
On Tue, Jan 17, 2017 at 08:19:28PM +0530, Vaibhav Agarwal wrote:
Acked-by: Mark Greer mgreer@animalcreek.com
From: Vaibhav Agarwal vaibhav.agarwal@linaro.org
Dummy codec register were initially added while populating dummy codec mixer controls until module topology parser was available. Now, these dummy registers are nowhere used and thus can be safely removed.
Since ASoC framework requires a valid callback for both read & write register APIS, currently empty placeholders are kept to avoid panic.
Later, register mapping logic can be defined: 1. Assuming fixed number of maximum modules connected and register bits corresponds to basic info of each module OR 2. With a logic to dynamically grow register_cache_size based on codec modules added/removed.
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- drivers/staging/greybus/audio_codec.c | 39 ++--------------------------------- 1 file changed, 2 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c index b9d66278ff87..30941f9e380d 100644 --- a/drivers/staging/greybus/audio_codec.c +++ b/drivers/staging/greybus/audio_codec.c @@ -1026,47 +1026,16 @@ static int gbcodec_remove(struct snd_soc_codec *codec) return 0; }
-static u8 gbcodec_reg[GBCODEC_REG_COUNT] = { - [GBCODEC_CTL_REG] = GBCODEC_CTL_REG_DEFAULT, - [GBCODEC_MUTE_REG] = GBCODEC_MUTE_REG_DEFAULT, - [GBCODEC_PB_LVOL_REG] = GBCODEC_PB_VOL_REG_DEFAULT, - [GBCODEC_PB_RVOL_REG] = GBCODEC_PB_VOL_REG_DEFAULT, - [GBCODEC_CAP_LVOL_REG] = GBCODEC_CAP_VOL_REG_DEFAULT, - [GBCODEC_CAP_RVOL_REG] = GBCODEC_CAP_VOL_REG_DEFAULT, - [GBCODEC_APB1_MUX_REG] = GBCODEC_APB1_MUX_REG_DEFAULT, - [GBCODEC_APB2_MUX_REG] = GBCODEC_APB2_MUX_REG_DEFAULT, -}; - static int gbcodec_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - int ret = 0; - - if (reg == SND_SOC_NOPM) - return 0; - - BUG_ON(reg >= GBCODEC_REG_COUNT); - - gbcodec_reg[reg] = value; - dev_dbg(codec->dev, "reg[%d] = 0x%x\n", reg, value); - - return ret; + return 0; }
static unsigned int gbcodec_read(struct snd_soc_codec *codec, unsigned int reg) { - unsigned int val = 0; - - if (reg == SND_SOC_NOPM) - return 0; - - BUG_ON(reg >= GBCODEC_REG_COUNT); - - val = gbcodec_reg[reg]; - dev_dbg(codec->dev, "reg[%d] = 0x%x\n", reg, val); - - return val; + return 0; }
static struct snd_soc_codec_driver soc_codec_dev_gbaudio = { @@ -1076,10 +1045,6 @@ static struct snd_soc_codec_driver soc_codec_dev_gbaudio = { .read = gbcodec_read, .write = gbcodec_write,
- .reg_cache_size = GBCODEC_REG_COUNT, - .reg_cache_default = gbcodec_reg_defaults, - .reg_word_size = 1, - .idle_bias_off = true, .ignore_pmdown_time = 1, };
On Tue, Jan 17, 2017 at 08:19:29PM +0530, Vaibhav Agarwal wrote:
gbcodec_reg_defaults[] and the definitions of 'GBCODEC_CTL_REG_DEFAULT', etc. can be removed now too, can't they?
The rest looks good.
Mark --
On Tue, Jan 17, 2017 at 11:04 PM, Mark Greer mgreer@animalcreek.com wrote:
Of course, this should be removed as well. Nice catch. Thanks :)
-- vaibhav
The rest looks good.
Mark
From: Vaibhav Agarwal vaibhav.agarwal@linaro.org
Proper byte order was completely disregarded for multi byte data shared between AP and module (and APB1). Fix this.
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- drivers/staging/greybus/audio_module.c | 2 +- drivers/staging/greybus/audio_topology.c | 88 +++++++++++++++++--------------- 2 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c index 17a9948b1ba1..094c3be79b33 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -134,7 +134,7 @@ static int gbaudio_request_stream(struct gbaudio_module_info *module, struct gb_audio_streaming_event_request *req) { dev_warn(module->dev, "Audio Event received: cport: %u, event: %u\n", - req->data_cport, req->event); + le16_to_cpu(req->data_cport), req->event);
return 0; } diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index a8b63a8b2bb0..4002a57977bd 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -141,13 +141,14 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb, { const char **strings; int i; + unsigned int items; __u8 *data;
- strings = devm_kzalloc(gb->dev, sizeof(char *) * gbenum->items, - GFP_KERNEL); + items = le32_to_cpu(gbenum->items); + strings = devm_kzalloc(gb->dev, sizeof(char *) * items, GFP_KERNEL); data = gbenum->names;
- for (i = 0; i < gbenum->items; i++) { + for (i = 0; i < items; i++) { strings[i] = (const char *)data; while (*data != '\0') data++; @@ -185,11 +186,11 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol, switch (info->type) { case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN: case GB_AUDIO_CTL_ELEM_TYPE_INTEGER: - uinfo->value.integer.min = info->value.integer.min; - uinfo->value.integer.max = info->value.integer.max; + uinfo->value.integer.min = le32_to_cpu(info->value.integer.min); + uinfo->value.integer.max = le32_to_cpu(info->value.integer.max); break; case GB_AUDIO_CTL_ELEM_TYPE_ENUMERATED: - max = info->value.enumerated.items; + max = le32_to_cpu(info->value.enumerated.items); uinfo->value.enumerated.items = max; if (uinfo->value.enumerated.item > max - 1) uinfo->value.enumerated.item = max - 1; @@ -249,17 +250,17 @@ static int gbcodec_mixer_ctl_get(struct snd_kcontrol *kcontrol, case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN: case GB_AUDIO_CTL_ELEM_TYPE_INTEGER: ucontrol->value.integer.value[0] = - gbvalue.value.integer_value[0]; + le32_to_cpu(gbvalue.value.integer_value[0]); if (data->vcount == 2) ucontrol->value.integer.value[1] = - gbvalue.value.integer_value[1]; + le32_to_cpu(gbvalue.value.integer_value[1]); break; case GB_AUDIO_CTL_ELEM_TYPE_ENUMERATED: ucontrol->value.enumerated.item[0] = - gbvalue.value.enumerated_item[0]; + le32_to_cpu(gbvalue.value.enumerated_item[0]); if (data->vcount == 2) ucontrol->value.enumerated.item[1] = - gbvalue.value.enumerated_item[1]; + le32_to_cpu(gbvalue.value.enumerated_item[1]); break; default: dev_err(codec->dev, "Invalid type: %d for %s:kcontrol\n", @@ -296,17 +297,17 @@ static int gbcodec_mixer_ctl_put(struct snd_kcontrol *kcontrol, case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN: case GB_AUDIO_CTL_ELEM_TYPE_INTEGER: gbvalue.value.integer_value[0] = - ucontrol->value.integer.value[0]; + cpu_to_le32(ucontrol->value.integer.value[0]); if (data->vcount == 2) gbvalue.value.integer_value[1] = - ucontrol->value.integer.value[1]; + cpu_to_le32(ucontrol->value.integer.value[1]); break; case GB_AUDIO_CTL_ELEM_TYPE_ENUMERATED: gbvalue.value.enumerated_item[0] = - ucontrol->value.enumerated.item[0]; + cpu_to_le32(ucontrol->value.enumerated.item[0]); if (data->vcount == 2) gbvalue.value.enumerated_item[1] = - ucontrol->value.enumerated.item[1]; + cpu_to_le32(ucontrol->value.enumerated.item[1]); break; default: dev_err(codec->dev, "Invalid type: %d for %s:kcontrol\n", @@ -420,7 +421,8 @@ static int gbcodec_mixer_dapm_ctl_get(struct snd_kcontrol *kcontrol, return ret; } /* update ucontrol */ - ucontrol->value.integer.value[0] = gbvalue.value.integer_value[0]; + ucontrol->value.integer.value[0] = + le32_to_cpu(gbvalue.value.integer_value[0]);
return ret; } @@ -470,7 +472,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol, connect); } gbvalue.value.integer_value[0] = - ucontrol->value.integer.value[0]; + cpu_to_le32(ucontrol->value.integer.value[0]);
ret = gb_pm_runtime_get_sync(bundle); if (ret) @@ -584,10 +586,11 @@ static int gbcodec_enum_ctl_get(struct snd_kcontrol *kcontrol, return ret; }
- ucontrol->value.enumerated.item[0] = gbvalue.value.enumerated_item[0]; + ucontrol->value.enumerated.item[0] = + le32_to_cpu(gbvalue.value.enumerated_item[0]); if (e->shift_l != e->shift_r) ucontrol->value.enumerated.item[1] = - gbvalue.value.enumerated_item[1]; + le32_to_cpu(gbvalue.value.enumerated_item[1]);
return 0; } @@ -613,13 +616,14 @@ static int gbcodec_enum_ctl_put(struct snd_kcontrol *kcontrol,
if (ucontrol->value.enumerated.item[0] > e->max - 1) return -EINVAL; - gbvalue.value.enumerated_item[0] = ucontrol->value.enumerated.item[0]; + gbvalue.value.enumerated_item[0] = + cpu_to_le32(ucontrol->value.enumerated.item[0]);
if (e->shift_l != e->shift_r) { if (ucontrol->value.enumerated.item[1] > e->max - 1) return -EINVAL; gbvalue.value.enumerated_item[1] = - ucontrol->value.enumerated.item[1]; + cpu_to_le32(ucontrol->value.enumerated.item[1]); }
bundle = to_gb_bundle(module->dev); @@ -656,13 +660,13 @@ static int gbaudio_tplg_create_enum_kctl(struct gbaudio_module_info *gb, gb_enum = &ctl->info.value.enumerated;
/* since count=1, and reg is dummy */ - gbe->max = gb_enum->items; + gbe->max = le32_to_cpu(gb_enum->items); gbe->texts = gb_generate_enum_strings(gb, gb_enum);
/* debug enum info */ - dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gb_enum->items, - gb_enum->names_length); - for (i = 0; i < gb_enum->items; i++) + dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->max, + le16_to_cpu(gb_enum->names_length)); + for (i = 0; i < gbe->max; i++) dev_dbg(gb->dev, "src[%d]: %s\n", i, gbe->texts[i]);
*kctl = (struct snd_kcontrol_new) @@ -691,7 +695,7 @@ static int gbaudio_tplg_create_kcontrol(struct gbaudio_module_info *gb, if (!ctldata) return -ENOMEM; ctldata->ctl_id = ctl->id; - ctldata->data_cport = ctl->data_cport; + ctldata->data_cport = le16_to_cpu(ctl->data_cport); ctldata->access = ctl->access; ctldata->vcount = ctl->count_values; ctldata->info = &ctl->info; @@ -865,13 +869,13 @@ static int gbaudio_tplg_create_enum_ctl(struct gbaudio_module_info *gb, gb_enum = &ctl->info.value.enumerated;
/* since count=1, and reg is dummy */ - gbe->max = gb_enum->items; + gbe->max = le32_to_cpu(gb_enum->items); gbe->texts = gb_generate_enum_strings(gb, gb_enum);
/* debug enum info */ - dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gb_enum->items, - gb_enum->names_length); - for (i = 0; i < gb_enum->items; i++) + dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->max, + le16_to_cpu(gb_enum->names_length)); + for (i = 0; i < gbe->max; i++) dev_dbg(gb->dev, "src[%d]: %s\n", i, gbe->texts[i]);
*kctl = (struct snd_kcontrol_new) @@ -891,7 +895,7 @@ static int gbaudio_tplg_create_mixer_ctl(struct gbaudio_module_info *gb, if (!ctldata) return -ENOMEM; ctldata->ctl_id = ctl->id; - ctldata->data_cport = ctl->data_cport; + ctldata->data_cport = le16_to_cpu(ctl->data_cport); ctldata->access = ctl->access; ctldata->vcount = ctl->count_values; ctldata->info = &ctl->info; @@ -1037,10 +1041,10 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, csize = offsetof(struct gb_audio_control, info); csize += offsetof(struct gb_audio_ctl_elem_info, value); csize += offsetof(struct gb_audio_enumerated, names); - csize += gbenum->names_length; + csize += le16_to_cpu(gbenum->names_length); control->texts = (const char * const *) gb_generate_enum_strings(module, gbenum); - control->items = gbenum->items; + control->items = le32_to_cpu(gbenum->items); } else { csize = sizeof(struct gb_audio_control); } @@ -1185,10 +1189,10 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, csize = offsetof(struct gb_audio_control, info); csize += offsetof(struct gb_audio_ctl_elem_info, value); csize += offsetof(struct gb_audio_enumerated, names); - csize += gbenum->names_length; + csize += le16_to_cpu(gbenum->names_length); control->texts = (const char * const *) gb_generate_enum_strings(module, gbenum); - control->items = gbenum->items; + control->items = le32_to_cpu(gbenum->items); } else { csize = sizeof(struct gb_audio_control); } @@ -1331,11 +1335,12 @@ static int gbaudio_tplg_process_header(struct gbaudio_module_info *module,
/* update block offset */ module->dai_offset = (unsigned long)&tplg_data->data; - module->control_offset = module->dai_offset + tplg_data->size_dais; + module->control_offset = module->dai_offset + + le32_to_cpu(tplg_data->size_dais); module->widget_offset = module->control_offset + - tplg_data->size_controls; + le32_to_cpu(tplg_data->size_controls); module->route_offset = module->widget_offset + - tplg_data->size_widgets; + le32_to_cpu(tplg_data->size_widgets);
dev_dbg(module->dev, "DAI offset is 0x%lx\n", module->dai_offset); dev_dbg(module->dev, "control offset is %lx\n", @@ -1353,6 +1358,7 @@ int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, struct gb_audio_control *controls; struct gb_audio_widget *widgets; struct gb_audio_route *routes; + unsigned int jack_type;
if (!tplg_data) return -EINVAL; @@ -1395,10 +1401,10 @@ int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, dev_dbg(module->dev, "Route parsing finished\n");
/* parse jack capabilities */ - if (tplg_data->jack_type) { - module->jack_mask = tplg_data->jack_type & GBCODEC_JACK_MASK; - module->button_mask = tplg_data->jack_type & - GBCODEC_JACK_BUTTON_MASK; + jack_type = le32_to_cpu(tplg_data->jack_type); + if (jack_type) { + module->jack_mask = jack_type & GBCODEC_JACK_MASK; + module->button_mask = jack_type & GBCODEC_JACK_BUTTON_MASK; }
return ret;
On Tue, Jan 17, 2017 at 08:19:30PM +0530, Vaibhav Agarwal wrote:
Hi Vaibhav.
I think you got them all except for this one in audio_topology.c:gbcodec_mixer_dapm_ctl_put():
max = info->value.integer.max; <<<
mask = (1 << fls(max)) - 1; val = ucontrol->value.integer.value[0] & mask; connect = !!val;
Mark --
On Tue, Jan 17, 2017 at 11:22 PM, Mark Greer mgreer@animalcreek.com wrote:
Oh Ok. I'll include this change in v3.
-- vaibhav
Mark
-
Mark Greer -
Vaibhav Agarwal