diff mbox series

[v2,8/8] ASoC: qcom: apq8096: add kcontrols to set PCM rate

Message ID 20200209154748.3015-9-adam@serbinski.com
State New, archived
Headers show
Series ASoC: qdsp6: db820c: Add support for external and bluetooth audio | expand

Commit Message

Adam Serbinski Feb. 9, 2020, 3:47 p.m. UTC
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.

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(-)

Comments

Mark Brown Feb. 10, 2020, 1:36 p.m. UTC | #1
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.
Adam Serbinski Feb. 10, 2020, 3:45 p.m. UTC | #2
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.
Jaroslav Kysela Feb. 10, 2020, 4:18 p.m. UTC | #3
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:
>
Adam Serbinski Feb. 10, 2020, 5:13 p.m. UTC | #4
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.
Mark Brown Feb. 10, 2020, 6:26 p.m. UTC | #5
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.
Adam Serbinski Feb. 10, 2020, 8 p.m. UTC | #6
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?
Mark Brown Feb. 10, 2020, 8:08 p.m. UTC | #7
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).
Adam Serbinski Feb. 10, 2020, 9:13 p.m. UTC | #8
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.
Mark Brown Feb. 11, 2020, 11:42 a.m. UTC | #9
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.
Adam Serbinski Feb. 11, 2020, 1:52 p.m. UTC | #10
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.
diff mbox series

Patch

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: