[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

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 */
>   

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

[Matt Flax]

On 27/3/20 10:56 am, Matt Flax wrote:
>
> Should this patch be handled through the ALSA team the R. Pi team or 
> the BCM team ?
>

Resending again with reduced recipients.


>
> 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 */

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

[Matthias Reichl]

On Fri, Mar 27, 2020 at 11:30:50AM +1100, Matt Flax wrote:
> 
> On 27/3/20 10:56 am, Matt Flax wrote:
> > 
> > Should this patch be handled through the ALSA team the R. Pi team or the
> > BCM team ?
> > 
> 
> Resending again with reduced recipients.
> 
> 
> > 
> > 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 */

This will break duplex operation if a second stream is opened when
a stream is already running as the channel position registers for
the second stream haven't been set up.

Note this code at the very beginning of hw_params:

        /*
         * If a stream is already enabled,
         * the registers are already set properly.
         */
        regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);

        if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
                return 0;

The reason for this check is that we can't change bcm2835 I2S registers
after I2S RX/TX has been enabled - the reason why is explained in the
datasheet:

> The PCM interface runs asynchronously at the PCM_CLK rate and
> automatically transfers transmit and receive data across to the
> internal APB clock domain. The control registers are NOT
> synchronised and should be programmed before the device is enabled
> and should NOT be changed whilst the interface is running.
> 
> Only the EN, RXON and TXON bits of the PCMCS register are synchronised
> across the PCM - APB clock domain and are allowed to be changed whilst
> the interface is running.

Therefore we need to set up channel masks for both RX and TX before
any stream is started.

so long,

Hias

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

[Matt Flax]

On 28/3/20 12:23 am, Matthias Reichl wrote:
> On Fri, Mar 27, 2020 at 11:30:50AM +1100, Matt Flax wrote:
>> On 27/3/20 10:56 am, Matt Flax wrote:
>>> Should this patch be handled through the ALSA team the R. Pi team or the
>>> BCM team ?
>>>
>> Resending again with reduced recipients.
>>
>>
>>> 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 */
> This will break duplex operation if a second stream is opened when
> a stream is already running as the channel position registers for
> the second stream haven't been set up.
>
> Note this code at the very beginning of hw_params:
>
>          /*
>           * If a stream is already enabled,
>           * the registers are already set properly.
>           */
>          regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
>
>          if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
>                  return 0;
>
> The reason for this check is that we can't change bcm2835 I2S registers
> after I2S RX/TX has been enabled - the reason why is explained in the
> datasheet:
>
>> The PCM interface runs asynchronously at the PCM_CLK rate and
>> automatically transfers transmit and receive data across to the
>> internal APB clock domain. The control registers are NOT
>> synchronised and should be programmed before the device is enabled
>> and should NOT be changed whilst the interface is running.
>>
>> Only the EN, RXON and TXON bits of the PCMCS register are synchronised
>> across the PCM - APB clock domain and are allowed to be changed whilst
>> the interface is running.
> Therefore we need to set up channel masks for both RX and TX before
> any stream is started.


I see what you mean. We can't change the registers once the system has 
started half duplex and then subsequently changed to full duplex.

There are cases however where playback and capture need to be set 
independently. In these cases the machine driver requires different 
format settings based on the stream direction.

What if we make a check for whether the system is already running and in 
that case return an error - forcing the user to use specify the same 
dai_fmt which is already in use before continuing ?

Would there be a better way to achieve different hwparams based on 
stream direction ?

Matt

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

[Matthias Reichl]

On Sat, Mar 28, 2020 at 08:50:52AM +1100, Matt Flax wrote:
> 
> On 28/3/20 12:23 am, Matthias Reichl wrote:
> > On Fri, Mar 27, 2020 at 11:30:50AM +1100, Matt Flax wrote:
> > > On 27/3/20 10:56 am, Matt Flax wrote:
> > > > Should this patch be handled through the ALSA team the R. Pi team or the
> > > > BCM team ?
> > > > 
> > > Resending again with reduced recipients.
> > > 
> > > 
> > > > 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 */
> > This will break duplex operation if a second stream is opened when
> > a stream is already running as the channel position registers for
> > the second stream haven't been set up.
> > 
> > Note this code at the very beginning of hw_params:
> > 
> >          /*
> >           * If a stream is already enabled,
> >           * the registers are already set properly.
> >           */
> >          regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
> > 
> >          if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
> >                  return 0;
> > 
> > The reason for this check is that we can't change bcm2835 I2S registers
> > after I2S RX/TX has been enabled - the reason why is explained in the
> > datasheet:
> > 
> > > The PCM interface runs asynchronously at the PCM_CLK rate and
> > > automatically transfers transmit and receive data across to the
> > > internal APB clock domain. The control registers are NOT
> > > synchronised and should be programmed before the device is enabled
> > > and should NOT be changed whilst the interface is running.
> > > 
> > > Only the EN, RXON and TXON bits of the PCMCS register are synchronised
> > > across the PCM - APB clock domain and are allowed to be changed whilst
> > > the interface is running.
> > Therefore we need to set up channel masks for both RX and TX before
> > any stream is started.
> 
> 
> I see what you mean. We can't change the registers once the system has
> started half duplex and then subsequently changed to full duplex.
> 
> There are cases however where playback and capture need to be set
> independently. In these cases the machine driver requires different format
> settings based on the stream direction.
> 
> What if we make a check for whether the system is already running and in
> that case return an error - forcing the user to use specify the same dai_fmt
> which is already in use before continuing ?

I'm not sure if I can follow you. dai_fmt, as the name implies, sets
the format of the DAI - you can't have different DAI formats for
playback/capture active at the same time.

This sounds a bit like you may be trying to work around some hardware
or codec configuration issue by creative use of the API.

> Would there be a better way to achieve different hwparams based on stream
> direction ?

If you really need different DAI formats for playback/capture it's
best to disallow full-duplex mode and set the DAI format based on
stream direction in the machine driver.

so long,

Hias

> 
> Matt
> 

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

[Matt Flax]

On 28/3/20 10:59 pm, Matthias Reichl wrote:
> On Sat, Mar 28, 2020 at 08:50:52AM +1100, Matt Flax wrote:
>> On 28/3/20 12:23 am, Matthias Reichl wrote:
>>> On Fri, Mar 27, 2020 at 11:30:50AM +1100, Matt Flax wrote:
>>>> On 27/3/20 10:56 am, Matt Flax wrote:
>>>>> Should this patch be handled through the ALSA team the R. Pi team or the
>>>>> BCM team ?
>>>>>
>>>> Resending again with reduced recipients.
>>>>
>>>>
>>>>> 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 */
>>> This will break duplex operation if a second stream is opened when
>>> a stream is already running as the channel position registers for
>>> the second stream haven't been set up.
>>>
>>> Note this code at the very beginning of hw_params:
>>>
>>>           /*
>>>            * If a stream is already enabled,
>>>            * the registers are already set properly.
>>>            */
>>>           regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
>>>
>>>           if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
>>>                   return 0;
>>>
>>> The reason for this check is that we can't change bcm2835 I2S registers
>>> after I2S RX/TX has been enabled - the reason why is explained in the
>>> datasheet:
>>>
>>>> The PCM interface runs asynchronously at the PCM_CLK rate and
>>>> automatically transfers transmit and receive data across to the
>>>> internal APB clock domain. The control registers are NOT
>>>> synchronised and should be programmed before the device is enabled
>>>> and should NOT be changed whilst the interface is running.
>>>>
>>>> Only the EN, RXON and TXON bits of the PCMCS register are synchronised
>>>> across the PCM - APB clock domain and are allowed to be changed whilst
>>>> the interface is running.
>>> Therefore we need to set up channel masks for both RX and TX before
>>> any stream is started.
>>
>> I see what you mean. We can't change the registers once the system has
>> started half duplex and then subsequently changed to full duplex.
>>
>> There are cases however where playback and capture need to be set
>> independently. In these cases the machine driver requires different format
>> settings based on the stream direction.
>>
>> What if we make a check for whether the system is already running and in
>> that case return an error - forcing the user to use specify the same dai_fmt
>> which is already in use before continuing ?
> I'm not sure if I can follow you. dai_fmt, as the name implies, sets
> the format of the DAI - you can't have different DAI formats for
> playback/capture active at the same time.
>
> This sounds a bit like you may be trying to work around some hardware
> or codec configuration issue by creative use of the API.


It is the nature of digital isolation chips. They have very significant 
latencies. If the codec is master, then the round trip latency which 
effects the DAC's I2S line is even more significant.


>> Would there be a better way to achieve different hwparams based on stream
>> direction ?
> If you really need different DAI formats for playback/capture it's
> best to disallow full-duplex mode and set the DAI format based on
> stream direction in the machine driver.


