diff mbox

[alsa-devel,RFC,00/14] ASoC: qcom: add support to apq8016 audio

Message ID 20150512040600.GA24087@kwestfie-linux.qualcomm.com (mailing list archive)
State RFC
Headers show

Commit Message

Kenneth Westfield May 12, 2015, 4:06 a.m. UTC
On Tue, May 05, 2015 at 11:54:16PM -0700, Srinivas Kandagatla wrote:
> Hi Kenneth,
> 
> On 06/05/15 06:47, Kenneth Westfield wrote:
> >>>
> >>>I will test the patches and let you know by Wednesday.  Also, I posted
> >>>some comments, but Patrick should be posting his comments separately
> >>>later next week.
> >Srinivas,
> >
> >After applying the patches, audio playback is no longer functional on
> >the storm board.  Give me some time to debug this and I will get back
> >to you.
> I found atleast one issue with rdma audif bits in ipq806x lpass
> 
> could you try change, this will be fixed properly in next version.
> 
> -----------------------><---------------------------------------------
> diff --git a/sound/soc/qcom/lpass-ipq806x.c
> b/sound/soc/qcom/lpass-ipq806x.c
> index 11a7053..2b00355 100644
> --- a/sound/soc/qcom/lpass-ipq806x.c
> +++ b/sound/soc/qcom/lpass-ipq806x.c
> @@ -69,6 +69,7 @@ struct lpass_variant ipq806x_data = {
>         .rdma_reg_base          = 0x6000,
>         .rdma_reg_stride        = 0x1000,
>         .rdma_channels          = 4,
> +       .rdmactl_audif_start    = 4,
>         .dai_driver             = &lpass_cpu_dai_driver,
>         .num_dai                = 1,
> 
> -----------------------><---------------------------------------------

Srinivas,

I was able to get audio working on the Storm board.  There were several
issues.  First, the I2S control port was saved in the DAI driver id field
(which was 4), but the DAI id field was used by the macro (which was 0).
The patch below fixed it:

-----------------------><---------------------------------------------
-----------------------><---------------------------------------------

Please incorporate these fixes into your next patch series.

Comments

Srinivas Kandagatla May 12, 2015, 10:21 a.m. UTC | #1
+ adding Lars

On 12/05/15 05:06, Kenneth Westfield wrote:
> Srinivas,
>
> I was able to get audio working on the Storm board.  There were several
> issues.  First, the I2S control port was saved in the DAI driver id field
> (which was 4), but the DAI id field was used by the macro (which was 0).
> The patch below fixed it:

Ah, I did not expect that to happen, dai->id and dai->driver->id should 
be same. Surprisingly this is not true for IPQ806x platform only because 
dais count == 1. Which results in dai->id not getting assigned to 
dai->driver->id due to below code snippet in  sound/soc/soc-core.c

--------------------><------------------------------------------
/*
  * Back in the old days when we still had component-less DAIs,
  * instead of having a static name, component-less DAIs would
  * inherit the name of the parent device so it is possible to
  * register multiple instances of the DAI. We still need to keep
  * the same naming style even though those DAIs are not
  * component-less anymore.
  */
if (count == 1 && legacy_dai_naming) {
	dai->name = fmt_single_name(dev, &dai->id);
} else {
	dai->name = fmt_multiple_name(dev, &dai_drv[i]);
	if (dai_drv[i].id)
		dai->id = dai_drv[i].id;
	else
		dai->id = i;
}
--------------------><------------------------------------------

Its not clear from code, why should we enter to legacy naming if the 
dais count == 1 even-though the dai_drv has has valid name and id 
information.

Mark/Lars,

I can workaround this by using dai->driver->id in the driver, But do you 
think that dai name and id should be assigned from dai drv if present?

--srini

>
> -----------------------><---------------------------------------------
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index 17ad20d..58ae8af 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -146,7 +146,7 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
>   	}
>
>   	ret = regmap_write(drvdata->lpaif_map,
> -			   LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +			   LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   			   regval);
>   	if (ret) {
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
> @@ -171,7 +171,7 @@ static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream *substream,
>   	int ret;
>
>   	ret = regmap_write(drvdata->lpaif_map,
> -			   LPAIF_I2SCTL_REG(drvdata->variant, dai->id), 0);
> +			   LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), 0);
>   	if (ret)
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
>   				__func__, ret);
> @@ -186,7 +186,7 @@ static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream,
>   	int ret;
>
>   	ret = regmap_update_bits(drvdata->lpaif_map,
> -			LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +			LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   			LPAIF_I2SCTL_SPKEN_MASK, LPAIF_I2SCTL_SPKEN_ENABLE);
>   	if (ret)
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
> @@ -206,7 +206,7 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>   	case SNDRV_PCM_TRIGGER_RESUME:
>   	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>   		ret = regmap_update_bits(drvdata->lpaif_map,
> -				LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +				LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   				LPAIF_I2SCTL_SPKEN_MASK,
>   				LPAIF_I2SCTL_SPKEN_ENABLE);
>   		if (ret)
> @@ -217,7 +217,7 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>   	case SNDRV_PCM_TRIGGER_SUSPEND:
>   	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>   		ret = regmap_update_bits(drvdata->lpaif_map,
> -				LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +				LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   				LPAIF_I2SCTL_SPKEN_MASK,
>   				LPAIF_I2SCTL_SPKEN_DISABLE);
>   		if (ret)
> @@ -247,7 +247,7 @@ int lpass_cpu_dai_probe(struct snd_soc_dai *dai)
>
>   	/* ensure audio hardware is disabled */
>   	ret = regmap_write(drvdata->lpaif_map,
> -			LPAIF_I2SCTL_REG(drvdata->variant, dai->id), 0);
> +			LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), 0);
>   	if (ret)
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
>   				__func__, ret);
> -----------------------><---------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla May 12, 2015, 1:11 p.m. UTC | #2
Hi Kenneth,

