Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats
@ 2019-10-08 11:57 Peter Ujfalusi
  2019-10-08 12:53 ` [alsa-devel] Applied "ASoC: pcm3168a: Use fixup instead of constraint for channels and formats" to the asoc tree Mark Brown
  2019-10-15  2:38 ` [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Kuninori Morimoto
  0 siblings, 2 replies; 5+ messages in thread
From: Peter Ujfalusi @ 2019-10-08 11:57 UTC (permalink / raw)
  To: broonie, lgirdwood; +Cc: alsa-devel, kuninori.morimoto.gx

The snd_pcm_hw_constraint_minmax() works fine when a single codec is
connected to a single CPU DAI, but in multicodec or DPCM setup the
constraints placed by the driver will apply to the whole PCM stream (FE
included) and thus prevents more than 8 playback channels for example.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
Hi,

I have a board which uses two pcm3168a codec connected to one McASP port with
parallel serializers.
One 16 channel (2x 8 channel) playback and one 12 (2x 6 channel) capture should
be possible as I use only one CPU DAI + platform.
However no matter how I tried to set things up ALSA blocked more than 8 channel
playback in simple multicodec or DPCM setup.

It turned out that the constraint set by the codec on the BE is actually applies
to the FE as well as they both use the same runtime.

The codec still have different constraints depending on the interface format, so
I replaced the constraint with a fixup function to make sure that the codec will
not advertise non supported formats.

Is this a know feature?

Another thing which I have came across is the lack of support for parallel data
pins in ASoC core.
For example the soc-pcm only checks the tdm slots to fix up the channels
supported by the codec, but when we have multiple pins in use and they are
operate in parallel, then the number of slots must be multiplied by the data
pins in use.
I have wip patches for this, adding

int snd_soc_dai_set_data_pins(struct snd_soc_dai *dai,
			      unsigned int tx_data_pins,
			      unsigned int rx_data_pins);

and then using the information in soc-pcm, but need to check further to see if
there are other places this might be needed.

Regards,
Peter

 sound/soc/codecs/pcm3168a.c | 98 +++++++++++++++----------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/sound/soc/codecs/pcm3168a.c b/sound/soc/codecs/pcm3168a.c
index 88b75695fbf7..708dac27feff 100644
--- a/sound/soc/codecs/pcm3168a.c
+++ b/sound/soc/codecs/pcm3168a.c
@@ -62,6 +62,7 @@ struct pcm3168a_priv {
 	unsigned long sysclk;
 
 	struct pcm3168a_io_params io_params[2];
+	struct snd_soc_dai_driver dai_drv[2];
 };
 
 static const char *const pcm3168a_roll_off[] = { "Sharp", "Slow" };
@@ -314,6 +315,37 @@ static int pcm3168a_set_dai_sysclk(struct snd_soc_dai *dai,
 	return 0;
 }
 
+static void pcm3168a_update_fixup_pcm_stream(struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm3168a_priv *pcm3168a = snd_soc_component_get_drvdata(component);
+	u64 formats = SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_LE;
+	unsigned int channel_max = dai->id == PCM3168A_DAI_DAC ? 8 : 6;
+
+	if (!pcm3168a->io_params[dai->id].fmt)
+		return;
+
+	if (pcm3168a->io_params[dai->id].fmt == PCM3168A_FMT_RIGHT_J) {
+		/* S16_LE is only supported in RIGHT_J mode */
+		formats |= SNDRV_PCM_FMTBIT_S16_LE;
+
+		/*
+		 * If multi DIN/DOUT is not selected, RIGHT_J can only support
+		 * two channels (no TDM support)
+		 */
+		if (pcm3168a->io_params[dai->id].tdm_slots != 2)
+			channel_max = 2;
+	}
+
+	if (dai->id == PCM3168A_DAI_DAC) {
+		dai->driver->playback.channels_max = channel_max;
+		dai->driver->playback.formats = formats;
+	} else {
+		dai->driver->capture.channels_max = channel_max;
+		dai->driver->capture.formats = formats;
+	}
+}
+
 static int pcm3168a_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format)
 {
 	struct snd_soc_component *component = dai->component;
@@ -376,6 +408,8 @@ static int pcm3168a_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format)
 
 	regmap_update_bits(pcm3168a->regmap, reg, mask, fmt << shift);
 
+	pcm3168a_update_fixup_pcm_stream(dai);
+
 	return 0;
 }
 
@@ -409,6 +443,8 @@ static int pcm3168a_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
 	else
 		io_params->tdm_mask = rx_mask;
 
+	pcm3168a_update_fixup_pcm_stream(dai);
+
 	return 0;
 }
 
