Re: [PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai driver
Thanks for the review comments,
On 02/03/18 12:50, Mark Brown wrote:
On Tue, Feb 13, 2018 at 04:58:26PM +, srinivas.kandaga...@linaro.org wrote:
+static int q6hdmi_format_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct q6afe_dai_data *dai_data = kcontrol->private_data;
+ int value = ucontrol->value.integer.value[0];
+
+ dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value;
+
+ return 0;
+}
No validation, and do we not need to tell a currently running stream if
the format changed on it (or block such changes if they're not going to
work, which seems more likely)?
Yes, It would not work if the stream is running.
This mixer has to be setup before the stream/port is prepared/started.
TBH, I have no means to test Compr format, I should probably remove this
control until am able to test this format.
+static int q6hdmi_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+ int channels = params_channels(params);
+ struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi;
+
+ hdmi->sample_rate = params_rate(params);
+ switch (params_format(params)) {
+ case SNDRV_PCM_FORMAT_S16_LE:
+ hdmi->bit_width = 16;
+ break;
+ case SNDRV_PCM_FORMAT_S24_LE:
+ hdmi->bit_width = 24;
+ break;
+ }
This silently accepts invalid values.
Yep, I will fix this in next version.
+ /*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/
Coding style, spaces around the /* */.
Agreed! Will fix it in next version.
+static int q6afe_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+
+ dai_data->is_port_started[dai->id] = false;
+
+ return 0;
+}
If this is needed it makes me a bit worried that we've got some kind of
bug with not shutting things down properly somewhere - what's going on
here?
This looks over done, we do not need to set this flag in startup, as it
would be properly reset in shutdown.
Will remove this function totally as its not required.
+static void q6afe_dai_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+ int rc;
+
+ rc = q6afe_port_stop(dai_data->port[dai->id]);
+ if (rc < 0)
+ dev_err(dai->dev, "fail to close AFE port\n");
Better to print the error code so users have more information to debug
the problem.
Yep.
+ .stream_name = "HDMI Playback",
+ .rates = SNDRV_PCM_RATE_48000 |
+SNDRV_PCM_RATE_96000 |
+SNDRV_PCM_RATE_192000,
Indentation again.
Will sort it out in next version.
+static int q6afe_of_xlate_dai_name(struct snd_soc_component *component,
+ struct of_phandle_args *args,
+ const char **dai_name)
+{
+ int id = args->args[0];
+ int i, ret = -EINVAL;
Coding style, don't mix initialization in with other variable
declarations on the same line like this.
Will fix all such instances in next version.
+int q6afe_dai_dev_remove(struct device *dev)
+{
+ return 0;
+}
Remove empty functions, if they can't be removed it's probably not OK
for them to be empty either.
Sure will do that.
thanks,
srini
Re: [PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai driver
On Tue, Feb 13, 2018 at 04:58:26PM +, srinivas.kandaga...@linaro.org wrote:
> +static int q6hdmi_format_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct q6afe_dai_data *dai_data = kcontrol->private_data;
> + int value = ucontrol->value.integer.value[0];
> +
> + dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value;
> +
> + return 0;
> +}
No validation, and do we not need to tell a currently running stream if
the format changed on it (or block such changes if they're not going to
work, which seems more likely)?
> +static int q6hdmi_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
> + int channels = params_channels(params);
> + struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi;
> +
> + hdmi->sample_rate = params_rate(params);
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_S16_LE:
> + hdmi->bit_width = 16;
> + break;
> + case SNDRV_PCM_FORMAT_S24_LE:
> + hdmi->bit_width = 24;
> + break;
> + }
This silently accepts invalid values.
> + /*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/
Coding style, spaces around the /* */.
> +static int q6afe_dai_startup(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
> +
> + dai_data->is_port_started[dai->id] = false;
> +
> + return 0;
> +}
If this is needed it makes me a bit worried that we've got some kind of
bug with not shutting things down properly somewhere - what's going on
here?
> +static void q6afe_dai_shutdown(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
> + int rc;
> +
> + rc = q6afe_port_stop(dai_data->port[dai->id]);
> + if (rc < 0)
> + dev_err(dai->dev, "fail to close AFE port\n");
Better to print the error code so users have more information to debug
the problem.
> + .stream_name = "HDMI Playback",
> + .rates = SNDRV_PCM_RATE_48000 |
> + SNDRV_PCM_RATE_96000 |
> + SNDRV_PCM_RATE_192000,
Indentation again.
> +static int q6afe_of_xlate_dai_name(struct snd_soc_component *component,
> +struct of_phandle_args *args,
> +const char **dai_name)
> +{
> + int id = args->args[0];
> + int i, ret = -EINVAL;
Coding style, don't mix initialization in with other variable
declarations on the same line like this.
> +int q6afe_dai_dev_remove(struct device *dev)
> +{
> + return 0;
> +}
Remove empty functions, if they can't be removed it's probably not OK
for them to be empty either.
signature.asc
Description: PGP signature
Re: [alsa-devel] [PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai driver
Thanks for testing this out, On 19/02/18 10:32, Rohit Kumar wrote: diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile index 660afcab98fd..c7833842b878 100644 --- a/sound/soc/qcom/qdsp6/Makefile +++ b/sound/soc/qcom/qdsp6/Makefile @@ -1,5 +1,5 @@ obj-$(CONFIG_SND_SOC_QDSP6_COMMON) += q6dsp-common.o -obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o +obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o q6afe-dai.o depmod: ERROR: Cycle detected: q6afe -> q6afe_dai -> q6afe need to use like: +qdsp6_afe-objs := q6afe.o q6afe-dai.o +obj-$(CONFIG_SND_SOC_QDSP6_AFE) += qdsp6_afe.o similarly for asm and adm Yep, will fix this in next version, thanks, srini obj-$(CONFIG_SND_SOC_QDSP6_ADM) += q6adm.o q6routing.o obj-$(CONFIG_SND_SOC_QDSP6_ASM) += q6asm.o obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c b/sound/soc/qcom/qdsp6/q6afe-dai.c new file mode 100644 index ..f6a618e9db9e
Re: [alsa-devel] [PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai driver
On 2/13/2018 10:28 PM, srinivas.kandaga...@linaro.org wrote:
From: Srinivas Kandagatla
This patch adds support to q6afe backend dais driver.
Signed-off-by: Srinivas Kandagatla
---
sound/soc/qcom/qdsp6/Makefile| 2 +-
sound/soc/qcom/qdsp6/q6afe-dai.c | 280 +++
sound/soc/qcom/qdsp6/q6afe.h | 3 +
3 files changed, 284 insertions(+), 1 deletion(-)
create mode 100644 sound/soc/qcom/qdsp6/q6afe-dai.c
diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile
index 660afcab98fd..c7833842b878 100644
--- a/sound/soc/qcom/qdsp6/Makefile
+++ b/sound/soc/qcom/qdsp6/Makefile
@@ -1,5 +1,5 @@
obj-$(CONFIG_SND_SOC_QDSP6_COMMON) += q6dsp-common.o
-obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o
+obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o q6afe-dai.o
depmod: ERROR: Cycle detected: q6afe -> q6afe_dai -> q6afe
need to use like:
+qdsp6_afe-objs := q6afe.o q6afe-dai.o
+obj-$(CONFIG_SND_SOC_QDSP6_AFE) += qdsp6_afe.o
similarly for asm and adm
obj-$(CONFIG_SND_SOC_QDSP6_ADM) += q6adm.o q6routing.o
obj-$(CONFIG_SND_SOC_QDSP6_ASM) += q6asm.o
obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o
diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c b/sound/soc/qcom/qdsp6/q6afe-dai.c
new file mode 100644
index ..f6a618e9db9e
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/q6afe-dai.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2011-2016, The Linux Foundation
+ * Copyright (c) 2018, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "q6afe.h"
+
+struct q6afe_dai_data {
+ struct q6afe_port *port[AFE_PORT_MAX];
+ struct q6afe_port_config port_config[AFE_PORT_MAX];
+ bool is_port_started[AFE_PORT_MAX];
+};
+
+static int q6hdmi_format_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct q6afe_dai_data *dai_data = kcontrol->private_data;
+ int value = ucontrol->value.integer.value[0];
+
+ dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value;
+
+ return 0;
+}
+
+static int q6hdmi_format_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+
+ struct q6afe_dai_data *dai_data = kcontrol->private_data;
+
+ ucontrol->value.integer.value[0] =
+ dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype;
+
+ return 0;
+}
+
+static const char * const hdmi_format[] = {
+ "LPCM",
+ "Compr"
+};
+
+static const struct soc_enum hdmi_config_enum[] = {
+ SOC_ENUM_SINGLE_EXT(2, hdmi_format),
+};
+
+static const struct snd_kcontrol_new q6afe_config_controls[] = {
+ SOC_ENUM_EXT("HDMI RX Format", hdmi_config_enum[0],
+q6hdmi_format_get,
+q6hdmi_format_put),
+};
+
+static int q6hdmi_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+ int channels = params_channels(params);
+ struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi;
+
+ hdmi->sample_rate = params_rate(params);
+ switch (params_format(params)) {
+ case SNDRV_PCM_FORMAT_S16_LE:
+ hdmi->bit_width = 16;
+ break;
+ case SNDRV_PCM_FORMAT_S24_LE:
+ hdmi->bit_width = 24;
+ break;
+ }
+
+ /*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/
+ switch (channels) {
+ case 2:
+ hdmi->channel_allocation = 0;
+ break;
+ case 3:
+ hdmi->channel_allocation = 0x02;
+ break;
+ case 4:
+ hdmi->channel_allocation = 0x06;
+ break;
+ case 5:
+ hdmi->channel_allocation = 0x0A;
+ break;
+ case 6:
+ hdmi->channel_allocation = 0x0B;
+ break;
+ case 7:
+ hdmi->channel_allocation = 0x12;
+ break;
+ case 8:
+ hdmi->channel_allocation = 0x13;
+ break;
+ default:
+ dev_err(dai->dev, "invalid Channels = %u\n", channels);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int q6afe_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+
+ dai_data->is_port_started[dai->id] = false;
+
+ return 0;
+}
+
+static void q6afe_dai_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+ int rc;
+
+ rc
[PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai driver
From: Srinivas Kandagatla
This patch adds support to q6afe backend dais driver.
Signed-off-by: Srinivas Kandagatla
---
sound/soc/qcom/qdsp6/Makefile| 2 +-
sound/soc/qcom/qdsp6/q6afe-dai.c | 280 +++
sound/soc/qcom/qdsp6/q6afe.h | 3 +
3 files changed, 284 insertions(+), 1 deletion(-)
create mode 100644 sound/soc/qcom/qdsp6/q6afe-dai.c
diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile
index 660afcab98fd..c7833842b878 100644
--- a/sound/soc/qcom/qdsp6/Makefile
+++ b/sound/soc/qcom/qdsp6/Makefile
@@ -1,5 +1,5 @@
obj-$(CONFIG_SND_SOC_QDSP6_COMMON) += q6dsp-common.o
-obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o
+obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o q6afe-dai.o
obj-$(CONFIG_SND_SOC_QDSP6_ADM) += q6adm.o q6routing.o
obj-$(CONFIG_SND_SOC_QDSP6_ASM) += q6asm.o
obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o
diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c b/sound/soc/qcom/qdsp6/q6afe-dai.c
new file mode 100644
index ..f6a618e9db9e
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/q6afe-dai.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2011-2016, The Linux Foundation
+ * Copyright (c) 2018, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "q6afe.h"
+
+struct q6afe_dai_data {
+ struct q6afe_port *port[AFE_PORT_MAX];
+ struct q6afe_port_config port_config[AFE_PORT_MAX];
+ bool is_port_started[AFE_PORT_MAX];
+};
+
+static int q6hdmi_format_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct q6afe_dai_data *dai_data = kcontrol->private_data;
+ int value = ucontrol->value.integer.value[0];
+
+ dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value;
+
+ return 0;
+}
+
+static int q6hdmi_format_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+
+ struct q6afe_dai_data *dai_data = kcontrol->private_data;
+
+ ucontrol->value.integer.value[0] =
+ dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype;
+
+ return 0;
+}
+
+static const char * const hdmi_format[] = {
+ "LPCM",
+ "Compr"
+};
+
+static const struct soc_enum hdmi_config_enum[] = {
+ SOC_ENUM_SINGLE_EXT(2, hdmi_format),
+};
+
+static const struct snd_kcontrol_new q6afe_config_controls[] = {
+ SOC_ENUM_EXT("HDMI RX Format", hdmi_config_enum[0],
+q6hdmi_format_get,
+q6hdmi_format_put),
+};
+
+static int q6hdmi_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+ int channels = params_channels(params);
+ struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi;
+
+ hdmi->sample_rate = params_rate(params);
+ switch (params_format(params)) {
+ case SNDRV_PCM_FORMAT_S16_LE:
+ hdmi->bit_width = 16;
+ break;
+ case SNDRV_PCM_FORMAT_S24_LE:
+ hdmi->bit_width = 24;
+ break;
+ }
+
+ /*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/
+ switch (channels) {
+ case 2:
+ hdmi->channel_allocation = 0;
+ break;
+ case 3:
+ hdmi->channel_allocation = 0x02;
+ break;
+ case 4:
+ hdmi->channel_allocation = 0x06;
+ break;
+ case 5:
+ hdmi->channel_allocation = 0x0A;
+ break;
+ case 6:
+ hdmi->channel_allocation = 0x0B;
+ break;
+ case 7:
+ hdmi->channel_allocation = 0x12;
+ break;
+ case 8:
+ hdmi->channel_allocation = 0x13;
+ break;
+ default:
+ dev_err(dai->dev, "invalid Channels = %u\n", channels);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int q6afe_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+
+ dai_data->is_port_started[dai->id] = false;
+
+ return 0;
+}
+
+static void q6afe_dai_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
+ int rc;
+
+ rc = q6afe_port_stop(dai_data->port[dai->id]);
+ if (rc < 0)
+ dev_err(dai->dev, "fail to close AFE port\n");
+
+ dai_data->is_port_started[dai->id] = false;
+
+}
+
+static int q6afe_dai_prepare(struct snd_pcm_substream *substream,
+
