[alsa-devel] [PATCH 2/5] ASoC: atmel_ssc_dai: rework DAI format configuration

[mirq-linux at rere.qmqm.pl]

On Wed, Jul 24, 2019 at 10:35:29AM +0000, Codrin.Ciubotariu at microchip.com wrote:
> On 22.07.2019 21:27, Michał Mirosław wrote:
> > Rework DAI format calculation in preparation for adding more formats
> > later.
> > 
> > Note: this changes FSEDGE to POSITIVE for I2S CBM_CFS mode as the TXSYN
> > interrupt is not used anyway.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux at rere.qmqm.pl>
> > ---
> >   sound/soc/atmel/atmel_ssc_dai.c | 283 +++++++++-----------------------
> >   1 file changed, 79 insertions(+), 204 deletions(-)
> > 
> > diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> > index 6f89483ac88c..b2992496e52f 100644
> > --- a/sound/soc/atmel/atmel_ssc_dai.c
> > +++ b/sound/soc/atmel/atmel_ssc_dai.c
[...]
> > +	if (atmel_ssc_cbs(ssc_p)) {
> > +		/*
> > +		 * SSC provides BCLK
> > +		 *
> > +		 * The SSC transmit and receive clocks are generated from the
> > +		 * MCK divider, and the BCLK signal is output
> > +		 * on the SSC TK line.
> > +		 */
> > +		rcmr |=	  SSC_BF(RCMR_CKS, SSC_CKS_DIV)
> > +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
> > +
> > +		tcmr |=	  SSC_BF(TCMR_CKS, SSC_CKS_DIV)
> > +			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS);
> > +	} else {
> > +		rcmr |=	  SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> > +					SSC_CKS_PIN : SSC_CKS_CLOCK)
> > +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
> > +
> > +		tcmr |=	  SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
> > +					SSC_CKS_CLOCK : SSC_CKS_PIN)
> > +			| SSC_BF(TCMR_CKO, SSC_CKO_NONE);
> > +	}
> > +
> > +	rcmr |=	  SSC_BF(RCMR_PERIOD, rcmr_period)
> > +		| SSC_BF(RCMR_CKI, SSC_CKI_RISING);
> 
> You can also add here SSC_BF(RCMR_CKO, SSC_CKO_NONE) and remove it from 
> the if-else above;

I left it to keep symmetry between TX and RX code. I can pull it here if
you prefer that way.

> > +
> > +	tcmr |=   SSC_BF(TCMR_PERIOD, tcmr_period)
> > +		| SSC_BF(TCMR_CKI, SSC_CKI_FALLING);
> > +
> > +	rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
> > +		| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> > +		| SSC_BF(RFMR_FSOS, fs_osync)
> > +		| SSC_BF(RFMR_FSLEN, fslen)
> > +		| SSC_BF(RFMR_DATNB, (channels - 1))
> > +		| SSC_BIT(RFMR_MSBF)
> > +		| SSC_BF(RFMR_LOOP, 0)
> > +		| SSC_BF(RFMR_DATLEN, (bits - 1));
> > +
> > +	tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
> > +		| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> > +		| SSC_BF(TFMR_FSDEN, 0)
> > +		| SSC_BF(TFMR_FSOS, fs_osync)
> > +		| SSC_BF(TFMR_FSLEN, fslen)
> > +		| SSC_BF(TFMR_DATNB, (channels - 1))
> > +		| SSC_BIT(TFMR_MSBF)
> > +		| SSC_BF(TFMR_DATDEF, 0)
> > +		| SSC_BF(TFMR_DATLEN, (bits - 1));
> > +
> > +	if (fslen_ext && !ssc->pdata->has_fslen_ext) {
> > +		dev_err(dai->dev, "sample size %d is too large for SSC device\n",
> > +			bits);
> > +		return -EINVAL;
> > +	}
> > +
> >   	pr_debug("atmel_ssc_hw_params: "
> >   			"RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n",
> >   			rcmr, rfmr, tcmr, tfmr);
> > 
> 
> You are adding support for SND_SOC_DAIFMT_DSP_A | 
> SND_SOC_DAIFMT_CBM_CFS. If this is intended, please make a separate 
> patch. If not, then:
> 
> printk(KERN_WARNING "atmel_ssc_dai: unsupported DAI format 0x%x\n",
> 			ssc_p->daifmt);
> return -EINVAL;

Hmm. I guess this is actually a good side effect. I can't see a way to
contain this change that doesn't involve adding code that's immediately
removed in next patch. So would you agree to just mentioning this in
commit message?

Best Regards,
Michał Mirosław