On 12/05/15 05:06, Kenneth Westfield wrote:
> On Tue, May 05, 2015 at 11:54:16PM -0700, Srinivas Kandagatla wrote:
>> Hi Kenneth,
>>
>> On 06/05/15 06:47, Kenneth Westfield wrote:
>>>>>
>>>>> I will test the patches and let you know by Wednesday.  Also, I posted
>>>>> some comments, but Patrick should be posting his comments separately
>>>>> later next week.
>>> Srinivas,
>>>
>>> After applying the patches, audio playback is no longer functional on
>>> the storm board.  Give me some time to debug this and I will get back
>>> to you.
>> I found atleast one issue with rdma audif bits in ipq806x lpass
>>
>> could you try change, this will be fixed properly in next version.
>>
>> -----------------------><---------------------------------------------
>> diff --git a/sound/soc/qcom/lpass-ipq806x.c
>> b/sound/soc/qcom/lpass-ipq806x.c
>> index 11a7053..2b00355 100644
>> --- a/sound/soc/qcom/lpass-ipq806x.c
>> +++ b/sound/soc/qcom/lpass-ipq806x.c
>> @@ -69,6 +69,7 @@ struct lpass_variant ipq806x_data = {
>>          .rdma_reg_base          = 0x6000,
>>          .rdma_reg_stride        = 0x1000,
>>          .rdma_channels          = 4,
>> +       .rdmactl_audif_start    = 4,
>>          .dai_driver             = &lpass_cpu_dai_driver,
>>          .num_dai                = 1,
>>
>> -----------------------><---------------------------------------------
>
> Srinivas,
>
> I was able to get audio working on the Storm board.  There were several
> issues.  First, the I2S control port was saved in the DAI driver id field
> (which was 4), but the DAI id field was used by the macro (which was 0).
> The patch below fixed it:
>
> -----------------------><---------------------------------------------
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index 17ad20d..58ae8af 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -146,7 +146,7 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
>   	}
>
>   	ret = regmap_write(drvdata->lpaif_map,
> -			   LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +			   LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   			   regval);
>   	if (ret) {
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
> @@ -171,7 +171,7 @@ static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream *substream,
>   	int ret;
>
>   	ret = regmap_write(drvdata->lpaif_map,
> -			   LPAIF_I2SCTL_REG(drvdata->variant, dai->id), 0);
> +			   LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), 0);
>   	if (ret)
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
>   				__func__, ret);
> @@ -186,7 +186,7 @@ static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream,
>   	int ret;
>
>   	ret = regmap_update_bits(drvdata->lpaif_map,
> -			LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +			LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   			LPAIF_I2SCTL_SPKEN_MASK, LPAIF_I2SCTL_SPKEN_ENABLE);
>   	if (ret)
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
> @@ -206,7 +206,7 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>   	case SNDRV_PCM_TRIGGER_RESUME:
>   	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>   		ret = regmap_update_bits(drvdata->lpaif_map,
> -				LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +				LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   				LPAIF_I2SCTL_SPKEN_MASK,
>   				LPAIF_I2SCTL_SPKEN_ENABLE);
>   		if (ret)
> @@ -217,7 +217,7 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>   	case SNDRV_PCM_TRIGGER_SUSPEND:
>   	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>   		ret = regmap_update_bits(drvdata->lpaif_map,
> -				LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
> +				LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
>   				LPAIF_I2SCTL_SPKEN_MASK,
>   				LPAIF_I2SCTL_SPKEN_DISABLE);
>   		if (ret)
> @@ -247,7 +247,7 @@ int lpass_cpu_dai_probe(struct snd_soc_dai *dai)
>
>   	/* ensure audio hardware is disabled */
>   	ret = regmap_write(drvdata->lpaif_map,
> -			LPAIF_I2SCTL_REG(drvdata->variant, dai->id), 0);
> +			LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), 0);
>   	if (ret)
>   		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
>   				__func__, ret);
> -----------------------><---------------------------------------------
>

