Re: [alsa-devel] [PATCH 1/8] ASoC: tegra: Add a TDM configuration callback

[Jon Hunter <jonathanh@nvidia.com>]

On 18/09/2019 11:11, Ben Dooks wrote:
> 
> 
> On 2019-09-18 09:42, Jon Hunter wrote:
>> On 17/09/2019 19:12, Ben Dooks wrote:
>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>
>>> Add a callback to configure TDM settings for the Tegra30 I2S ASoC
>>> 'platform'
>>> driver.
>>>
>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>> ---
>>>  sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>
>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>> b/sound/soc/tegra/tegra30_i2s.c
>>> index ac6983c6bd72..d36b4662b420 100644
>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>> @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct
>>> snd_pcm_substream *substream, int cmd,
>>>      return 0;
>>>  }
>>>
>>> +/*
>>> + * Set up TDM
>>> + */
>>> +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,
>>> +                   unsigned int tx_mask, unsigned int rx_mask,
>>> +                   int slots, int slot_width)
>>> +{
>>> +    struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>> +    unsigned int mask = 0, val = 0;
>>> +
>>> +    dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask:
>>> 0x%08x "
>>
>> dev_dbg() please. Also I don't think it is necessary to print both the
>> function name and 'setting TDM', the function name should be sufficient.
> 
> Yes, already sorted from previous review.
> 
>>> +         "slots: 0x%08x " "width: %d\n",
>>
>> Why are there extra quotes here?
> 
> No idea, I'll remove these later.
> 
>>> +         __func__, tx_mask, rx_mask, slots, slot_width)> +
>>> +    /* Set up slots and tx/rx masks */
>>> +    mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |
>>> +           TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |
>>> +           TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;
>>> +
>>> +    val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |
>>> +          (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |
>>> +          ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
>>> +
>>> +    regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
>>> +
>>> +    /* Set FSYNC width */
>>> +    regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL,
>>> +        TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK,
>>> +        (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);
>>
>> What happens if there is only one slot? The fsync will be the width of
>> the slot. Typically, TDM is used with DSP-A/B formats and although the
>> driver does not appear to program the fsync width, it probably should
>> during the tegra30_i2s_set_fmt() depending on the format used rather
>> than here.
> 
> Hmm, should we check.
> 
> The work was done to keep as close to the original client's 2.6 kernel
> as possible which set the fsync field to a whole data word. We could try
> and just set this to say 2 here and have a much shorter frame-sync pulse.
> 
> I'll add a check for slots < 2 and set the fsync width to 2.

Why 2? From looking at various codecs that support dsp-a/b modes, it is
more common for the f-sync to be 1 regardless of the number of slots.

Jon
-- 
nvpublic
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel