| Message ID | 5549BA98.9080501@linaro.org |
|---|---|
| State | New |
| Headers | show |
+ 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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,