ASoC: sun4i-codec: fixed 32bit audio capture support for H3/H2+

Message ID 20171029094048.GA5295@ubuntu
State New
Headers show
Series
  • ASoC: sun4i-codec: fixed 32bit audio capture support for H3/H2+
Related show

Commit Message

Andrea Bondavalli Oct. 29, 2017, 9:41 a.m.
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(-)

Comments

Maxime Ripard Oct. 30, 2017, 8:55 a.m. | #1
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
Andrea Bondavalli Oct. 30, 2017, 12:16 p.m. | #2
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
Maxime Ripard Oct. 31, 2017, 10:19 a.m. | #3
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

Patch

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;
 }