> In addition to your patch above, I also needed to correct the rdma_port
> assignment by removing the i2s port reference:


>
> -----------------------><---------------------------------------------
> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> index c5907d5..580cae1 100644
> --- a/sound/soc/qcom/lpass-platform.c
> +++ b/sound/soc/qcom/lpass-platform.c
> @@ -91,7 +91,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_pcm_substream *substream,
>   	unsigned int channels = params_channels(params);
>   	unsigned int regval;
>   	int bitwidth;
> -	int ret, rdma_port = pcm_data->i2s_port + v->rdmactl_audif_start;
> +	int ret, rdma_port = v->rdmactl_audif_start;
>
>   	bitwidth = snd_pcm_format_width(format);
>   	if (bitwidth < 0) {
> -----------------------><---------------------------------------------
>
> Please incorporate these fixes into your next patch series.
>
I think my patch and above change for rdma port are not necessary as the 
rdma_port existing calculation would result 4 anyway, for 
rdmactl_audif_start= 0 and dai->driver->id = 4. Which is correct in MI2S 
case.

Only replacing dai->id to dai->driver->id should fix the issue on storm 
board. So I will drop these two changes in next verion.
--srini
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen May 12, 2015, 5:04 p.m. UTC | #3
On 05/12/2015 12:21 PM, Srinivas Kandagatla wrote:
> + adding Lars
>
> On 12/05/15 05:06, Kenneth Westfield wrote:
>> Srinivas,
>>
>> I was able to get audio working on the Storm board.  There were several
>> issues.  First, the I2S control port was saved in the DAI driver id field
>> (which was 4), but the DAI id field was used by the macro (which was 0).
>> The patch below fixed it:
>
> Ah, I did not expect that to happen, dai->id and dai->driver->id should be
> same. Surprisingly this is not true for IPQ806x platform only because dais
> count == 1. Which results in dai->id not getting assigned to dai->driver->id
> due to below code snippet in  sound/soc/soc-core.c
>
> --------------------><------------------------------------------
> /*
>   * Back in the old days when we still had component-less DAIs,
>   * instead of having a static name, component-less DAIs would
>   * inherit the name of the parent device so it is possible to
>   * register multiple instances of the DAI. We still need to keep
>   * the same naming style even though those DAIs are not
>   * component-less anymore.
>   */
> if (count == 1 && legacy_dai_naming) {
>      dai->name = fmt_single_name(dev, &dai->id);
> } else {
>      dai->name = fmt_multiple_name(dev, &dai_drv[i]);
>      if (dai_drv[i].id)
>          dai->id = dai_drv[i].id;
>      else
>          dai->id = i;
> }
> --------------------><------------------------------------------
>
> Its not clear from code, why should we enter to legacy naming if the dais
> count == 1 even-though the dai_drv has has valid name and id information.
>
> Mark/Lars,
>
> I can workaround this by using dai->driver->id in the driver, But do you
> think that dai name and id should be assigned from dai drv if present?

I think it should be OK to extend that if condition to

if (count == 1 && dai_drv[0].id == 0 && legacy_dai_naming) ...

It doesn't look like any of the systems for which we currently take the 
legacy path do set id to non 0. And the less new systems following legacy 
naming scheme the better.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla May 14, 2015, 7:55 a.m. UTC | #4
On 12/05/15 18:04, Lars-Peter Clausen wrote:
>>
>> Its not clear from code, why should we enter to legacy naming if the dais
>> count == 1 even-though the dai_drv has has valid name and id information.
>>
>> Mark/Lars,
>>
>> I can workaround this by using dai->driver->id in the driver, But do you
>> think that dai name and id should be assigned from dai drv if present?
>
> I think it should be OK to extend that if condition to
>
> if (count == 1 && dai_drv[0].id == 0 && legacy_dai_naming) ...
>
> It doesn't look like any of the systems for which we currently take the
> legacy path do set id to non 0. And the less new systems following
> legacy naming scheme the better.
>
Thanks Lars, will send out an RFC patch and see what other people thing.

--srini
> - Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index 17ad20d..58ae8af 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -146,7 +146,7 @@  static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	ret = regmap_write(drvdata->lpaif_map,
-			   LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
+			   LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
 			   regval);
 	if (ret) {
 		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
@@ -171,7 +171,7 @@  static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream *substream,
 	int ret;
 
 	ret = regmap_write(drvdata->lpaif_map,
-			   LPAIF_I2SCTL_REG(drvdata->variant, dai->id), 0);
+			   LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), 0);
 	if (ret)
 		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
 				__func__, ret);
