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@xxxxxxxxxxxxxxx>
>>>
>>> Add a callback to configure TDM settings for the Tegra30 I2S ASoC
>>> 'platform'
>>> driver.
>>>
>>> Signed-off-by: Edward Cragg <edward.cragg@xxxxxxxxxxxxxxx>
>>> ---
>>> 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
