| Message ID | 20171029094048.GA5295@ubuntu |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: sun4i-codec: fixed 32bit audio capture support for H3/H2+ | expand |
On Sun, Oct 29, 2017 at 02:41:01AM -0700, Andrea Bondavalli wrote: > Fixed support for 32bit audio capture for Allwinner H3/H2+ SoC > > Signed-off-by: Andrea Bondavalli <andrea.bondavalli74@gmail.com> A more detailed commit log would be welcome. What are the issues involved would be the more valuable information, and then how you'ring fixing it and why would be great. > --- > sound/soc/sunxi/sun4i-codec.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c > index baa9007..f40fa34 100644 > --- a/sound/soc/sunxi/sun4i-codec.c > +++ b/sound/soc/sunxi/sun4i-codec.c > @@ -346,11 +346,6 @@ static int sun4i_codec_prepare_capture(struct snd_pcm_substream *substream, > 0x3 << 8, > 0x1 << 8); > > - /* Fill most significant bits with valid data MSB */ > - regmap_field_update_bits(scodec->reg_adc_fifoc, > - BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), > - BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE)); > - > return 0; > } > > @@ -490,6 +485,30 @@ static int sun4i_codec_hw_params_capture(struct sun4i_codec *scodec, > BIT(SUN4I_CODEC_ADC_FIFOC_MONO_EN), > 0); > > + /* Set the number of sample bits to either 16 or 24 bits */ > + if (hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS)->min == 32) { > + regmap_field_update_bits(scodec->reg_adc_fifoc, > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS), > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS)); > + > + regmap_field_update_bits(scodec->reg_adc_fifoc, > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), > + 0); > + > + scodec->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + } else { > + regmap_field_update_bits(scodec->reg_adc_fifoc, > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS), > + 0); > + > + /* Fill most significant bits with valid data MSB */ > + regmap_field_update_bits(scodec->reg_adc_fifoc, > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE)); > + > + scodec->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; Doesn't that break 24 bits? Maxime
Hello Maxime, everyone, see comments inline. On Mon, Oct 30, 2017 at 09:55:42AM +0100, Maxime Ripard wrote: > On Sun, Oct 29, 2017 at 02:41:01AM -0700, Andrea Bondavalli wrote: > > Fixed support for 32bit audio capture for Allwinner H3/H2+ SoC > > > > Signed-off-by: Andrea Bondavalli <andrea.bondavalli74@gmail.com> > > A more detailed commit log would be welcome. > > What are the issues involved would be the more valuable information, > and then how you'ring fixing it and why would be great. > H3 audio codec supports 16 and 24 bits capture formats (same for the playback). Switching between these two modes can be achieved by using the RX_SAMPLE_BITS (bit 6) of the AC_ADC_FIFOC register (offset 0x10) of the audio codec. The RX_FIFO_MODE (bit 24) register must also be set accordingly to have the desired RX FIFO output mode. The current implementation declares support for 16bit and 32bit (with a resolution of 24bit) capture formats but the 32bit implementation is broken because the RX_SAMPLE_BITS and the RX_FIFO_MODE bits are not set correctly and in addition to this the DMA bus transfer width is left to 2 bytes instead of 4 bytes causing invalid captures whose durating is double than the expected period. The following patch sets the H3 audio codec registers properly when a 24/32bit capture is requested. Similar approach was already implemented for the audio playback part. > > --- > > sound/soc/sunxi/sun4i-codec.c | 29 ++++++++++++++++++++++++----- > > 1 file changed, 24 insertions(+), 5 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c > > index baa9007..f40fa34 100644 > > --- a/sound/soc/sunxi/sun4i-codec.c > > +++ b/sound/soc/sunxi/sun4i-codec.c > > @@ -346,11 +346,6 @@ static int sun4i_codec_prepare_capture(struct snd_pcm_substream *substream, > > 0x3 << 8, > > 0x1 << 8); > > > > - /* Fill most significant bits with valid data MSB */ > > - regmap_field_update_bits(scodec->reg_adc_fifoc, > > - BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), > > - BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE)); > > - > > return 0; > > } > > > > @@ -490,6 +485,30 @@ static int sun4i_codec_hw_params_capture(struct sun4i_codec *scodec, > > BIT(SUN4I_CODEC_ADC_FIFOC_MONO_EN), > > 0); > > > > + /* Set the number of sample bits to either 16 or 24 bits */ > > + if (hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS)->min == 32) { > > + regmap_field_update_bits(scodec->reg_adc_fifoc, > > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS), > > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS)); > > + > > + regmap_field_update_bits(scodec->reg_adc_fifoc, > > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), > > + 0); > > + > > + scodec->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + } else { > > + regmap_field_update_bits(scodec->reg_adc_fifoc, > > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS), > > + 0); > > + > > + /* Fill most significant bits with valid data MSB */ > > + regmap_field_update_bits(scodec->reg_adc_fifoc, > > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), > > + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE)); > > + > > + scodec->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; > > Doesn't that break 24 bits? No. I did tests with 16/24/32bit captures and all seem to work properly now: arecord -vv -r 48000 -c 2 -f S32_LE /home/root/s32_le.wav -d 10 arecord -vv -r 48000 -c 2 -f S24_LE /home/root/s24_le.wav -d 10 arecord -vv -r 48000 -c 2 -f S16_LE /home/root/s16_le.wav -d 10 For the reasons explained above 24bit and 32bit captures were previously broken. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Kind regards, Andrea Bondavalli
On Mon, Oct 30, 2017 at 01:16:03PM +0100, Andrea Bondavalli wrote: > Hello Maxime, everyone, > > see comments inline. > > On Mon, Oct 30, 2017 at 09:55:42AM +0100, Maxime Ripard wrote: > > On Sun, Oct 29, 2017 at 02:41:01AM -0700, Andrea Bondavalli wrote: > > > Fixed support for 32bit audio capture for Allwinner H3/H2+ SoC > > > > > > Signed-off-by: Andrea Bondavalli <andrea.bondavalli74@gmail.com> > > > > A more detailed commit log would be welcome. > > > > What are the issues involved would be the more valuable information, > > and then how you'ring fixing it and why would be great. > > > > H3 audio codec supports 16 and 24 bits capture formats (same for the playback). > Switching between these two modes can be achieved by using the RX_SAMPLE_BITS (bit 6) > of the AC_ADC_FIFOC register (offset 0x10) of the audio codec. > The RX_FIFO_MODE (bit 24) register must also be set accordingly to have > the desired RX FIFO output mode. > > The current implementation declares support for 16bit and 32bit (with a > resolution of 24bit) capture formats but the 32bit implementation is broken > because the RX_SAMPLE_BITS and the RX_FIFO_MODE bits are not set correctly and > in addition to this the DMA bus transfer width is left to 2 bytes instead of > 4 bytes causing invalid captures whose durating is double than the expected period. > > The following patch sets the H3 audio codec registers properly when a > 24/32bit capture is requested. > Similar approach was already implemented for the audio playback part. Thanks. Can you put that in your commit log and resend the patch? Maxime
diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c index baa9007..f40fa34 100644 --- a/sound/soc/sunxi/sun4i-codec.c +++ b/sound/soc/sunxi/sun4i-codec.c @@ -346,11 +346,6 @@ static int sun4i_codec_prepare_capture(struct snd_pcm_substream *substream, 0x3 << 8, 0x1 << 8); - /* Fill most significant bits with valid data MSB */ - regmap_field_update_bits(scodec->reg_adc_fifoc, - BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), - BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE)); - return 0; } @@ -490,6 +485,30 @@ static int sun4i_codec_hw_params_capture(struct sun4i_codec *scodec, BIT(SUN4I_CODEC_ADC_FIFOC_MONO_EN), 0); + /* Set the number of sample bits to either 16 or 24 bits */ + if (hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS)->min == 32) { + regmap_field_update_bits(scodec->reg_adc_fifoc, + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS), + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS)); + + regmap_field_update_bits(scodec->reg_adc_fifoc, + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), + 0); + + scodec->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + } else { + regmap_field_update_bits(scodec->reg_adc_fifoc, + BIT(SUN4I_CODEC_ADC_FIFOC_RX_SAMPLE_BITS), + 0); + + /* Fill most significant bits with valid data MSB */ + regmap_field_update_bits(scodec->reg_adc_fifoc, + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE), + BIT(SUN4I_CODEC_ADC_FIFOC_RX_FIFO_MODE)); + + scodec->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; + } + return 0; }
Fixed support for 32bit audio capture for Allwinner H3/H2+ SoC Signed-off-by: Andrea Bondavalli <andrea.bondavalli74@gmail.com> --- sound/soc/sunxi/sun4i-codec.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)