[alsa-devel] [PATCH 1/3] ASoC: bcm2835: add 24bit support
From: Matthias Reichl hias@horus.com
This adds 24 bit support to the I2S driver of the BCM2835
Code ported from bcm2708-i2s driver in Raspberry Pi tree.
Signed-off-by: Florian Meier florian.meier@koalo.de Signed-off-by: Matthias Reichl hias@horus.com Signed-off-by: Martin Sperl kernel@martin.sperl.org --- sound/soc/bcm/bcm2835-i2s.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index 1c1f221..d2663e7 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -259,6 +259,9 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream, case SNDRV_PCM_FORMAT_S16_LE: data_length = 16; break; + case SNDRV_PCM_FORMAT_S24_LE: + data_length = 24; + break; case SNDRV_PCM_FORMAT_S32_LE: data_length = 32; break; @@ -279,7 +282,7 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream, /* Setup the frame format */ format = BCM2835_I2S_CHEN;
- if (data_length > 24) + if (data_length >= 24) format |= BCM2835_I2S_CHWEX;
format |= BCM2835_I2S_CHWID((data_length-8)&0xf); @@ -570,6 +573,7 @@ static struct snd_soc_dai_driver bcm2835_i2s_dai = { .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_192000, .formats = SNDRV_PCM_FMTBIT_S16_LE + | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE }, .capture = { @@ -577,6 +581,7 @@ static struct snd_soc_dai_driver bcm2835_i2s_dai = { .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_192000, .formats = SNDRV_PCM_FMTBIT_S16_LE + | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE }, .ops = &bcm2835_i2s_dai_ops, -- 2.1.4
From: Matthias Reichl hias@horus.com
We only need to enable the clock if we are a clock master.
Code ported from bcm2708-i2s driver in Raspberry Pi tree. Original work by Zoltan Szenczi.
Signed-off-by: Matthias Reichl hias@horus.com Signed-off-by: Martin Sperl kernel@martin.sperl.org --- sound/soc/bcm/bcm2835-i2s.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index d2663e7..a0026e2 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -276,8 +276,15 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream, /* otherwise calculate a fitting block ratio */ bclk_ratio = 2 * data_length;
- /* set target clock rate*/ - clk_set_rate(dev->clk, sampling_rate * bclk_ratio); + /* Clock should only be set up here if CPU is clock master */ + switch (dev->fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBS_CFS: + case SND_SOC_DAIFMT_CBS_CFM: + clk_set_rate(dev->clk, sampling_rate * bclk_ratio); + break; + default: + break; + }
/* Setup the frame format */ format = BCM2835_I2S_CHEN; -- 2.1.4
From: Matthias Reichl hias@horus.com
Register i2s also as pcm device.
Code ported from bcm2708-i2s driver in Raspberry Pi tree.
Signed-off-by: Florian Meier florian.meier@koalo.de Signed-off-by: Matthias Reichl hias@horus.com Signed-off-by: Martin Sperl kernel@martin.sperl.org --- sound/soc/bcm/bcm2835-i2s.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index a0026e2..8e93295 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -632,6 +632,27 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = { .name = "bcm2835-i2s-comp", };
+static const struct snd_pcm_hardware bcm2835_pcm_hardware = { + .info = SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_JOINT_DUPLEX | + SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID, + .formats = SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S32_LE, + .period_bytes_min = 32, + .period_bytes_max = 64 * PAGE_SIZE, + .periods_min = 2, + .periods_max = 255, + .buffer_bytes_max = 128 * PAGE_SIZE, +}; + +static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = { + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, + .pcm_hardware = &bcm2835_pcm_hardware, + .prealloc_buffer_size = 256 * PAGE_SIZE, +}; + static int bcm2835_i2s_probe(struct platform_device *pdev) { struct bcm2835_i2s_dev *dev; @@ -704,7 +725,9 @@ static int bcm2835_i2s_probe(struct platform_device *pdev) return ret; }
- ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0); + ret = devm_snd_dmaengine_pcm_register( + &pdev->dev, &bcm2835_dmaengine_pcm_config, + SND_DMAENGINE_PCM_FLAG_COMPAT); if (ret) { dev_err(&pdev->dev, "Could not register PCM: %d\n", ret); return ret;
On 04/25/2016 03:39 PM, kernel@martin.sperl.org wrote:
From: Matthias Reichl hias@horus.com
Register i2s also as pcm device.
This is not really what this patch does.
Code ported from bcm2708-i2s driver in Raspberry Pi tree.
Signed-off-by: Florian Meier florian.meier@koalo.de Signed-off-by: Matthias Reichl hias@horus.com Signed-off-by: Martin Sperl kernel@martin.sperl.org
sound/soc/bcm/bcm2835-i2s.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index a0026e2..8e93295 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -632,6 +632,27 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = { .name = "bcm2835-i2s-comp", };
+static const struct snd_pcm_hardware bcm2835_pcm_hardware = {
- .info = SNDRV_PCM_INFO_INTERLEAVED |
SNDRV_PCM_INFO_JOINT_DUPLEX |SNDRV_PCM_INFO_MMAP |SNDRV_PCM_INFO_MMAP_VALID,- .formats = SNDRV_PCM_FMTBIT_S16_LE |
SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S32_LE,- .period_bytes_min = 32,
- .period_bytes_max = 64 * PAGE_SIZE,
- .periods_min = 2,
- .periods_max = 255,
- .buffer_bytes_max = 128 * PAGE_SIZE,
+};
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
- .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
- .pcm_hardware = &bcm2835_pcm_hardware,
- .prealloc_buffer_size = 256 * PAGE_SIZE,
+};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
static int bcm2835_i2s_probe(struct platform_device *pdev) { struct bcm2835_i2s_dev *dev; @@ -704,7 +725,9 @@ static int bcm2835_i2s_probe(struct platform_device *pdev) return ret; }
- ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
- ret = devm_snd_dmaengine_pcm_register(
&pdev->dev, &bcm2835_dmaengine_pcm_config,SND_DMAENGINE_PCM_FLAG_COMPAT);
On Mon, Apr 25, 2016 at 03:54:18PM +0200, Lars-Peter Clausen wrote:
On 04/25/2016 03:39 PM, kernel@martin.sperl.org wrote:
From: Matthias Reichl hias@horus.com
Register i2s also as pcm device.
This is not really what this patch does.
Agreed, we need a better description.
Code ported from bcm2708-i2s driver in Raspberry Pi tree.
Signed-off-by: Florian Meier florian.meier@koalo.de Signed-off-by: Matthias Reichl hias@horus.com Signed-off-by: Martin Sperl kernel@martin.sperl.org
sound/soc/bcm/bcm2835-i2s.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index a0026e2..8e93295 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -632,6 +632,27 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = { .name = "bcm2835-i2s-comp", };
+static const struct snd_pcm_hardware bcm2835_pcm_hardware = {
- .info = SNDRV_PCM_INFO_INTERLEAVED |
SNDRV_PCM_INFO_JOINT_DUPLEX |SNDRV_PCM_INFO_MMAP |SNDRV_PCM_INFO_MMAP_VALID,- .formats = SNDRV_PCM_FMTBIT_S16_LE |
SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S32_LE,- .period_bytes_min = 32,
- .period_bytes_max = 64 * PAGE_SIZE,
I think it'd be better to use SZ constants instead of x * PAGE_SIZE.
- .periods_min = 2,
- .periods_max = 255,
- .buffer_bytes_max = 128 * PAGE_SIZE,
+};
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
- .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
- .pcm_hardware = &bcm2835_pcm_hardware,
- .prealloc_buffer_size = 256 * PAGE_SIZE,
+};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
static int bcm2835_i2s_probe(struct platform_device *pdev) { struct bcm2835_i2s_dev *dev; @@ -704,7 +725,9 @@ static int bcm2835_i2s_probe(struct platform_device *pdev) return ret; }
- ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
- ret = devm_snd_dmaengine_pcm_register(
&pdev->dev, &bcm2835_dmaengine_pcm_config,SND_DMAENGINE_PCM_FLAG_COMPAT);
- .periods_min = 2,
- .periods_max = 255,
- .buffer_bytes_max = 128 * PAGE_SIZE,
+};
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
- .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
- .pcm_hardware = &bcm2835_pcm_hardware,
- .prealloc_buffer_size = 256 * PAGE_SIZE,
+};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
With your patch we should just get an error when trying to configure a stream with 16-bit samples since when setting up the DMA controller the generic code will still choose a 16-bit device port size and this will be rejected by the DMA controller.
When you look at peripherals that have DMA support there are basically two types.
Type A expect that each write (same applies for read) to the memory mapped FIFO corresponds to exactly one sample. So if the sample width is 16-bit a 16-bit write is done, if the sample width is 32-bit a 32-bit write is done. In this case it is up to the DMA controller to fragment the byte stream into individual samples.
Type B on the other hand has a fixed port width (usually the bus size) and expects that each write to the memory mapped FIFO uses the port width. It then internally unpacks the data into the sample data. E.g. if the FIFO is 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry into two samples.
Currently the generic code only supports type A. It would be great if you could add support for type B to support the BCM2835 I2S controller properly.
- Lars
Hi Lars,
first of all thanks a lot for your detailled feedback!
On Tue, Apr 26, 2016 at 10:47:05AM +0200, Lars-Peter Clausen wrote:
- .periods_min = 2,
- .periods_max = 255,
- .buffer_bytes_max = 128 * PAGE_SIZE,
+};
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
- .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
- .pcm_hardware = &bcm2835_pcm_hardware,
- .prealloc_buffer_size = 256 * PAGE_SIZE,
+};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
With your patch we should just get an error when trying to configure a stream with 16-bit samples since when setting up the DMA controller the generic code will still choose a 16-bit device port size and this will be rejected by the DMA controller.
We had that code in downstream RPi kernel for ages (IIRC since 3.10) and so far it worked fine.
I traced the code for the S16_LE case to find out why:
snd_hwparams_to_dma_slave_config() sets src/dst_addr_width to 16bit.
But immediately afterwards snd_dmaengine_pcm_set_config_from_dai_data() overrides addr_width to 32bit.
bcm2835-i2s only supports 32bit access to the FIFO/data register and dma_data.addr_width is set to 32bit in the I2S driver - that code is also in mainline since the initial bcm2835-i2s commit.
Of course all this only works because the number of channels is always 2.
When you look at peripherals that have DMA support there are basically two types.
Type A expect that each write (same applies for read) to the memory mapped FIFO corresponds to exactly one sample. So if the sample width is 16-bit a 16-bit write is done, if the sample width is 32-bit a 32-bit write is done. In this case it is up to the DMA controller to fragment the byte stream into individual samples.
Type B on the other hand has a fixed port width (usually the bus size) and expects that each write to the memory mapped FIFO uses the port width. It then internally unpacks the data into the sample data. E.g. if the FIFO is 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry into two samples.
Currently the generic code only supports type A. It would be great if you could add support for type B to support the BCM2835 I2S controller properly.
Do you have a particular solution in mind?
Introducing a flag to just auto-add some packed formats would be easy, but a generic, robust solution might be tricky. We'd have to make sure that unsupported configurations (like an odd number of 16bit channels on 32bit-only setups) get rejected or we might be in trouble.
so long,
Hias
On 04/26/2016 03:09 PM, Matthias Reichl wrote:
Hi Lars,
first of all thanks a lot for your detailled feedback!
On Tue, Apr 26, 2016 at 10:47:05AM +0200, Lars-Peter Clausen wrote:
- .periods_min = 2,
- .periods_max = 255,
- .buffer_bytes_max = 128 * PAGE_SIZE,
+};
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
- .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
- .pcm_hardware = &bcm2835_pcm_hardware,
- .prealloc_buffer_size = 256 * PAGE_SIZE,
+};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
With your patch we should just get an error when trying to configure a stream with 16-bit samples since when setting up the DMA controller the generic code will still choose a 16-bit device port size and this will be rejected by the DMA controller.
We had that code in downstream RPi kernel for ages (IIRC since 3.10) and so far it worked fine.
I traced the code for the S16_LE case to find out why:
snd_hwparams_to_dma_slave_config() sets src/dst_addr_width to 16bit.
But immediately afterwards snd_dmaengine_pcm_set_config_from_dai_data() overrides addr_width to 32bit.
Ok, makes sense.
bcm2835-i2s only supports 32bit access to the FIFO/data register and dma_data.addr_width is set to 32bit in the I2S driver - that code is also in mainline since the initial bcm2835-i2s commit.
Of course all this only works because the number of channels is always 2.
When you look at peripherals that have DMA support there are basically two types.
Type A expect that each write (same applies for read) to the memory mapped FIFO corresponds to exactly one sample. So if the sample width is 16-bit a 16-bit write is done, if the sample width is 32-bit a 32-bit write is done. In this case it is up to the DMA controller to fragment the byte stream into individual samples.
Type B on the other hand has a fixed port width (usually the bus size) and expects that each write to the memory mapped FIFO uses the port width. It then internally unpacks the data into the sample data. E.g. if the FIFO is 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry into two samples.
Currently the generic code only supports type A. It would be great if you could add support for type B to support the BCM2835 I2S controller properly.
Do you have a particular solution in mind?
Introducing a flag to just auto-add some packed formats would be easy, but a generic, robust solution might be tricky. We'd have to make sure that unsupported configurations (like an odd number of 16bit channels on 32bit-only setups) get rejected or we might be in trouble.
I think in this case the DMA shouldn't limit the supported sample types. Since the unpacking is done by the peripheral the peripheral is the one component that limits what is supported and what is not and the DMA itself has no influence on this. In the peripheral driver you have all the information available there to specify the proper constraints and can handle all the corner cases. The overall constraints are the intersection of the DMA and peripheral constraints, so by having no DMA constraints the peripheral is the one providing all the constraints.
We could either say that we assume that when the addr_width is fixed the DMA shouldn't supply any constraints, or we could introduce a new flag in snd_dmaengine_dai_dma_data that the peripheral has unpack support and the DMA constraints don't matter.
- Lars
On Tue, Apr 26, 2016 at 03:22:29PM +0200, Lars-Peter Clausen wrote:
On 04/26/2016 03:09 PM, Matthias Reichl wrote:
Hi Lars,
first of all thanks a lot for your detailled feedback!
On Tue, Apr 26, 2016 at 10:47:05AM +0200, Lars-Peter Clausen wrote:
- .periods_min = 2,
- .periods_max = 255,
- .buffer_bytes_max = 128 * PAGE_SIZE,
+};
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
- .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
- .pcm_hardware = &bcm2835_pcm_hardware,
- .prealloc_buffer_size = 256 * PAGE_SIZE,
+};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
With your patch we should just get an error when trying to configure a stream with 16-bit samples since when setting up the DMA controller the generic code will still choose a 16-bit device port size and this will be rejected by the DMA controller.
We had that code in downstream RPi kernel for ages (IIRC since 3.10) and so far it worked fine.
I traced the code for the S16_LE case to find out why:
snd_hwparams_to_dma_slave_config() sets src/dst_addr_width to 16bit.
But immediately afterwards snd_dmaengine_pcm_set_config_from_dai_data() overrides addr_width to 32bit.
Ok, makes sense.
bcm2835-i2s only supports 32bit access to the FIFO/data register and dma_data.addr_width is set to 32bit in the I2S driver - that code is also in mainline since the initial bcm2835-i2s commit.
Of course all this only works because the number of channels is always 2.
When you look at peripherals that have DMA support there are basically two types.
Type A expect that each write (same applies for read) to the memory mapped FIFO corresponds to exactly one sample. So if the sample width is 16-bit a 16-bit write is done, if the sample width is 32-bit a 32-bit write is done. In this case it is up to the DMA controller to fragment the byte stream into individual samples.
Type B on the other hand has a fixed port width (usually the bus size) and expects that each write to the memory mapped FIFO uses the port width. It then internally unpacks the data into the sample data. E.g. if the FIFO is 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry into two samples.
Currently the generic code only supports type A. It would be great if you could add support for type B to support the BCM2835 I2S controller properly.
Do you have a particular solution in mind?
Introducing a flag to just auto-add some packed formats would be easy, but a generic, robust solution might be tricky. We'd have to make sure that unsupported configurations (like an odd number of 16bit channels on 32bit-only setups) get rejected or we might be in trouble.
I think in this case the DMA shouldn't limit the supported sample types. Since the unpacking is done by the peripheral the peripheral is the one component that limits what is supported and what is not and the DMA itself has no influence on this. In the peripheral driver you have all the information available there to specify the proper constraints and can handle all the corner cases.
Do you mean let the DAI driver check for and reject invalid configurations in hw_params? Yes, I think that should do it.
The overall constraints are the intersection of the DMA and peripheral constraints, so by having no DMA constraints the peripheral is the one providing all the constraints.
We could either say that we assume that when the addr_width is fixed the DMA shouldn't supply any constraints, or we could introduce a new flag in snd_dmaengine_dai_dma_data that the peripheral has unpack support and the DMA constraints don't matter.
The additional flag sounds like a good idea. How about something like the patch below?
If the ...PACK_16_32 flag is set, 16-bit data will always be packed into 32bit and 32bit DMA is done instead of 16bit. No further checks are done.
I only did a very quick test on downstream kernel 4.4.8 and it seemed to work fine, S16_LE was available and usable. Setting addr_width in the DAI driver was also no longer necessary (but that still can be used as a final override if needed).
I'm not sure if that approach is able to support other packed configurations as well or if I missed something important so I'd be glad to get some feedback on that.
so long,
Hias
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h index f86ef5e..d22b54a 100644 --- a/include/sound/dmaengine_pcm.h +++ b/include/sound/dmaengine_pcm.h @@ -51,6 +51,11 @@ struct dma_chan *snd_dmaengine_pcm_request_channel(dma_filter_fn filter_fn, void *filter_data); struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream);
+/* + * The DAI accepts 2 16-bit values packed into a 32bit word. + */ +#define SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32 BIT(0) + /** * struct snd_dmaengine_dai_dma_data - DAI DMA configuration data * @addr: Address of the DAI data source or destination register. @@ -63,6 +68,8 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream) * requesting the DMA channel. * @chan_name: Custom channel name to use when requesting DMA channel. * @fifo_size: FIFO size of the DAI controller in bytes + * @flags: PCM_DAI flags, only SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32 + * is currently checked */ struct snd_dmaengine_dai_dma_data { dma_addr_t addr; @@ -72,6 +79,7 @@ struct snd_dmaengine_dai_dma_data { void *filter_data; const char *chan_name; unsigned int fifo_size; + unsigned int flags; };
void snd_dmaengine_pcm_set_config_from_dai_data( diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index fba365a..a429f94 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -117,11 +117,21 @@ void snd_dmaengine_pcm_set_config_from_dai_data( if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { slave_config->dst_addr = dma_data->addr; slave_config->dst_maxburst = dma_data->maxburst; + if ((slave_config->dst_addr_width == + DMA_SLAVE_BUSWIDTH_2_BYTES) && + (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32)) + slave_config->dst_addr_width = + DMA_SLAVE_BUSWIDTH_4_BYTES; if (dma_data->addr_width != DMA_SLAVE_BUSWIDTH_UNDEFINED) slave_config->dst_addr_width = dma_data->addr_width; } else { slave_config->src_addr = dma_data->addr; slave_config->src_maxburst = dma_data->maxburst; + if ((slave_config->src_addr_width == + DMA_SLAVE_BUSWIDTH_2_BYTES) && + (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32)) + slave_config->src_addr_width = + DMA_SLAVE_BUSWIDTH_4_BYTES; if (dma_data->addr_width != DMA_SLAVE_BUSWIDTH_UNDEFINED) slave_config->src_addr_width = dma_data->addr_width; } diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index c413973..03d4ad1 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -877,11 +877,11 @@ static int bcm2835_i2s_probe(struct platform_device *pdev) dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr = dma_reg_base + BCM2835_I2S_FIFO_A_REG;
- /* Set the bus width */ - dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr_width = - DMA_SLAVE_BUSWIDTH_4_BYTES; - dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr_width = - DMA_SLAVE_BUSWIDTH_4_BYTES; + /* signal that the DAI needs 2 16-bit values packed into 32bit */ + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].flags = + SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32; + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].flags = + SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32;
/* Set burst */ dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].maxburst = 2; diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 6fd1906..25552c2 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -173,6 +173,11 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) { int bits = snd_pcm_format_physical_width(i);
+ if ((bits == 16) && + (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32)) + /* 16-bit data needs to be packed into 32bit */ + bits = 32; + /* Enable only samples with DMA supported physical widths */ switch (bits) { case 8:
On 04/26/2016 05:18 PM, Matthias Reichl wrote:
On Tue, Apr 26, 2016 at 03:22:29PM +0200, Lars-Peter Clausen wrote:
On 04/26/2016 03:09 PM, Matthias Reichl wrote:
Hi Lars,
first of all thanks a lot for your detailled feedback!
On Tue, Apr 26, 2016 at 10:47:05AM +0200, Lars-Peter Clausen wrote:
> + .periods_min = 2, > + .periods_max = 255, > + .buffer_bytes_max = 128 * PAGE_SIZE, > +}; > + > +static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = { > + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > + .pcm_hardware = &bcm2835_pcm_hardware, > + .prealloc_buffer_size = 256 * PAGE_SIZE, > +};
The generic dmaengine PCM driver auto-discovers these things, no need to provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
With your patch we should just get an error when trying to configure a stream with 16-bit samples since when setting up the DMA controller the generic code will still choose a 16-bit device port size and this will be rejected by the DMA controller.
We had that code in downstream RPi kernel for ages (IIRC since 3.10) and so far it worked fine.
I traced the code for the S16_LE case to find out why:
snd_hwparams_to_dma_slave_config() sets src/dst_addr_width to 16bit.
But immediately afterwards snd_dmaengine_pcm_set_config_from_dai_data() overrides addr_width to 32bit.
Ok, makes sense.
bcm2835-i2s only supports 32bit access to the FIFO/data register and dma_data.addr_width is set to 32bit in the I2S driver - that code is also in mainline since the initial bcm2835-i2s commit.
Of course all this only works because the number of channels is always 2.
When you look at peripherals that have DMA support there are basically two types.
Type A expect that each write (same applies for read) to the memory mapped FIFO corresponds to exactly one sample. So if the sample width is 16-bit a 16-bit write is done, if the sample width is 32-bit a 32-bit write is done. In this case it is up to the DMA controller to fragment the byte stream into individual samples.
Type B on the other hand has a fixed port width (usually the bus size) and expects that each write to the memory mapped FIFO uses the port width. It then internally unpacks the data into the sample data. E.g. if the FIFO is 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry into two samples.
Currently the generic code only supports type A. It would be great if you could add support for type B to support the BCM2835 I2S controller properly.
Do you have a particular solution in mind?
Introducing a flag to just auto-add some packed formats would be easy, but a generic, robust solution might be tricky. We'd have to make sure that unsupported configurations (like an odd number of 16bit channels on 32bit-only setups) get rejected or we might be in trouble.
I think in this case the DMA shouldn't limit the supported sample types. Since the unpacking is done by the peripheral the peripheral is the one component that limits what is supported and what is not and the DMA itself has no influence on this. In the peripheral driver you have all the information available there to specify the proper constraints and can handle all the corner cases.
Do you mean let the DAI driver check for and reject invalid configurations in hw_params? Yes, I think that should do it.
The DAI driver already specifies which formats it can handle, just keep that as it is. The ALSA core takes care of rejecting invalid configurations, so need to do anything special in hw_params.
The overall constraints are the intersection of the DMA and peripheral constraints, so by having no DMA constraints the peripheral is the one providing all the constraints.
We could either say that we assume that when the addr_width is fixed the DMA shouldn't supply any constraints, or we could introduce a new flag in snd_dmaengine_dai_dma_data that the peripheral has unpack support and the DMA constraints don't matter.
The additional flag sounds like a good idea. How about something like the patch below?
If the ...PACK_16_32 flag is set, 16-bit data will always be packed into 32bit and 32bit DMA is done instead of 16bit. No further checks are done.
You don't need to be specific about what kind of unpack the the peripheral supports. If it supports unpacking it is in charge of what formats can be supported.
[...]
void snd_dmaengine_pcm_set_config_from_dai_data( diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index fba365a..a429f94 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -117,11 +117,21 @@ void snd_dmaengine_pcm_set_config_from_dai_data( if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { slave_config->dst_addr = dma_data->addr; slave_config->dst_maxburst = dma_data->maxburst;
if ((slave_config->dst_addr_width ==DMA_SLAVE_BUSWIDTH_2_BYTES) &&(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32))slave_config->dst_addr_width =DMA_SLAVE_BUSWIDTH_4_BYTES;
If the PACK flag is set we should set the width to UNKOWN, that means the DMA controller is free to choose whatever width it things is most efficient. If the port has a fixed width it will be overwritten later as you described in the earlier mail.
[...]
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 6fd1906..25552c2 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -173,6 +173,11 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
In this function when the flag is set just skip this step altogether and set hw->formats. That means the DMA does not restrict the sample formats that are supported.
for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) { int bits = snd_pcm_format_physical_width(i);
if ((bits == 16) &&(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32))/* 16-bit data needs to be packed into 32bit */bits = 32;- /* Enable only samples with DMA supported physical widths */ switch (bits) { case 8:
On Tue, Apr 26, 2016 at 05:30:44PM +0200, Lars-Peter Clausen wrote:
On 04/26/2016 05:18 PM, Matthias Reichl wrote:
On Tue, Apr 26, 2016 at 03:22:29PM +0200, Lars-Peter Clausen wrote:
On 04/26/2016 03:09 PM, Matthias Reichl wrote:
Hi Lars,
first of all thanks a lot for your detailled feedback!
On Tue, Apr 26, 2016 at 10:47:05AM +0200, Lars-Peter Clausen wrote:
>> + .periods_min = 2, >> + .periods_max = 255, >> + .buffer_bytes_max = 128 * PAGE_SIZE, >> +}; >> + >> +static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = { >> + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, >> + .pcm_hardware = &bcm2835_pcm_hardware, >> + .prealloc_buffer_size = 256 * PAGE_SIZE, >> +}; > > The generic dmaengine PCM driver auto-discovers these things, no need to > provide them. The code is OK as it is.
With the auto-discover code we loose the S16_LE format.
If I understood the code in dmaengine_pcm_set_runtime_hwparams correctly, this is because the DMA controller doesn't support 16bit transfers (only multiples of 32bit are allowed).
But since the I2S driver needs exactly 2 channels S16_LE actually works fine (one 32bit transfer per frame).
Do you know of a better way to get S16_LE support? It could well be that I missed something important...
With your patch we should just get an error when trying to configure a stream with 16-bit samples since when setting up the DMA controller the generic code will still choose a 16-bit device port size and this will be rejected by the DMA controller.
We had that code in downstream RPi kernel for ages (IIRC since 3.10) and so far it worked fine.
I traced the code for the S16_LE case to find out why:
snd_hwparams_to_dma_slave_config() sets src/dst_addr_width to 16bit.
But immediately afterwards snd_dmaengine_pcm_set_config_from_dai_data() overrides addr_width to 32bit.
Ok, makes sense.
bcm2835-i2s only supports 32bit access to the FIFO/data register and dma_data.addr_width is set to 32bit in the I2S driver - that code is also in mainline since the initial bcm2835-i2s commit.
Of course all this only works because the number of channels is always 2.
When you look at peripherals that have DMA support there are basically two types.
Type A expect that each write (same applies for read) to the memory mapped FIFO corresponds to exactly one sample. So if the sample width is 16-bit a 16-bit write is done, if the sample width is 32-bit a 32-bit write is done. In this case it is up to the DMA controller to fragment the byte stream into individual samples.
Type B on the other hand has a fixed port width (usually the bus size) and expects that each write to the memory mapped FIFO uses the port width. It then internally unpacks the data into the sample data. E.g. if the FIFO is 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry into two samples.
Currently the generic code only supports type A. It would be great if you could add support for type B to support the BCM2835 I2S controller properly.
Do you have a particular solution in mind?
Introducing a flag to just auto-add some packed formats would be easy, but a generic, robust solution might be tricky. We'd have to make sure that unsupported configurations (like an odd number of 16bit channels on 32bit-only setups) get rejected or we might be in trouble.
I think in this case the DMA shouldn't limit the supported sample types. Since the unpacking is done by the peripheral the peripheral is the one component that limits what is supported and what is not and the DMA itself has no influence on this. In the peripheral driver you have all the information available there to specify the proper constraints and can handle all the corner cases.
Do you mean let the DAI driver check for and reject invalid configurations in hw_params? Yes, I think that should do it.
The DAI driver already specifies which formats it can handle, just keep that as it is. The ALSA core takes care of rejecting invalid configurations, so need to do anything special in hw_params.
I was thinking about the odd corner cases which might not be caught by the ALSA core. Like packed single channel transfers with an odd period length. Not sure if there are any devices out there that support such things, but you never know :)
For bcm2835-i2s it's no problem at all, it'll work fine without any changes to hw_params.
The overall constraints are the intersection of the DMA and peripheral constraints, so by having no DMA constraints the peripheral is the one providing all the constraints.
We could either say that we assume that when the addr_width is fixed the DMA shouldn't supply any constraints, or we could introduce a new flag in snd_dmaengine_dai_dma_data that the peripheral has unpack support and the DMA constraints don't matter.
The additional flag sounds like a good idea. How about something like the patch below?
If the ...PACK_16_32 flag is set, 16-bit data will always be packed into 32bit and 32bit DMA is done instead of 16bit. No further checks are done.
You don't need to be specific about what kind of unpack the the peripheral supports. If it supports unpacking it is in charge of what formats can be supported.
[...]
void snd_dmaengine_pcm_set_config_from_dai_data( diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index fba365a..a429f94 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -117,11 +117,21 @@ void snd_dmaengine_pcm_set_config_from_dai_data( if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { slave_config->dst_addr = dma_data->addr; slave_config->dst_maxburst = dma_data->maxburst;
if ((slave_config->dst_addr_width ==DMA_SLAVE_BUSWIDTH_2_BYTES) &&(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32))slave_config->dst_addr_width =DMA_SLAVE_BUSWIDTH_4_BYTES;If the PACK flag is set we should set the width to UNKOWN, that means the DMA controller is free to choose whatever width it things is most efficient. If the port has a fixed width it will be overwritten later as you described in the earlier mail.
[...]
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 6fd1906..25552c2 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -173,6 +173,11 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
In this function when the flag is set just skip this step altogether and set hw->formats. That means the DMA does not restrict the sample formats that are supported.
for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) { int bits = snd_pcm_format_physical_width(i);
if ((bits == 16) &&(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK_16_32))/* 16-bit data needs to be packed into 32bit */bits = 32;- /* Enable only samples with DMA supported physical widths */ switch (bits) { case 8:
Ok, I think I got you now and I have to say that solution is really elegant. I wouldn't have thought that it could be that simple. V2 of the patch is below.
so long,
Hias
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h index f86ef5e..67be244 100644 --- a/include/sound/dmaengine_pcm.h +++ b/include/sound/dmaengine_pcm.h @@ -51,6 +51,16 @@ struct dma_chan *snd_dmaengine_pcm_request_channel(dma_filter_fn filter_fn, void *filter_data); struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream);
+/* + * The DAI supports packed transfers, eg 2 16-bit samples in a 32-bit word. + * If this flag is set the dmaengine driver won't put any restriction on + * the supported sample formats and set the DMA transfer size to undefined. + * The DAI driver is responsible to disable any unsupported formats in it's + * configuration and catch corner cases that are not already handled in + * the ALSA core. + */ +#define SND_DMAENGINE_PCM_DAI_FLAG_PACK BIT(0) + /** * struct snd_dmaengine_dai_dma_data - DAI DMA configuration data * @addr: Address of the DAI data source or destination register. @@ -63,6 +73,7 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream) * requesting the DMA channel. * @chan_name: Custom channel name to use when requesting DMA channel. * @fifo_size: FIFO size of the DAI controller in bytes + * @flags: PCM_DAI flags, only SND_DMAENGINE_PCM_DAI_FLAG_PACK for now */ struct snd_dmaengine_dai_dma_data { dma_addr_t addr; @@ -72,6 +83,7 @@ struct snd_dmaengine_dai_dma_data { void *filter_data; const char *chan_name; unsigned int fifo_size; + unsigned int flags; };
void snd_dmaengine_pcm_set_config_from_dai_data( diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index fba365a..c6519ca 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -106,8 +106,9 @@ EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config); * direction of the substream. If the substream is a playback stream the dst * fields will be initialized, if it is a capture stream the src fields will be * initialized. The {dst,src}_addr_width field will only be initialized if the - * addr_width field of the DAI DMA data struct is not equal to - * DMA_SLAVE_BUSWIDTH_UNDEFINED. + * SND_DMAENGINE_PCM_DAI_FLAG_PACK flag is set or if the addr_width field of + * the DAI DMA data struct is not equal to DMA_SLAVE_BUSWIDTH_UNDEFINED. If + * both conditions are met the latter takes priority. */ void snd_dmaengine_pcm_set_config_from_dai_data( const struct snd_pcm_substream *substream, @@ -117,11 +118,17 @@ void snd_dmaengine_pcm_set_config_from_dai_data( if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { slave_config->dst_addr = dma_data->addr; slave_config->dst_maxburst = dma_data->maxburst; + if (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK) + slave_config->dst_addr_width = + DMA_SLAVE_BUSWIDTH_UNDEFINED; if (dma_data->addr_width != DMA_SLAVE_BUSWIDTH_UNDEFINED) slave_config->dst_addr_width = dma_data->addr_width; } else { slave_config->src_addr = dma_data->addr; slave_config->src_maxburst = dma_data->maxburst; + if (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK) + slave_config->src_addr_width = + DMA_SLAVE_BUSWIDTH_UNDEFINED; if (dma_data->addr_width != DMA_SLAVE_BUSWIDTH_UNDEFINED) slave_config->src_addr_width = dma_data->addr_width; } diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index c413973..68c8e524 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -887,6 +887,15 @@ static int bcm2835_i2s_probe(struct platform_device *pdev) dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].maxburst = 2; dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].maxburst = 2;
+ /* + * Set the PACK flag to enable S16_LE support (2 S16_LE values + * packed into 32-bit transfers). + */ + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].flags = + SND_DMAENGINE_PCM_DAI_FLAG_PACK; + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].flags = + SND_DMAENGINE_PCM_DAI_FLAG_PACK; + /* BCLK ratio - use default */ dev->bclk_ratio = 0;
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 6fd1906..30d3b2a 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -162,32 +162,44 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea addr_widths = dma_caps.src_addr_widths; }
- /* - * Prepare formats mask for valid/allowed sample types. If the dma does - * not have support for the given physical word size, it needs to be - * masked out so user space can not use the format which produces - * corrupted audio. - * In case the dma driver does not implement the slave_caps the default - * assumption is that it supports 1, 2 and 4 bytes widths. - */ - for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) { - int bits = snd_pcm_format_physical_width(i); - - /* Enable only samples with DMA supported physical widths */ - switch (bits) { - case 8: - case 16: - case 24: - case 32: - case 64: - if (addr_widths & (1 << (bits / 8))) - hw.formats |= (1LL << i); - break; - default: - /* Unsupported types */ - break; + if (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK) + /* + * Don't put a restriction on supported formats. + * It's the responsibility of the DAI driver to provide the + * supported format information. + */ + hw.formats = GENMASK_ULL((int)SNDRV_PCM_FORMAT_LAST-1, 0); + else + /* + * Prepare formats mask for valid/allowed sample types. If the + * dma does not have support for the given physical word size, + * it needs to be masked out so user space can not use the + * format which produces corrupted audio. + * In case the dma driver does not implement the slave_caps the + * default assumption is that it supports 1, 2 and 4 bytes + * widths. + */ + for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) { + int bits = snd_pcm_format_physical_width(i); + + /* + * Enable only samples with DMA supported physical + * widths + */ + switch (bits) { + case 8: + case 16: + case 24: + case 32: + case 64: + if (addr_widths & (1 << (bits / 8))) + hw.formats |= (1LL << i); + break; + default: + /* Unsupported types */ + break; + } } - }
return snd_soc_set_runtime_hwparams(substream, &hw); }
On 04/26/2016 08:05 PM, Matthias Reichl wrote: [...]
Ok, I think I got you now and I have to say that solution is really elegant. I wouldn't have thought that it could be that simple. V2 of the patch is below.
Looks good. One minor thing inline below. Please split this into two patches, one adding support for the flag and the other using it in the bcm diver and then send them to the ASoC maintainers.
[...]
hw.formats = GENMASK_ULL((int)SNDRV_PCM_FORMAT_LAST-1, 0);
setting it to 0 is OK. 0 is a special case that means all formats supported.
participants (3)
-
kernel＠martin.sperl.org -
Lars-Peter Clausen -
Matthias Reichl