@@ -530,63 +566,7 @@ static int pcm3168a_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int pcm3168a_startup(struct snd_pcm_substream *substream,
-			    struct snd_soc_dai *dai)
-{
-	struct snd_soc_component *component = dai->component;
-	struct pcm3168a_priv *pcm3168a = snd_soc_component_get_drvdata(component);
-	unsigned int sample_min;
-	unsigned int channel_max;
-	unsigned int channel_maxs[] = {
-		8, /* DAC */
-		6  /* ADC */
-	};
-
-	/*
-	 * Available Data Bits
-	 *
-	 * RIGHT_J : 24 / 16
-	 * LEFT_J  : 24
-	 * I2S     : 24
-	 *
-	 * TDM available
-	 *
-	 * I2S
-	 * LEFT_J
-	 */
-	switch (pcm3168a->io_params[dai->id].fmt) {
-	case PCM3168A_FMT_RIGHT_J:
-		sample_min  = 16;
-		channel_max =  2;
-		break;
-	case PCM3168A_FMT_LEFT_J:
-	case PCM3168A_FMT_I2S:
-	case PCM3168A_FMT_DSP_A:
-	case PCM3168A_FMT_DSP_B:
-		sample_min  = 24;
-		channel_max = channel_maxs[dai->id];
-		break;
-	default:
-		sample_min  = 24;
-		channel_max =  2;
-	}
-
-	snd_pcm_hw_constraint_minmax(substream->runtime,
-				     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
-				     sample_min, 32);
-
-	/* Allow all channels in multi DIN/DOUT mode */
-	if (pcm3168a->io_params[dai->id].tdm_slots == 2)
-		channel_max = channel_maxs[dai->id];
-
-	snd_pcm_hw_constraint_minmax(substream->runtime,
-				     SNDRV_PCM_HW_PARAM_CHANNELS,
-				     2, channel_max);
-
-	return 0;
-}
 static const struct snd_soc_dai_ops pcm3168a_dai_ops = {
-	.startup	= pcm3168a_startup,
 	.set_fmt	= pcm3168a_set_dai_fmt,
 	.set_sysclk	= pcm3168a_set_dai_sysclk,
 	.hw_params	= pcm3168a_hw_params,
@@ -776,8 +756,10 @@ int pcm3168a_probe(struct device *dev, struct regmap *regmap)
 	pm_runtime_enable(dev);
 	pm_runtime_idle(dev);
 
-	ret = devm_snd_soc_register_component(dev, &pcm3168a_driver, pcm3168a_dais,
-			ARRAY_SIZE(pcm3168a_dais));
+	memcpy(pcm3168a->dai_drv, pcm3168a_dais, sizeof(pcm3168a->dai_drv));
+	ret = devm_snd_soc_register_component(dev, &pcm3168a_driver,
+					      pcm3168a->dai_drv,
+					      ARRAY_SIZE(pcm3168a->dai_drv));
 	if (ret) {
 		dev_err(dev, "failed to register component: %d\n", ret);
 		goto err_regulator;
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [alsa-devel] Applied "ASoC: pcm3168a: Use fixup instead of constraint for channels and formats" to the asoc tree
  2019-10-08 11:57 [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Peter Ujfalusi
@ 2019-10-08 12:53 ` Mark Brown
  2019-10-15  2:38 ` [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Kuninori Morimoto
  1 sibling, 0 replies; 5+ messages in thread
From: Mark Brown @ 2019-10-08 12:53 UTC (permalink / raw)
  To: Peter Ujfalusi; +Cc: alsa-devel, Mark Brown, lgirdwood, kuninori.morimoto.gx

The patch

   ASoC: pcm3168a: Use fixup instead of constraint for channels and formats

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From cfc28ac124c456ffbff5b34d4108d8a201cd7fbb Mon Sep 17 00:00:00 2001
From: Peter Ujfalusi <peter.ujfalusi@ti.com>
Date: Tue, 8 Oct 2019 14:57:20 +0300
Subject: [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels
 and formats

The snd_pcm_hw_constraint_minmax() works fine when a single codec is
connected to a single CPU DAI, but in multicodec or DPCM setup the
constraints placed by the driver will apply to the whole PCM stream (FE
included) and thus prevents more than 8 playback channels for example.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Link: https://lore.kernel.org/r/20191008115720.7135-1-peter.ujfalusi@ti.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/pcm3168a.c | 98 +++++++++++++++----------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/sound/soc/codecs/pcm3168a.c b/sound/soc/codecs/pcm3168a.c
index 88b75695fbf7..708dac27feff 100644
--- a/sound/soc/codecs/pcm3168a.c
+++ b/sound/soc/codecs/pcm3168a.c
@@ -62,6 +62,7 @@ struct pcm3168a_priv {
 	unsigned long sysclk;
 
 	struct pcm3168a_io_params io_params[2];
+	struct snd_soc_dai_driver dai_drv[2];
 };
 
 static const char *const pcm3168a_roll_off[] = { "Sharp", "Slow" };
@@ -314,6 +315,37 @@ static int pcm3168a_set_dai_sysclk(struct snd_soc_dai *dai,
 	return 0;
 }
 
+static void pcm3168a_update_fixup_pcm_stream(struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm3168a_priv *pcm3168a = snd_soc_component_get_drvdata(component);
+	u64 formats = SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_LE;
+	unsigned int channel_max = dai->id == PCM3168A_DAI_DAC ? 8 : 6;
+
+	if (!pcm3168a->io_params[dai->id].fmt)
+		return;
+
+	if (pcm3168a->io_params[dai->id].fmt == PCM3168A_FMT_RIGHT_J) {
+		/* S16_LE is only supported in RIGHT_J mode */
+		formats |= SNDRV_PCM_FMTBIT_S16_LE;
+
+		/*
+		 * If multi DIN/DOUT is not selected, RIGHT_J can only support
+		 * two channels (no TDM support)
+		 */
+		if (pcm3168a->io_params[dai->id].tdm_slots != 2)
+			channel_max = 2;
+	}
+
+	if (dai->id == PCM3168A_DAI_DAC) {
+		dai->driver->playback.channels_max = channel_max;
+		dai->driver->playback.formats = formats;
+	} else {
+		dai->driver->capture.channels_max = channel_max;
+		dai->driver->capture.formats = formats;
+	}
+}
+
 static int pcm3168a_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format)
 {
 	struct snd_soc_component *component = dai->component;
@@ -376,6 +408,8 @@ static int pcm3168a_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format)
 
 	regmap_update_bits(pcm3168a->regmap, reg, mask, fmt << shift);
 
+	pcm3168a_update_fixup_pcm_stream(dai);
+
 	return 0;
 }
 
@@ -409,6 +443,8 @@ static int pcm3168a_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
 	else
 		io_params->tdm_mask = rx_mask;
 
+	pcm3168a_update_fixup_pcm_stream(dai);
+
 	return 0;
 }
 
@@ -530,63 +566,7 @@ static int pcm3168a_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int pcm3168a_startup(struct snd_pcm_substream *substream,
-			    struct snd_soc_dai *dai)
-{
-	struct snd_soc_component *component = dai->component;
-	struct pcm3168a_priv *pcm3168a = snd_soc_component_get_drvdata(component);
-	unsigned int sample_min;
-	unsigned int channel_max;
-	unsigned int channel_maxs[] = {
-		8, /* DAC */
-		6  /* ADC */
-	};
-
-	/*
-	 * Available Data Bits
-	 *
-	 * RIGHT_J : 24 / 16
-	 * LEFT_J  : 24
-	 * I2S     : 24
-	 *
-	 * TDM available
-	 *
-	 * I2S
-	 * LEFT_J
-	 */
-	switch (pcm3168a->io_params[dai->id].fmt) {
-	case PCM3168A_FMT_RIGHT_J:
-		sample_min  = 16;
-		channel_max =  2;
-		break;
-	case PCM3168A_FMT_LEFT_J:
-	case PCM3168A_FMT_I2S:
-	case PCM3168A_FMT_DSP_A:
-	case PCM3168A_FMT_DSP_B:
-		sample_min  = 24;
-		channel_max = channel_maxs[dai->id];
-		break;
-	default:
-		sample_min  = 24;
-		channel_max =  2;
-	}
-
-	snd_pcm_hw_constraint_minmax(substream->runtime,
-				     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
-				     sample_min, 32);
-
-	/* Allow all channels in multi DIN/DOUT mode */
-	if (pcm3168a->io_params[dai->id].tdm_slots == 2)
-		channel_max = channel_maxs[dai->id];
-
-	snd_pcm_hw_constraint_minmax(substream->runtime,
-				     SNDRV_PCM_HW_PARAM_CHANNELS,
-				     2, channel_max);
-
-	return 0;
-}
 static const struct snd_soc_dai_ops pcm3168a_dai_ops = {
-	.startup	= pcm3168a_startup,
 	.set_fmt	= pcm3168a_set_dai_fmt,
 	.set_sysclk	= pcm3168a_set_dai_sysclk,
 	.hw_params	= pcm3168a_hw_params,
@@ -776,8 +756,10 @@ int pcm3168a_probe(struct device *dev, struct regmap *regmap)
 	pm_runtime_enable(dev);
 	pm_runtime_idle(dev);
 
-	ret = devm_snd_soc_register_component(dev, &pcm3168a_driver, pcm3168a_dais,
-			ARRAY_SIZE(pcm3168a_dais));
+	memcpy(pcm3168a->dai_drv, pcm3168a_dais, sizeof(pcm3168a->dai_drv));
+	ret = devm_snd_soc_register_component(dev, &pcm3168a_driver,
+					      pcm3168a->dai_drv,
+					      ARRAY_SIZE(pcm3168a->dai_drv));
 	if (ret) {
 		dev_err(dev, "failed to register component: %d\n", ret);
 		goto err_regulator;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats
  2019-10-08 11:57 [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Peter Ujfalusi
  2019-10-08 12:53 ` [alsa-devel] Applied "ASoC: pcm3168a: Use fixup instead of constraint for channels and formats" to the asoc tree Mark Brown
@ 2019-10-15  2:38 ` Kuninori Morimoto
  2019-10-15  8:57   ` Peter Ujfalusi
  1 sibling, 1 reply; 5+ messages in thread
From: Kuninori Morimoto @ 2019-10-15  2:38 UTC (permalink / raw)
  To: Peter Ujfalusi; +Cc: alsa-devel, broonie, lgirdwood


Hi Peter

> The snd_pcm_hw_constraint_minmax() works fine when a single codec is
> connected to a single CPU DAI, but in multicodec or DPCM setup the
> constraints placed by the driver will apply to the whole PCM stream (FE
> included) and thus prevents more than 8 playback channels for example.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---

This patch breaks my sound.
I'm selecting I2S between CPU<->Codec, and 
before this patch, it had been selected 24bit + I2S.
But after this patch, it selects 16bit + I2S,
and say "16-bit slots are supported only for slave mode using right justified"

I'm not 100% understand pcm3168a.
I'm happy if you can solve this issue.
If you can't, can you please teach me where should I check ?

Thank you for your help !!
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats
  2019-10-15  2:38 ` [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Kuninori Morimoto
@ 2019-10-15  8:57   ` Peter Ujfalusi
  2019-10-16  0:33     ` Kuninori Morimoto
  0 siblings, 1 reply; 5+ messages in thread
From: Peter Ujfalusi @ 2019-10-15  8:57 UTC (permalink / raw)
  To: Kuninori Morimoto; +Cc: alsa-devel, broonie, lgirdwood

Hi Morimoto-san,

On 15/10/2019 5.38, Kuninori Morimoto wrote:
> 
> Hi Peter
> 
>> The snd_pcm_hw_constraint_minmax() works fine when a single codec is
>> connected to a single CPU DAI, but in multicodec or DPCM setup the
>> constraints placed by the driver will apply to the whole PCM stream (FE
>> included) and thus prevents more than 8 playback channels for example.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
> 
> This patch breaks my sound.
> I'm selecting I2S between CPU<->Codec, and 
> before this patch, it had been selected 24bit + I2S.
> But after this patch, it selects 16bit + I2S,
> and say "16-bit slots are supported only for slave mode using right justified"

Oops. I have sent a patch to fix this. Sorry for the inconvenience!

FWIW, I'm running one pcm3168a in CBS_CFS | RIGHT_J with parallel
serializers (to one McASP) and other two also in CBS_CFS | RIGHT_J
connected to a single McASP in parallel (so I can play 16 channel audio
using the two).

> I'm not 100% understand pcm3168a.
> I'm happy if you can solve this issue.
> If you can't, can you please teach me where should I check ?

Just sent a patch to fix the issue you are seeing with I2S mode (tested
it myself).

Sorry again.

> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats
  2019-10-15  8:57   ` Peter Ujfalusi
@ 2019-10-16  0:33     ` Kuninori Morimoto
  0 siblings, 0 replies; 5+ messages in thread
From: Kuninori Morimoto @ 2019-10-16  0:33 UTC (permalink / raw)
  To: Peter Ujfalusi; +Cc: alsa-devel, broonie, lgirdwood


Hi Peter

Thank you for your fixup patch

> > This patch breaks my sound.
> > I'm selecting I2S between CPU<->Codec, and 
> > before this patch, it had been selected 24bit + I2S.
> > But after this patch, it selects 16bit + I2S,
> > and say "16-bit slots are supported only for slave mode using right justified"
> 
> Oops. I have sent a patch to fix this. Sorry for the inconvenience!

No problem.
Thank you for your quick response.

> > I'm not 100% understand pcm3168a.
> > I'm happy if you can solve this issue.
> > If you can't, can you please teach me where should I check ?
> 
> Just sent a patch to fix the issue you are seeing with I2S mode (tested
> it myself).

Yes, Thanks.
It could solve my issue.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 11:57 [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Peter Ujfalusi
2019-10-08 12:53 ` [alsa-devel] Applied "ASoC: pcm3168a: Use fixup instead of constraint for channels and formats" to the asoc tree Mark Brown
2019-10-15  2:38 ` [alsa-devel] [PATCH] ASoC: pcm3168a: Use fixup instead of constraint for channels and formats Kuninori Morimoto
2019-10-15  8:57   ` Peter Ujfalusi
2019-10-16  0:33     ` Kuninori Morimoto

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org alsa-devel@archiver.kernel.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox