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@xxxxxxxxxxx>
> > > ---
> > > 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