If we were to disallow full duplex mode in the machine driver, how would 
it look in the machine driver ? Would the user still be able to do full 
duplex capture/playback ?

We could take that approach if the user could still perform full duplex 
operations. However it doesn't represent the hardware. The hardware is 
in full duplex mode and requires different word offsets for ADC and DAC 
I2S lines.

It seems that the ALSA core system can't handle this because the set_fmt 
functions don't specify the stream direction. This happens both for the 
CPU and the Codec drivers.


Matt

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

[Matthias Reichl]

On Sun, Mar 29, 2020 at 10:47:47AM +1100, Matt Flax wrote:
> 
> On 28/3/20 10:59 pm, Matthias Reichl wrote:
> > On Sat, Mar 28, 2020 at 08:50:52AM +1100, Matt Flax wrote:
> > > On 28/3/20 12:23 am, Matthias Reichl wrote:
> > > > On Fri, Mar 27, 2020 at 11:30:50AM +1100, Matt Flax wrote:
> > > > > On 27/3/20 10:56 am, Matt Flax wrote:
> > > > > > Should this patch be handled through the ALSA team the R. Pi team or the
> > > > > > BCM team ?
> > > > > > 
> > > > > Resending again with reduced recipients.
> > > > > 
> > > > > 
> > > > > > 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 */
> > > > This will break duplex operation if a second stream is opened when
> > > > a stream is already running as the channel position registers for
> > > > the second stream haven't been set up.
> > > > 
> > > > Note this code at the very beginning of hw_params:
> > > > 
> > > >           /*
> > > >            * If a stream is already enabled,
> > > >            * the registers are already set properly.
> > > >            */
> > > >           regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
> > > > 
> > > >           if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
> > > >                   return 0;
> > > > 
> > > > The reason for this check is that we can't change bcm2835 I2S registers
> > > > after I2S RX/TX has been enabled - the reason why is explained in the
> > > > datasheet:
> > > > 
> > > > > The PCM interface runs asynchronously at the PCM_CLK rate and
> > > > > automatically transfers transmit and receive data across to the
> > > > > internal APB clock domain. The control registers are NOT
> > > > > synchronised and should be programmed before the device is enabled
> > > > > and should NOT be changed whilst the interface is running.
> > > > > 
> > > > > Only the EN, RXON and TXON bits of the PCMCS register are synchronised
> > > > > across the PCM - APB clock domain and are allowed to be changed whilst
> > > > > the interface is running.
> > > > Therefore we need to set up channel masks for both RX and TX before
> > > > any stream is started.
> > > 
> > > I see what you mean. We can't change the registers once the system has
> > > started half duplex and then subsequently changed to full duplex.
> > > 
> > > There are cases however where playback and capture need to be set
> > > independently. In these cases the machine driver requires different format
> > > settings based on the stream direction.
> > > 
> > > What if we make a check for whether the system is already running and in
> > > that case return an error - forcing the user to use specify the same dai_fmt
> > > which is already in use before continuing ?
> > I'm not sure if I can follow you. dai_fmt, as the name implies, sets
> > the format of the DAI - you can't have different DAI formats for
> > playback/capture active at the same time.
> > 
> > This sounds a bit like you may be trying to work around some hardware
> > or codec configuration issue by creative use of the API.
> 
> 
> It is the nature of digital isolation chips. They have very significant
> latencies. If the codec is master, then the round trip latency which effects
> the DAC's I2S line is even more significant.

This is something you need to fix at the hardware level, you have to
make sure the signals adhere to the I2S timing requirements. Trying
to work around that with software is the wrong approach.

You probably need to use faster optocouplers or whatever you are using
for isolation.

> > > Would there be a better way to achieve different hwparams based on stream
> > > direction ?
> > If you really need different DAI formats for playback/capture it's
> > best to disallow full-duplex mode and set the DAI format based on
> > stream direction in the machine driver.
> 
> 
> If we were to disallow full duplex mode in the machine driver, how would it
> look in the machine driver ? Would the user still be able to do full duplex
> capture/playback ?

No.

so long,

Hias

> 
> We could take that approach if the user could still perform full duplex
> operations. However it doesn't represent the hardware. The hardware is in
> full duplex mode and requires different word offsets for ADC and DAC I2S
> lines.
> 
> It seems that the ALSA core system can't handle this because the set_fmt
> functions don't specify the stream direction. This happens both for the CPU
> and the Codec drivers.
> 
> 
> Matt
> 

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

[Mark Brown]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]