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