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.
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 | 94 ++++++++++++++++---------------- 3 files changed, 58 insertions(+), 84 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; }
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 Signed-off-by: Vaibhav Agarwal vaibhav.sr@gmail.com --- 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, },
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 Signed-off-by: Vaibhav Agarwal vaibhav.sr@gmail.com --- 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, };
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 Signed-off-by: Vaibhav Agarwal vaibhav.sr@gmail.com --- drivers/staging/greybus/audio_module.c | 2 +- drivers/staging/greybus/audio_topology.c | 82 +++++++++++++++++--------------- 2 files changed, 45 insertions(+), 39 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..1cff2440c6d3 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++) + le16_to_cpu(gb_enum->names_length)); + for (i = 0; i < le32_to_cpu(gb_enum->items); 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,12 +869,12 @@ 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); + le16_to_cpu(gb_enum->names_length)); for (i = 0; i < gb_enum->items; i++) dev_dbg(gb->dev, "src[%d]: %s\n", i, gbe->texts[i]);
@@ -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 Sat, Jan 14, 2017 at 11:17:07PM +0530, Vaibhav Agarwal wrote:
@@ -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,
This is printing little endian max. Just use gbe->max here.
gb_enum->names_length);- for (i = 0; i < gb_enum->items; i++)
le16_to_cpu(gb_enum->names_length));- for (i = 0; i < le32_to_cpu(gb_enum->items); i++)
And here as well probably?
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,12 +869,12 @@ 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,
Same.
gb_enum->names_length);
le16_to_cpu(gb_enum->names_length));
^^^^^^^^^^^^^^ This one needs to be converted as well, I think.
dev_dbg(gb->dev, "src[%d]: %s\n", i, gbe->texts[i]);
regards, dan carpenter
On Mon, Jan 16, 2017 at 4:17 PM, Dan Carpenter dan.carpenter@oracle.com wrote:
On Sat, Jan 14, 2017 at 11:17:07PM +0530, Vaibhav Agarwal wrote:
@@ -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,This is printing little endian max. Just use gbe->max here.
Yes, this makes more sense. I should have noticed this earlier :( Surely, I'll include this one & other suggested changes in v2 series. Thanks Dan :)
-- vaibhav
-
Dan Carpenter -
Vaibhav Agarwal