Re: [PATCH] ASoC: fsl_sai: Clean code for synchronize mode

On Tue, Aug 4, 2020 at 5:57 AM Nicolin Chen [off-list ref] wrote:

On Mon, Aug 03, 2020 at 04:04:23PM +0800, Shengjiu Wang wrote:

quoted

quoted

quoted

clock generation. The TCSR.TE is no need to enabled when only RX
is enabled.

You are correct if there's only RX running without TX joining.
However, that's something we can't guarantee. Then we'd enable
TE after RE is enabled, which is against what RM recommends:

# From 54.3.3.1 Synchronous mode in IMX6SXRM
# If the receiver bit clock and frame sync are to be used by
# both the transmitter and receiver, it is recommended that
# the receiver is the last enabled and the first disabled.

I remember I did this "ugly" design by strictly following what
RM says. If hardware team has updated the RM or removed this
limitation, please quote in the commit logs.

There is no change in RM and same recommandation.

My change does not violate the RM. The direction which generates
the clock is still last enabled.

Using Tx syncing with Rx clock for example,
T1: arecord (non-stop) => set RE
T2: aplay => set TE then RE (but RE is already set at T1)

Anything that I am missing?

This is a good example.
We have used this change locally for a long time, so I think it is
safe to do this change, a little different with the recommandation.

quoted

quoted

quoted

+             if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
+                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
+                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
+             } else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
+                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
+                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);

Two identical regmap_update_bits calls -- both on !tx (RX?)

The content for regmap_update_bits is the same, but the precondition
is different.
The first one is for tx=false and enable TCSR.TE. (TX generate clock)
The second one is for tx=true and enable RSCR.RE (RX generate clock)

Why not merge them?

+               if ((!sai->synchronous[TX] && sai->synchronous[RX] && !tx) ||
+                  ((!sai->synchronous[RX] && sai->synchronous[TX] && tx) {

oh, yes, good point!

best regards
wang shengjiu