On 24.07.2019 14:15, mirq-linux@xxxxxxxxxxxx wrote:
> External E-Mail
>
>
> On Wed, Jul 24, 2019 at 10:35:29AM +0000, Codrin.Ciubotariu@xxxxxxxxxxxxx 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@xxxxxxxxxxxx>
>>> ---
>>> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