@@ -186,7 +186,7 @@  static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream,
 	int ret;
 
 	ret = regmap_update_bits(drvdata->lpaif_map,
-			LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
+			LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
 			LPAIF_I2SCTL_SPKEN_MASK, LPAIF_I2SCTL_SPKEN_ENABLE);
 	if (ret)
 		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
@@ -206,7 +206,7 @@  static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		ret = regmap_update_bits(drvdata->lpaif_map,
-				LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
+				LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
 				LPAIF_I2SCTL_SPKEN_MASK,
 				LPAIF_I2SCTL_SPKEN_ENABLE);
 		if (ret)
@@ -217,7 +217,7 @@  static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		ret = regmap_update_bits(drvdata->lpaif_map,
-				LPAIF_I2SCTL_REG(drvdata->variant, dai->id),
+				LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
 				LPAIF_I2SCTL_SPKEN_MASK,
 				LPAIF_I2SCTL_SPKEN_DISABLE);
 		if (ret)
@@ -247,7 +247,7 @@  int lpass_cpu_dai_probe(struct snd_soc_dai *dai)
 
 	/* ensure audio hardware is disabled */
 	ret = regmap_write(drvdata->lpaif_map,
-			LPAIF_I2SCTL_REG(drvdata->variant, dai->id), 0);
+			LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), 0);
 	if (ret)
 		dev_err(dai->dev, "%s() error writing to i2sctl reg: %d\n",
 				__func__, ret);
-----------------------><---------------------------------------------

In addition to your patch above, I also needed to correct the rdma_port
assignment by removing the i2s port reference:

-----------------------><---------------------------------------------
diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
index c5907d5..580cae1 100644
--- a/sound/soc/qcom/lpass-platform.c
+++ b/sound/soc/qcom/lpass-platform.c
@@ -91,7 +91,7 @@  static int lpass_platform_pcmops_hw_params(struct snd_pcm_substream *substream,
 	unsigned int channels = params_channels(params);
 	unsigned int regval;
 	int bitwidth;
-	int ret, rdma_port = pcm_data->i2s_port + v->rdmactl_audif_start;
+	int ret, rdma_port = v->rdmactl_audif_start;
 
 	bitwidth = snd_pcm_format_width(format);
 	if (bitwidth < 0) {