Re: [PATCH 2/5] ASoC: atmel_ssc_dai: rework DAI format configuration

[<Codrin.Ciubotariu@microchip.com>]

On 24.07.2019 14:15, mirq-linux@rere.qmqm.pl wrote:
> External E-Mail
> 
> 
> On Wed, Jul 24, 2019 at 10:35:29AM +0000, Codrin.Ciubotariu@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@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.

Right, you can leave it then.

> 
>>> +
>>> +	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?

I prefer a separate patch, for clarity mostly, but I don't have a strong 
opinion on this. Later, it might prove trickier to investigate a bug 
this case and review this patch. Also, we should test and see that this 
format works indeed...

Best regards,
Codrin