On Wed, Nov 22, 2017 at 12:54:26AM +0100, Maciej S. Szmigiero wrote:
> We need to make sure that only proper channel slots (in SACCST register)
> are enabled at playback start time since some AC'97 CODECs (like VT1613 on
> UDOO board) were observed requesting via SLOTREQ spurious ones just after
> an AC'97 link is started but before the CODEC is configured by its driver.
> When a bit for some channel slot is set in a SLOTREQ request then SSI sets
> the relevant bit in SACCST automatically, which then 'sticks' until it is
> manually unset.
> The SACCST register is not writable directly, we have to use SACCDIS and
> SACCEN registers to configure it instead (these aren't normal registers:
> writing a '1' bit at some position in SACCEN sets the relevant bit in
> SACCST; SACCDIS operates in a similar way but allows unsetting bits in
> SACCST).
>
> Theoretically, this should be necessary only for the very first playback
> but since some CODECs are so untrustworthy and extra channel slots enabled
> mean ruined playback let's play safe here and make sure that no extra
> slots are enabled in SACCST every time a playback is started.
>
> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
The inline comments feel over descriptive but not critical. Anyway,
I plan to do some clean up to this driver after all pending changes
get finalized. So,
Acked-by: Nicolin Chen <nicoleots...@gmail.com>
> ---
> Changes from v1: Split out this part from
> "fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode" commit,
> describe the problem and its solution better both in the commit message and
> in the code, move the SACCST setup code into a separate function and call
> it from TX config instead of doing it from trigger handler function.
>
> sound/soc/fsl/fsl_ssi.c | 52
> +++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 48bb850a34d9..375aaaf6080d 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -574,8 +574,54 @@ static void fsl_ssi_rx_config(struct fsl_ssi_private
> *ssi_private, bool enable)
> fsl_ssi_config(ssi_private, enable, &ssi_private->rxtx_reg_val.rx);
> }
>
> +static void fsl_ssi_tx_ac97_saccst_setup(struct fsl_ssi_private *ssi_private)
> +{
> + struct regmap *regs = ssi_private->regs;
> +
> + /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
> + if (!ssi_private->soc->imx21regs) {
> + /*
> + * Note that these below aren't just normal registers.
> + * They are a way to disable or enable bits in SACCST
> + * register:
> + * - writing a '1' bit at some position in SACCEN sets the
> + * relevant bit in SACCST,
> + * - writing a '1' bit at some position in SACCDIS unsets
> + * the relevant bit in SACCST register.
> + *
> + * The two writes below first disable all channels slots,
> + * then enable just slots 3 & 4 ("PCM Playback Left Channel"
> + * and "PCM Playback Right Channel").
> + */
> + regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
> + regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
> + }
> +}
> +
> static void fsl_ssi_tx_config(struct fsl_ssi_private *ssi_private, bool
> enable)
> {
> + /*
> + * Why are we setting up SACCST everytime we are starting a
> + * playback?
> + * Some CODECs (like VT1613 CODEC on UDOO board) like to
> + * (sometimes) set extra bits in their SLOTREQ requests.
> + * When a bit is set in a SLOTREQ request then SSI sets the
> + * relevant bit in SACCST automatically (it is enough if a bit was
> + * set in a SLOTREQ just once, bits in SACCST are 'sticky').
> + * If an extra slot gets enabled that's a disaster for playback
> + * because some of normal left or right channel samples are
> + * redirected instead to this extra slot.
> + *
> + * A workaround implemented in fsl-asoc-card of setting an
> + * appropriate CODEC register so that slots 3 & 4 (the normal
> + * stereo playback slots) are used for S/PDIF seems to mostly fix
> + * this issue on the UDOO board but since this CODEC is so
> + * untrustworthy let's play safe here and make sure that no extra
> + * slots are enabled every time a playback is started.
> + */
> + if (enable && fsl_ssi_is_ac97(ssi_private))
> + fsl_ssi_tx_ac97_saccst_setup(ssi_private);
> +
> fsl_ssi_config(ssi_private, enable, &ssi_private->rxtx_reg_val.tx);
> }
>
> @@ -630,12 +676,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private
> *ssi_private)
> regmap_write(regs, CCSR_SSI_SACNT,
> CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
>
> - /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
> - if (!ssi_private->soc->imx21regs) {
> - regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
> - regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
> - }
> -
> /*
> * Enable SSI, Transmit and Receive. AC97 has to communicate with the
> * codec before a stream is started.
>
> _______________________________________________
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
