| Message ID | 20200209154748.3015-1-adam@serbinski.com |
|---|---|
| Headers | show |
| Series | ASoC: qdsp6: db820c: Add support for external and bluetooth audio | expand |
On Sun, Feb 09, 2020 at 10:47:40AM -0500, Adam Serbinski wrote: > Changes from V1: > > Rename patch: > from: dts: msm8996/db820c: enable primary pcm and quaternary i2s Please don't send new serieses in reply to old ones, it can make it confusing what's going on and what the current version is.
On Sun, Feb 09, 2020 at 10:47:42AM -0500, Adam Serbinski wrote: > > +#define AFE_API_VERSION_PCM_CONFIG 0x1 > +/* Enumeration for the auxiliary PCM synchronization signal > + * provided by an external source. > + */ > + > +#define AFE_PORT_PCM_SYNC_SRC_EXTERNAL 0x0 > +/* Enumeration for the auxiliary PCM synchronization signal > + * provided by an internal source. > + */ This is a *weird* commenting style for these #defines and it's not consistent within the block, I'm seeing at least 3 different styles. > +/* Payload of the #AFE_PARAM_ID_PCM_CONFIG command's > + * (PCM configuration parameter). > + */ > + > +struct afe_param_id_pcm_cfg { Similar weird commenting here, please follow coding-style.rst. > + switch (cfg->fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBS_CFS: > + pcfg->pcm_cfg.sync_src = AFE_PORT_PCM_SYNC_SRC_INTERNAL; > + break; > + case SND_SOC_DAIFMT_CBM_CFM: > + /* CPU is slave */ > + pcfg->pcm_cfg.sync_src = AFE_PORT_PCM_SYNC_SRC_EXTERNAL; > + break; > + default: > + break; > + } Why is this not returning an error on unsupported values? > + > + switch (cfg->sample_rate) { > + case 8000: > + pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_128; > + break; > + case 16000: > + pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_64; > + break; > + } Same here.
On Sun, Feb 09, 2020 at 10:47:43AM -0500, Adam Serbinski wrote: > +static int q6pcm_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + struct q6afe_dai_data *dai_data = dev_get_drvdata(dai->dev); > + struct q6afe_pcm_cfg *pcm = &dai_data->port_config[dai->id].pcm_cfg; > + > + pcm->sample_rate = params_rate(params); > + This and set_fmt() don't do any validation of the value being set. > static const struct snd_soc_dai_ops q6tdm_ops = { > .prepare = q6afe_dai_prepare, > .shutdown = q6afe_dai_shutdown, > - .set_sysclk = q6afe_mi2s_set_sysclk, > + .set_sysclk = q6afe_tdm_set_sysclk, > .set_tdm_slot = q6tdm_set_tdm_slot, > .set_channel_map = q6tdm_set_channel_map, > .hw_params = q6tdm_hw_params, This looks like a separate bug fix that should be split out? > + }, { > + .playback = { > + .stream_name = "Primary PCM Playback", > + .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S24_LE, > + .rate_min = 8000, > + .rate_max = 16000, > + }, It is surprising to see rate_min and rate_max specified when we're not using _KNOT, and again there's weird formatting here with the tabs before the rate values.
On Sun, Feb 09, 2020 at 10:47:46AM -0500, Adam Serbinski wrote: > When not specifying a codec, use snd-soc-dummy-dai. This supports > the case where a fixed configuration codec is attached, such as > bluetooth hfp. Fixed configuration devices should still have normal drivers that say what those fixed configurations are.
On Sun, Feb 09, 2020 at 10:47:48AM -0500, Adam Serbinski wrote: > This makes it possible for the backend sample rate to be > set to 8000 or 16000 Hz, depending on the needs of the HFP > call being set up. This would seem like an excellent thing to put in the driver for the baseband or bluetooth.
On 2020-02-10 07:17, Mark Brown wrote: > On Sun, Feb 09, 2020 at 10:47:40AM -0500, Adam Serbinski wrote: >> Changes from V1: >> >> Rename patch: >> from: dts: msm8996/db820c: enable primary pcm and quaternary i2s > > Please don't send new serieses in reply to old ones, it can make it > confusing what's going on and what the current version is. My apologies. Its my first time doing this. Thank you for the advice. -Adam
On 2020-02-10 08:31, Mark Brown wrote: > On Sun, Feb 09, 2020 at 10:47:42AM -0500, Adam Serbinski wrote: > >> >> +#define AFE_API_VERSION_PCM_CONFIG 0x1 >> +/* Enumeration for the auxiliary PCM synchronization signal >> + * provided by an external source. >> + */ >> + >> +#define AFE_PORT_PCM_SYNC_SRC_EXTERNAL 0x0 >> +/* Enumeration for the auxiliary PCM synchronization signal >> + * provided by an internal source. >> + */ > > This is a *weird* commenting style for these #defines and it's not > consistent within the block, I'm seeing at least 3 different styles. I will clean up the commenting. >> + default: >> + break; >> + } > > Why is this not returning an error on unsupported values? Only to be consistent with the pre-existing implementation for i2s ports. I will add an error return. > >> + >> + switch (cfg->sample_rate) { >> + case 8000: >> + pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_128; >> + break; >> + case 16000: >> + pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_64; >> + break; >> + } > > Same here. I will also add the error return here.
On 2020-02-10 08:36, Mark Brown wrote: > On Sun, Feb 09, 2020 at 10:47:48AM -0500, Adam Serbinski wrote: >> This makes it possible for the backend sample rate to be >> set to 8000 or 16000 Hz, depending on the needs of the HFP >> call being set up. > > This would seem like an excellent thing to put in the driver for the > baseband or bluetooth. The value that must be set to this control is not available to the bluetooth driver. It originates from the bluetooth stack in userspace, typically either blueZ or fluoride, as a result of a negotiation between the two devices participating in the HFP call.
Dne 09. 02. 20 v 16:47 Adam Serbinski napsal(a): > This makes it possible for the backend sample rate to be > set to 8000 or 16000 Hz, depending on the needs of the HFP > call being set up. Two points: Why enum? It adds just more code than the integer value handlers. Also, this belongs to the PCM interface, so it should be handled with SNDRV_CTL_ELEM_IFACE_PCM not mixer. The name should be probably "Rate" and assigned to the corresponding PCM device. Add this to Documentation/sound/designs/control-names.rst . Jaroslav > > Signed-off-by: Adam Serbinski <adam@serbinski.com> > CC: Andy Gross <agross@kernel.org> > CC: Mark Rutland <mark.rutland@arm.com> > CC: Liam Girdwood <lgirdwood@gmail.com> > CC: Patrick Lai <plai@codeaurora.org> > CC: Banajit Goswami <bgoswami@codeaurora.org> > CC: Jaroslav Kysela <perex@perex.cz> > CC: Takashi Iwai <tiwai@suse.com> > CC: alsa-devel@alsa-project.org > CC: linux-arm-msm@vger.kernel.org > CC: devicetree@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > sound/soc/qcom/apq8096.c | 92 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 90 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/qcom/apq8096.c b/sound/soc/qcom/apq8096.c > index 1edcaa15234f..882f2c456321 100644 > --- a/sound/soc/qcom/apq8096.c > +++ b/sound/soc/qcom/apq8096.c > @@ -16,6 +16,9 @@ > #define MI2S_BCLK_RATE 1536000 > #define PCM_BCLK_RATE 1024000 > > +static int pri_pcm_sample_rate = 16000; > +static int quat_pcm_sample_rate = 16000; > + > static int msm_snd_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params) > { > @@ -33,10 +36,15 @@ static int msm_snd_hw_params(struct snd_pcm_substream *substream, > switch (cpu_dai->id) { > case PRIMARY_PCM_RX: > case PRIMARY_PCM_TX: > + rate->min = pri_pcm_sample_rate; > + rate->max = pri_pcm_sample_rate; > + channels->min = 1; > + channels->max = 1; > + break; > case QUATERNARY_PCM_RX: > case QUATERNARY_PCM_TX: > - rate->min = 16000; > - rate->max = 16000; > + rate->min = quat_pcm_sample_rate; > + rate->max = quat_pcm_sample_rate; > channels->min = 1; > channels->max = 1; > break; > @@ -121,6 +129,83 @@ static struct snd_soc_ops apq8096_ops = { > .startup = msm_snd_startup, > }; > > +static char const *pcm_sample_rate_text[] = {"8 kHz", "16 kHz"}; > +static const struct soc_enum pcm_snd_enum = > + SOC_ENUM_SINGLE_EXT(2, pcm_sample_rate_text); > + > +static int get_sample_rate_idx(int sample_rate) > +{ > + int sample_rate_idx = 0; > + > + switch (sample_rate) { > + case 8000: > + sample_rate_idx = 0; > + break; > + case 16000: > + default: > + sample_rate_idx = 1; > + break; > + } > + > + return sample_rate_idx; > +} > + > +static int pri_pcm_sample_rate_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + ucontrol->value.integer.value[0] = > + get_sample_rate_idx(pri_pcm_sample_rate); > + return 0; > +} > + > +static int quat_pcm_sample_rate_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + ucontrol->value.integer.value[0] = > + get_sample_rate_idx(quat_pcm_sample_rate); > + return 0; > +} > + > +static int get_sample_rate(int idx) > +{ > + int sample_rate_val = 0; > + > + switch (idx) { > + case 0: > + sample_rate_val = 8000; > + break; > + case 1: > + default: > + sample_rate_val = 16000; > + break; > + } > + > + return sample_rate_val; > +} > + > +static int pri_pcm_sample_rate_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + pri_pcm_sample_rate = > + get_sample_rate(ucontrol->value.integer.value[0]); > + return 0; > +} > + > +static int quat_pcm_sample_rate_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + quat_pcm_sample_rate = > + get_sample_rate(ucontrol->value.integer.value[0]); > + return 0; > +} > + > +static const struct snd_kcontrol_new card_controls[] = { > + SOC_ENUM_EXT("PRI_PCM SampleRate", pcm_snd_enum, > + pri_pcm_sample_rate_get, pri_pcm_sample_rate_put), > + SOC_ENUM_EXT("QUAT_PCM SampleRate", pcm_snd_enum, > + quat_pcm_sample_rate_get, quat_pcm_sample_rate_put), > +}; > + > static int apq8096_init(struct snd_soc_pcm_runtime *rtd) > { > struct snd_soc_dai *codec_dai = rtd->codec_dai; > @@ -182,6 +267,9 @@ static int apq8096_platform_probe(struct platform_device *pdev) > if (ret) > goto err_card_register; > > + snd_soc_add_card_controls(card, card_controls, > + ARRAY_SIZE(card_controls)); > + > return 0; > > err_card_register: >
Few minor comments On 09/02/2020 15:47, Adam Serbinski wrote: > This patch adds support of AFE DAI for PCM port. > > Signed-off-by: Adam Serbinski <adam@serbinski.com> > CC: Andy Gross <agross@kernel.org> > CC: Mark Rutland <mark.rutland@arm.com> > CC: Liam Girdwood <lgirdwood@gmail.com> > CC: Patrick Lai <plai@codeaurora.org> > CC: Banajit Goswami <bgoswami@codeaurora.org> > CC: Jaroslav Kysela <perex@perex.cz> > CC: Takashi Iwai <tiwai@suse.com> > CC: alsa-devel@alsa-project.org > CC: linux-arm-msm@vger.kernel.org > CC: devicetree@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > sound/soc/qcom/qdsp6/q6afe-dai.c | 198 ++++++++++++++++++++++++++++++- > 1 file changed, 197 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c b/sound/soc/qcom/qdsp6/q6afe-dai.c > index c1a7624eaf17..23b29591ef47 100644 > --- a/sound/soc/qcom/qdsp6/q6afe-dai.c > +++ b/sound/soc/qcom/qdsp6/q6afe-dai.c ... > +static int q6afe_tdm_set_sysclk(struct snd_soc_dai *dai, > + int clk_id, unsigned int freq, int dir) > +{ Why are we adding exactly duplicate function of q6afe_mi2s_set_sysclk here? > + struct q6afe_dai_data *dai_data = dev_get_drvdata(dai->dev); > + struct q6afe_port *port = dai_data->port[dai->id]; > + > + switch (clk_id) { > + case LPAIF_DIG_CLK: > + return q6afe_port_set_sysclk(port, clk_id, 0, 5, freq, dir); > + case LPAIF_BIT_CLK: > + case LPAIF_OSR_CLK: > + return q6afe_port_set_sysclk(port, clk_id, > + Q6AFE_LPASS_CLK_SRC_INTERNAL, > + Q6AFE_LPASS_CLK_ROOT_DEFAULT, > + freq, dir); > case Q6AFE_LPASS_CLK_ID_PRI_TDM_IBIT ... Q6AFE_LPASS_CLK_ID_QUIN_TDM_EBIT: > return q6afe_port_set_sysclk(port, clk_id, > Q6AFE_LPASS_CLK_ATTRIBUTE_INVERT_COUPLE_NO, > @@ -468,6 +520,11 @@ static const struct snd_soc_dapm_route q6afe_dapm_routes[] = { > {"Tertiary MI2S Playback", NULL, "TERT_MI2S_RX"}, > {"Quaternary MI2S Playback", NULL, "QUAT_MI2S_RX"}, > > + {"Primary PCM Playback", NULL, "PRI_PCM_RX"}, > + {"Secondary PCM Playback", NULL, "SEC_PCM_RX"}, > + {"Tertiary PCM Playback", NULL, "TERT_PCM_RX"}, > + {"Quaternary PCM Playback", NULL, "QUAT_PCM_RX"}, > + > {"Primary TDM0 Playback", NULL, "PRIMARY_TDM_RX_0"}, > {"Primary TDM1 Playback", NULL, "PRIMARY_TDM_RX_1"}, > {"Primary TDM2 Playback", NULL, "PRIMARY_TDM_RX_2"}, > @@ -562,6 +619,11 @@ static const struct snd_soc_dapm_route q6afe_dapm_routes[] = { > {"PRI_MI2S_TX", NULL, "Primary MI2S Capture"}, > {"SEC_MI2S_TX", NULL, "Secondary MI2S Capture"}, > {"QUAT_MI2S_TX", NULL, "Quaternary MI2S Capture"}, > + > + {"PRI_PCM_TX", NULL, "Primary PCM Capture"}, > + {"SEC_PCM_TX", NULL, "Secondary PCM Capture"}, > + {"TERT_PCM_TX", NULL, "Tertiary PCM Capture"}, > + {"QUAT_PCM_TX", NULL, "Quaternary PCM Capture"}, > }; > ... > > + SND_SOC_DAPM_AIF_IN("QUAT_PCM_RX", NULL, > + 0, 0, 0, 0), This can be in single line, same for below > + SND_SOC_DAPM_AIF_OUT("QUAT_PCM_TX", NULL, > + 0, 0, 0, 0), > + SND_SOC_DAPM_AIF_IN("TERT_PCM_RX", NULL, > + 0, 0, 0, 0), > + SND_SOC_DAPM_AIF_OUT("TERT_PCM_TX", NULL, > + 0, 0, 0, 0), > + SND_SOC_DAPM_AIF_IN("SEC_PCM_RX", NULL, > + 0, 0, 0, 0), > + SND_SOC_DAPM_AIF_OUT("SEC_PCM_TX", NULL, > + 0, 0, 0, 0), > + SND_SOC_DAPM_AIF_IN("PRI_PCM_RX", NULL, > + 0, 0, 0, 0), > + SND_SOC_DAPM_AIF_OUT("PRI_PCM_TX", NULL, > + 0, 0, 0, 0), > +
On 09/02/2020 15:47, Adam Serbinski wrote: > This patch adds support to pcm ports in AFE. > > Signed-off-by: Adam Serbinski <adam@serbinski.com> > CC: Andy Gross <agross@kernel.org> > CC: Mark Rutland <mark.rutland@arm.com> > CC: Liam Girdwood <lgirdwood@gmail.com> > CC: Patrick Lai <plai@codeaurora.org> > CC: Banajit Goswami <bgoswami@codeaurora.org> > CC: Jaroslav Kysela <perex@perex.cz> > CC: Takashi Iwai <tiwai@suse.com> > CC: alsa-devel@alsa-project.org > CC: linux-arm-msm@vger.kernel.org > CC: devicetree@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > sound/soc/qcom/qdsp6/q6afe.c | 246 +++++++++++++++++++++++++++++++++++ > sound/soc/qcom/qdsp6/q6afe.h | 9 +- > 2 files changed, 254 insertions(+), 1 deletion(-) > Few general comments. 1>documentation to "struct afe_param_id_pcm_cfg " Either we follow kerneldoc style or not add this as we did with other similar afe port config structures. Am okay either way! 2> some of the defines in this patch has no reals users, so we better remove all the unused constants. > diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c > index e0945f7a58c8..b53ad14a78fd 100644 > --- a/sound/soc/qcom/qdsp6/q6afe.c > +++ b/sound/soc/qcom/qdsp6/q6afe.c > @@ -40,6 +40,7 @@ > ... > +/** > + * q6afe_pcm_port_prepare() - Prepare pcm afe port. > + * > + * @port: Instance of afe port > + * @cfg: PCM configuration for the afe port > + * > + */ > +int q6afe_pcm_port_prepare(struct q6afe_port *port, struct q6afe_pcm_cfg *cfg) > +{ > + union afe_port_config *pcfg = &port->port_cfg; > + > + pcfg->pcm_cfg.pcm_cfg_minor_version = AFE_API_VERSION_PCM_CONFIG; > + pcfg->pcm_cfg.aux_mode = AFE_PORT_PCM_AUX_MODE_PCM; > + > + switch (cfg->fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBS_CFS: > + pcfg->pcm_cfg.sync_src = AFE_PORT_PCM_SYNC_SRC_INTERNAL; > + break; > + case SND_SOC_DAIFMT_CBM_CFM: > + /* CPU is slave */ > + pcfg->pcm_cfg.sync_src = AFE_PORT_PCM_SYNC_SRC_EXTERNAL; > + break; > + default: > + break; > + } > + > + switch (cfg->sample_rate) { > + case 8000: > + pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_128; > + break; > + case 16000: > + pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_64; > + break; > + } > + pcfg->pcm_cfg.quantype = AFE_PORT_PCM_LINEAR_NOPADDING; > + pcfg->pcm_cfg.ctrl_data_out_enable = AFE_PORT_PCM_CTRL_DATA_OE_DISABLE; > + pcfg->pcm_cfg.reserved = 0; > + pcfg->pcm_cfg.sample_rate = cfg->sample_rate; > + > + /* 16 bit mono */ > + pcfg->pcm_cfg.bit_width = 16; > + pcfg->pcm_cfg.num_channels = 1; > + pcfg->pcm_cfg.slot_number_mapping[0] = 1; PCM quantization type and Slot Mapping should come from device tree. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(q6afe_pcm_port_prepare); > +
On 2020-02-10 11:18, Jaroslav Kysela wrote: > Dne 09. 02. 20 v 16:47 Adam Serbinski napsal(a): >> This makes it possible for the backend sample rate to be >> set to 8000 or 16000 Hz, depending on the needs of the HFP >> call being set up. > > Two points: > > Why enum? It adds just more code than the integer value handlers. Because enum allows the potential values to be restricted to a set of distinct values rather than a range. And while yes, I understand that the value can be validated, or the step can in this case be set to correspond to the difference between the current 2 values, this approach would neither make it clear to the user what the permitted values are, nor would it scale well once additional values are required. > Also, this belongs to the PCM interface, so it should be handled with > SNDRV_CTL_ELEM_IFACE_PCM not mixer. > > The name should be probably "Rate" and assigned to the corresponding > PCM device. > > Add this to Documentation/sound/designs/control-names.rst . Above 3 lines are noted, I will make these changed.
On 09/02/2020 15:47, Adam Serbinski wrote: > This patch adds support to PCM_PORT mixers required to > select path between ASM stream and AFE ports. > > Signed-off-by: Adam Serbinski <adam@serbinski.com> > CC: Andy Gross <agross@kernel.org> > CC: Mark Rutland <mark.rutland@arm.com> > CC: Liam Girdwood <lgirdwood@gmail.com> > CC: Patrick Lai <plai@codeaurora.org> > CC: Banajit Goswami <bgoswami@codeaurora.org> > CC: Jaroslav Kysela <perex@perex.cz> > CC: Takashi Iwai <tiwai@suse.com> > CC: alsa-devel@alsa-project.org > CC: linux-arm-msm@vger.kernel.org > CC: devicetree@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > sound/soc/qcom/qdsp6/q6routing.c | 44 ++++++++++++++++++++++++++++++++ Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > 1 file changed, 44 insertions(+) > > diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c > index 20724102e85a..3a81d2161707 100644 > --- a/sound/soc/qcom/qdsp6/q6routing.c > +++ b/sound/soc/qcom/qdsp6/q6routing.c > @@ -67,6 +67,10 @@ > { mix_name, "SEC_MI2S_TX", "SEC_MI2S_TX" }, \ > { mix_name, "QUAT_MI2S_TX", "QUAT_MI2S_TX" }, \ > { mix_name, "TERT_MI2S_TX", "TERT_MI2S_TX" }, \ > + { mix_name, "PRI_PCM_TX", "PRI_PCM_TX" }, \ > + { mix_name, "SEC_PCM_TX", "SEC_PCM_TX" }, \ > + { mix_name, "TERT_PCM_TX", "TERT_PCM_TX" }, \ > + { mix_name, "QUAT_PCM_TX", "QUAT_PCM_TX" }, \ > { mix_name, "SLIMBUS_0_TX", "SLIMBUS_0_TX" }, \ > { mix_name, "SLIMBUS_1_TX", "SLIMBUS_1_TX" }, \ > { mix_name, "SLIMBUS_2_TX", "SLIMBUS_2_TX" }, \ > @@ -128,6 +132,18 @@ > SOC_SINGLE_EXT("QUAT_MI2S_TX", QUATERNARY_MI2S_TX, \ > id, 1, 0, msm_routing_get_audio_mixer, \ > msm_routing_put_audio_mixer), \ > + SOC_SINGLE_EXT("PRI_PCM_TX", PRIMARY_PCM_TX, \ > + id, 1, 0, msm_routing_get_audio_mixer, \ > + msm_routing_put_audio_mixer), \ > + SOC_SINGLE_EXT("SEC_PCM_TX", SECONDARY_PCM_TX, \ > + id, 1, 0, msm_routing_get_audio_mixer, \ > + msm_routing_put_audio_mixer), \ > + SOC_SINGLE_EXT("TERT_PCM_TX", TERTIARY_PCM_TX, \ > + id, 1, 0, msm_routing_get_audio_mixer, \ > + msm_routing_put_audio_mixer), \ > + SOC_SINGLE_EXT("QUAT_PCM_TX", QUATERNARY_PCM_TX, \ > + id, 1, 0, msm_routing_get_audio_mixer, \ > + msm_routing_put_audio_mixer), \ > SOC_SINGLE_EXT("SLIMBUS_0_TX", SLIMBUS_0_TX, \ > id, 1, 0, msm_routing_get_audio_mixer, \ > msm_routing_put_audio_mixer), \ > @@ -468,6 +484,18 @@ static const struct snd_kcontrol_new quaternary_mi2s_rx_mixer_controls[] = { > static const struct snd_kcontrol_new tertiary_mi2s_rx_mixer_controls[] = { > Q6ROUTING_RX_MIXERS(TERTIARY_MI2S_RX) }; > > +static const struct snd_kcontrol_new primary_pcm_rx_mixer_controls[] = { > + Q6ROUTING_RX_MIXERS(PRIMARY_PCM_RX) }; > + > +static const struct snd_kcontrol_new secondary_pcm_rx_mixer_controls[] = { > + Q6ROUTING_RX_MIXERS(SECONDARY_PCM_RX) }; > + > +static const struct snd_kcontrol_new tertiary_pcm_rx_mixer_controls[] = { > + Q6ROUTING_RX_MIXERS(TERTIARY_PCM_RX) }; > + > +static const struct snd_kcontrol_new quaternary_pcm_rx_mixer_controls[] = { > + Q6ROUTING_RX_MIXERS(QUATERNARY_PCM_RX) }; > + > static const struct snd_kcontrol_new slimbus_rx_mixer_controls[] = { > Q6ROUTING_RX_MIXERS(SLIMBUS_0_RX) }; > > @@ -695,6 +723,18 @@ static const struct snd_soc_dapm_widget msm_qdsp6_widgets[] = { > SND_SOC_DAPM_MIXER("TERT_MI2S_RX Audio Mixer", SND_SOC_NOPM, 0, 0, > tertiary_mi2s_rx_mixer_controls, > ARRAY_SIZE(tertiary_mi2s_rx_mixer_controls)), > + SND_SOC_DAPM_MIXER("PRI_PCM_RX Audio Mixer", SND_SOC_NOPM, 0, 0, > + primary_pcm_rx_mixer_controls, > + ARRAY_SIZE(primary_pcm_rx_mixer_controls)), > + SND_SOC_DAPM_MIXER("SEC_PCM_RX Audio Mixer", SND_SOC_NOPM, 0, 0, > + secondary_pcm_rx_mixer_controls, > + ARRAY_SIZE(secondary_pcm_rx_mixer_controls)), > + SND_SOC_DAPM_MIXER("TERT_PCM_RX Audio Mixer", SND_SOC_NOPM, 0, 0, > + tertiary_pcm_rx_mixer_controls, > + ARRAY_SIZE(tertiary_pcm_rx_mixer_controls)), > + SND_SOC_DAPM_MIXER("QUAT_PCM_RX Audio Mixer", SND_SOC_NOPM, 0, 0, > + quaternary_pcm_rx_mixer_controls, > + ARRAY_SIZE(quaternary_pcm_rx_mixer_controls)), > SND_SOC_DAPM_MIXER("PRIMARY_TDM_RX_0 Audio Mixer", SND_SOC_NOPM, 0, 0, > pri_tdm_rx_0_mixer_controls, > ARRAY_SIZE(pri_tdm_rx_0_mixer_controls)), > @@ -853,6 +893,10 @@ static const struct snd_soc_dapm_route intercon[] = { > Q6ROUTING_RX_DAPM_ROUTE("TERT_MI2S_RX Audio Mixer", "TERT_MI2S_RX"), > Q6ROUTING_RX_DAPM_ROUTE("SEC_MI2S_RX Audio Mixer", "SEC_MI2S_RX"), > Q6ROUTING_RX_DAPM_ROUTE("PRI_MI2S_RX Audio Mixer", "PRI_MI2S_RX"), > + Q6ROUTING_RX_DAPM_ROUTE("PRI_PCM_RX Audio Mixer", "PRI_PCM_RX"), > + Q6ROUTING_RX_DAPM_ROUTE("SEC_PCM_RX Audio Mixer", "SEC_PCM_RX"), > + Q6ROUTING_RX_DAPM_ROUTE("TERT_PCM_RX Audio Mixer", "TERT_PCM_RX"), > + Q6ROUTING_RX_DAPM_ROUTE("QUAT_PCM_RX Audio Mixer", "QUAT_PCM_RX"), > Q6ROUTING_RX_DAPM_ROUTE("PRIMARY_TDM_RX_0 Audio Mixer", > "PRIMARY_TDM_RX_0"), > Q6ROUTING_RX_DAPM_ROUTE("PRIMARY_TDM_RX_1 Audio Mixer", >
On 2020-02-10 12:13, Srinivas Kandagatla wrote: > Few minor comments > >> +static int q6afe_tdm_set_sysclk(struct snd_soc_dai *dai, >> + int clk_id, unsigned int freq, int dir) >> +{ > > Why are we adding exactly duplicate function of q6afe_mi2s_set_sysclk > here? It isn't an exact duplicate. The reason I split off the new function is because the clock IDs for PCM overlap/duplicate the clock IDs for TDM, yet the parameters to q6afe_port_set_sysclk are not the same for PCM and TDM. >> + SND_SOC_DAPM_AIF_IN("QUAT_PCM_RX", NULL, >> + 0, 0, 0, 0), > > This can be in single line, same for below I will adjust these.
On 10/02/2020 17:22, Adam Serbinski wrote: >>> >> >> Why are we adding exactly duplicate function of q6afe_mi2s_set_sysclk >> here? > > It isn't an exact duplicate. > > The reason I split off the new function is because the clock IDs for PCM > overlap/duplicate the clock IDs for TDM, yet the parameters to > q6afe_port_set_sysclk are not the same for PCM and TDM. > we should be able to use dai->id to make that decision. --srini > >>> + SND_SOC_DAPM_AIF_IN("QUAT_PCM_RX", NULL, >>> + 0, 0, 0, 0),
On Mon, Feb 10, 2020 at 10:45:16AM -0500, Adam Serbinski wrote: > On 2020-02-10 08:36, Mark Brown wrote: > > This would seem like an excellent thing to put in the driver for the > > baseband or bluetooth. > The value that must be set to this control is not available to the bluetooth > driver. It originates from the bluetooth stack in userspace, typically > either blueZ or fluoride, as a result of a negotiation between the two > devices participating in the HFP call. To repeat my comment on another patch in the series there should still be some representation of the DAI for this device in the kernel.
On 2020-02-10 13:26, Mark Brown wrote: > On Mon, Feb 10, 2020 at 10:45:16AM -0500, Adam Serbinski wrote: >> On 2020-02-10 08:36, Mark Brown wrote: > >> > This would seem like an excellent thing to put in the driver for the >> > baseband or bluetooth. > >> The value that must be set to this control is not available to the >> bluetooth >> driver. It originates from the bluetooth stack in userspace, typically >> either blueZ or fluoride, as a result of a negotiation between the two >> devices participating in the HFP call. > > To repeat my comment on another patch in the series there should still > be some representation of the DAI for this device in the kernel. Respectfully, I'm not sure I understand what it is that you are suggesting. Is it your intention to suggest that instead of adding controls to the machine driver, I should instead write a codec driver to contain those controls? Or is it your intention to suggest that something within the kernel is already aware of the rate to be set, and it is that which should set the rate rather than a control?
On Mon, Feb 10, 2020 at 03:00:55PM -0500, Adam Serbinski wrote: > On 2020-02-10 13:26, Mark Brown wrote: > > To repeat my comment on another patch in the series there should still > > be some representation of the DAI for this device in the kernel. > Respectfully, I'm not sure I understand what it is that you are suggesting. > Is it your intention to suggest that instead of adding controls to the > machine driver, I should instead write a codec driver to contain those > controls? I have already separately said that you should write a CODEC driver for this CODEC. I'm saying that this seems like the sort of thing that might fit in that CODEC driver. > Or is it your intention to suggest that something within the kernel is > already aware of the rate to be set, and it is that which should set the > rate rather than a control? That would be one example of how such a CODEC driver could be configured, and is how other baseband/BT devices have ended up going (see cx20442.c for example).
On 2020-02-10 15:08, Mark Brown wrote: > On Mon, Feb 10, 2020 at 03:00:55PM -0500, Adam Serbinski wrote: >> On 2020-02-10 13:26, Mark Brown wrote: > >> > To repeat my comment on another patch in the series there should still >> > be some representation of the DAI for this device in the kernel. > >> Respectfully, I'm not sure I understand what it is that you are >> suggesting. > >> Is it your intention to suggest that instead of adding controls to the >> machine driver, I should instead write a codec driver to contain those >> controls? > > I have already separately said that you should write a CODEC driver for > this CODEC. I'm saying that this seems like the sort of thing that > might fit in that CODEC driver. I see. My initial thought with respect to the codec driver would be just to use bt-sco.c, which is a dummy codec. I can certainly implement a new codec driver. >> Or is it your intention to suggest that something within the kernel is >> already aware of the rate to be set, and it is that which should set >> the >> rate rather than a control? > > That would be one example of how such a CODEC driver could be > configured, and is how other baseband/BT devices have ended up going > (see cx20442.c for example). I am not aware of how this could be done for bluetooth, since the value still has to originate from userspace. The driver you referred to supports only a single sample rate, whereas for bluetooth, 2 sample rates are required, and nothing in the kernel is aware of the appropriate rate, at least in the case of the qca6174a I'm working with right now, or for that matter, TI Wilink 8, which I've also worked with. My concern with implementing this in a new codec driver, is that this codec driver will be bound to qdsp6, since its purpose is to work around a characteristic of this DSP. Under simple-card, for instance, it would be redundant, since in that case, the parameters userspace uses to open the pcm will be propagated to the port. But under qdsp6, userspace could open the pcm at 44.1 kHz, yet the backend port is still set to 8 or 16 kHz, and the DSP resamples between them, so the sole purpose of this change is to allow userspace to deliver the required sample rate to the back end of qdsp6.
On Mon, Feb 10, 2020 at 04:13:52PM -0500, Adam Serbinski wrote: > I am not aware of how this could be done for bluetooth, since the value > still has to originate from userspace. The driver you referred to supports > only a single sample rate, whereas for bluetooth, 2 sample rates are > required, and nothing in the kernel is aware of the appropriate rate, at > least in the case of the qca6174a I'm working with right now, or for that > matter, TI Wilink 8, which I've also worked with. There's generic support in the CODEC<->CODEC link code for setting the DAI configuration from userspace.
On 2020-02-11 06:42, Mark Brown wrote: > On Mon, Feb 10, 2020 at 04:13:52PM -0500, Adam Serbinski wrote: > >> I am not aware of how this could be done for bluetooth, since the >> value >> still has to originate from userspace. The driver you referred to >> supports >> only a single sample rate, whereas for bluetooth, 2 sample rates are >> required, and nothing in the kernel is aware of the appropriate rate, >> at >> least in the case of the qca6174a I'm working with right now, or for >> that >> matter, TI Wilink 8, which I've also worked with. > > There's generic support in the CODEC<->CODEC link code for setting the > DAI configuration from userspace. Ok. Its going to take some time to get my head around that, so for the time being I'm going to drop this feature and get the rest fixed for inclusion. Thanks.
Changes from V1: Rename patch: from: dts: msm8996/db820c: enable primary pcm and quaternary i2s to: dts: qcom: db820c: Enable primary PCM and quaternary I2S CC: Andy Gross <agross@kernel.org> CC: Mark Rutland <mark.rutland@arm.com> CC: Liam Girdwood <lgirdwood@gmail.com> CC: Patrick Lai <plai@codeaurora.org> CC: Banajit Goswami <bgoswami@codeaurora.org> CC: Jaroslav Kysela <perex@perex.cz> CC: Takashi Iwai <tiwai@suse.com> CC: alsa-devel@alsa-project.org CC: linux-arm-msm@vger.kernel.org CC: devicetree@vger.kernel.org CC: linux-kernel@vger.kernel.org Adam Serbinski (8): ASoC: qdsp6: dt-bindings: Add q6afe pcm dt binding ASoC: qdsp6: q6afe: add support to pcm ports ASoC: qdsp6: q6afe-dai: add support to pcm port dais ASoC: qdsp6: q6routing: add pcm port routing ASoC: qcom: apq8096: add support for primary and quaternary I2S/PCM ASoC: qcom/common: Use snd-soc-dummy-dai when codec is not specified arm64: dts: qcom: db820c: Enable primary PCM and quaternary I2S ASoC: qcom: apq8096: add kcontrols to set PCM rate arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 113 +++++++++ arch/arm64/boot/dts/qcom/msm8996-pins.dtsi | 162 ++++++++++++ include/dt-bindings/sound/qcom,q6afe.h | 8 + sound/soc/qcom/apq8096.c | 172 +++++++++++-- sound/soc/qcom/common.c | 22 +- sound/soc/qcom/qdsp6/q6afe-dai.c | 198 ++++++++++++++- sound/soc/qcom/qdsp6/q6afe.c | 246 +++++++++++++++++++ sound/soc/qcom/qdsp6/q6afe.h | 9 +- sound/soc/qcom/qdsp6/q6routing.c | 44 ++++ 9 files changed, 953 insertions(+), 21 deletions(-)