[PATCH] ASoC: bcm2835-i2s: substream alignment now independent in hwparams

[PATCH] ASoC: bcm2835-i2s: substream alignment now independent in hwparams

[Matt Flax]

Substream sample alignment was being set in hwparams for both
substreams at the same time. This became a problem when	the Audio
Injector isolated sound card needed to offset sample alignment
for high sample	rates. The latency difference between playback
and capture occurs because of the digital isolation chip
propagation time, particularly when the codec is master and
the DAC return is twice delayed.

This patch sets sample alignment registers  based on the substream
direction in hwparams. This gives the machine driver more control
over sample alignment in the bcm2835 i2s driver.

Signed-off-by: Matt Flax <flatmax@flatmax.org>
---
 sound/soc/bcm/bcm2835-i2s.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
index e6a12e271b07..9db542699a13 100644
--- a/sound/soc/bcm/bcm2835-i2s.c
+++ b/sound/soc/bcm/bcm2835-i2s.c
@@ -493,11 +493,6 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-	bcm2835_i2s_calc_channel_pos(&rx_ch1_pos, &rx_ch2_pos,
-		rx_mask, slot_width, data_delay, odd_slot_offset);
-	bcm2835_i2s_calc_channel_pos(&tx_ch1_pos, &tx_ch2_pos,
-		tx_mask, slot_width, data_delay, odd_slot_offset);
-
 	/*
 	 * Transmitting data immediately after frame start, eg
 	 * in left-justified or DSP mode A, only works stable
@@ -508,19 +503,26 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
 			"Unstable slave config detected, L/R may be swapped");
 
 	/*
-	 * Set format for both streams.
-	 * We cannot set another frame length
-	 * (and therefore word length) anyway,
-	 * so the format will be the same.
+	 * Set format on a per stream basis.
+	 * The alignment format can be different depending on direction.
 	 */
-	regmap_write(dev->i2s_regmap, BCM2835_I2S_RXC_A_REG, 
-		  format
-		| BCM2835_I2S_CH1_POS(rx_ch1_pos)
-		| BCM2835_I2S_CH2_POS(rx_ch2_pos));
-	regmap_write(dev->i2s_regmap, BCM2835_I2S_TXC_A_REG, 
-		  format
-		| BCM2835_I2S_CH1_POS(tx_ch1_pos)
-		| BCM2835_I2S_CH2_POS(tx_ch2_pos));
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		bcm2835_i2s_calc_channel_pos(&rx_ch1_pos, &rx_ch2_pos,
+			rx_mask, slot_width, data_delay, odd_slot_offset);
+		regmap_write(dev->i2s_regmap, BCM2835_I2S_RXC_A_REG,
+			  format
+			| BCM2835_I2S_CH1_POS(rx_ch1_pos)
+			| BCM2835_I2S_CH2_POS(rx_ch2_pos));
+	}
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		bcm2835_i2s_calc_channel_pos(&tx_ch1_pos, &tx_ch2_pos,
+			tx_mask, slot_width, data_delay, odd_slot_offset);
+		regmap_write(dev->i2s_regmap, BCM2835_I2S_TXC_A_REG,
+			  format
+			| BCM2835_I2S_CH1_POS(tx_ch1_pos)
+			| BCM2835_I2S_CH2_POS(tx_ch2_pos));
+	}
 
 	/* Setup the I2S mode */
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ASoC: bcm2835-i2s: substream alignment now independent in hwparams

[Matt Flax]

Should this patch be handled through the ALSA team the R. Pi team or the 
BCM team ?


thanks

Matt

On 24/3/20 8:08 pm, Matt Flax wrote:
> Substream sample alignment was being set in hwparams for both
> substreams at the same time. This became a problem when	the Audio
> Injector isolated sound card needed to offset sample alignment
> for high sample	rates. The latency difference between playback
> and capture occurs because of the digital isolation chip
> propagation time, particularly when the codec is master and
> the DAC return is twice delayed.
>
> This patch sets sample alignment registers  based on the substream
> direction in hwparams. This gives the machine driver more control
> over sample alignment in the bcm2835 i2s driver.
>
> Signed-off-by: Matt Flax <flatmax@flatmax.org>
> ---
>   sound/soc/bcm/bcm2835-i2s.c | 36 +++++++++++++++++++-----------------
>   1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
> index e6a12e271b07..9db542699a13 100644
> --- a/sound/soc/bcm/bcm2835-i2s.c
> +++ b/sound/soc/bcm/bcm2835-i2s.c
> @@ -493,11 +493,6 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
>   		return -EINVAL;
>   	}
>   
> -	bcm2835_i2s_calc_channel_pos(&rx_ch1_pos, &rx_ch2_pos,
> -		rx_mask, slot_width, data_delay, odd_slot_offset);
> -	bcm2835_i2s_calc_channel_pos(&tx_ch1_pos, &tx_ch2_pos,
> -		tx_mask, slot_width, data_delay, odd_slot_offset);
> -
>   	/*
>   	 * Transmitting data immediately after frame start, eg
>   	 * in left-justified or DSP mode A, only works stable
> @@ -508,19 +503,26 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
>   			"Unstable slave config detected, L/R may be swapped");
>   
>   	/*
> -	 * Set format for both streams.
> -	 * We cannot set another frame length
> -	 * (and therefore word length) anyway,
> -	 * so the format will be the same.
> +	 * Set format on a per stream basis.
> +	 * The alignment format can be different depending on direction.
>   	 */
> -	regmap_write(dev->i2s_regmap, BCM2835_I2S_RXC_A_REG,
> -		  format
> -		| BCM2835_I2S_CH1_POS(rx_ch1_pos)
> -		| BCM2835_I2S_CH2_POS(rx_ch2_pos));
> -	regmap_write(dev->i2s_regmap, BCM2835_I2S_TXC_A_REG,
> -		  format
> -		| BCM2835_I2S_CH1_POS(tx_ch1_pos)
> -		| BCM2835_I2S_CH2_POS(tx_ch2_pos));
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		bcm2835_i2s_calc_channel_pos(&rx_ch1_pos, &rx_ch2_pos,
> +			rx_mask, slot_width, data_delay, odd_slot_offset);
> +		regmap_write(dev->i2s_regmap, BCM2835_I2S_RXC_A_REG,
> +			  format
> +			| BCM2835_I2S_CH1_POS(rx_ch1_pos)
> +			| BCM2835_I2S_CH2_POS(rx_ch2_pos));
> +	}
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		bcm2835_i2s_calc_channel_pos(&tx_ch1_pos, &tx_ch2_pos,
> +			tx_mask, slot_width, data_delay, odd_slot_offset);
> +		regmap_write(dev->i2s_regmap, BCM2835_I2S_TXC_A_REG,
> +			  format
> +			| BCM2835_I2S_CH1_POS(tx_ch1_pos)
> +			| BCM2835_I2S_CH2_POS(tx_ch2_pos));
> +	}
>   
>   	/* Setup the I2S mode */
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ASoC: bcm2835-i2s: substream alignment now independent in hwparams

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 919 bytes --]

On Fri, Mar 27, 2020 at 10:56:50AM +1100, Matt Flax wrote:
> 
> Should this patch be handled through the ALSA team the R. Pi team or the BCM
> team ?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel