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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel