| Message ID | 20180727125931.9794-3-jorge.sanjuan@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Tegra30 TDM support | expand |
On Fri, Jul 27, 2018 at 01:59:29PM +0100, Jorge Sanjuan 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: Ben Dooks <ben.dooks@codethink.co.uk> > Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> This says it was britten by Edward but there's a signoff from Ben before his? > + dev_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x" > + "slots: 0x%08x width: %d\n", > + __func__, tx_mask, rx_mask, slots, slot_width); Please don't split log messages over lines, it makes it harder to grep for them. Just use a long line. I'm also not seeing any validation of the parameters?
On Mon, Jul 30, 2018 at 09:49:08AM +0100, Mark Brown wrote: > On Fri, Jul 27, 2018 at 01:59:29PM +0100, Jorge Sanjuan 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: Ben Dooks <ben.dooks@codethink.co.uk> > > Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> > > This says it was britten by Edward but there's a signoff from Ben before > his? Editing accdient, I was originally going to submit this series. > > + dev_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x" > > + "slots: 0x%08x width: %d\n", > > + __func__, tx_mask, rx_mask, slots, slot_width); > > Please don't split log messages over lines, it makes it harder to grep > for them. Just use a long line. > > I'm also not seeing any validation of the parameters? > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 27/07/18 13:59, Jorge Sanjuan 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: Ben Dooks <ben.dooks@codethink.co.uk> > Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> > [jorge.sanjuan@codethink.co.uk: Style fixes] > Signed-off-by: Jorge Sanjuan <jorge.sanjuan@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 0b176ea24914..ff1996f215ed 100644 > --- a/sound/soc/tegra/tegra30_i2s.c > +++ b/sound/soc/tegra/tegra30_i2s.c > @@ -265,6 +265,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, > return 0; > } > > +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_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x" > + "slots: 0x%08x width: %d\n", > + __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); > + > + pm_runtime_get_sync(dai->dev); > + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); > + > + /* Set FSYNC width */ > + mask = TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK; > + val = (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT; > + > + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, mask, val); > + pm_runtime_put(dai->dev); > + > + return 0; > +} > + Looking at the TRM for Tegra30 and Tegra124, the I2S_SLOT_CTRL register is different where for Tegra30 the 'TOTAL_SLOTS' bit are in position 18:16, but for Tegra124 they are 3:0. This driver supports both Tegra30 and Tegra124, and so this function will need to handle both. It can be quite common for the fsync-width for DSP modes to be a single clock and so I am not sure that is makes sense to set this here always to the slot width. It maybe worth considering add a DT property for specifying the fsync width. Cheers Jon
On Mon, Jul 30, 2018 at 10:31:16AM +0100, Jon Hunter wrote: > It can be quite common for the fsync-width for DSP modes to be a single clock and so > I am not sure that is makes sense to set this here always to the slot width. It maybe > worth considering add a DT property for specifying the fsync width. DSP modes only care about the rising edge of the LRCLK, the pulse can be any width without causing interoperability problems.
On 30/07/18 11:18, Mark Brown wrote: > On Mon, Jul 30, 2018 at 10:31:16AM +0100, Jon Hunter wrote: > >> It can be quite common for the fsync-width for DSP modes to be a single clock and so >> I am not sure that is makes sense to set this here always to the slot width. It maybe >> worth considering add a DT property for specifying the fsync width. > > DSP modes only care about the rising edge of the LRCLK, the pulse can be > any width without causing interoperability problems. OK, thanks I was not able to find a spec that defines this, but I saw a lot of codecs use a single bit clock width. So then equally making the default '1' should also be fine. I still do not like configuring the fsync width in this function. The fsync width needs to be configured for both DSP modes and normal I2S modes and so it seems it would be more appropriate to do this in the hw_params function for this driver. Cheers Jon
On 30/07/18 15:04, Jon Hunter wrote: > I still do not like configuring the fsync width in this function. The > fsync width needs to be configured for both DSP modes and normal I2S > modes and so it seems it would be more appropriate to do this in the > hw_params function for this driver. That said, it does not look like this current driver ever programs the fsync width for normal I2S mode (which I thought was necessary as we do in other Tegra I2S drivers). So I will check on this and confirm. Jon
On Mon, Jul 30, 2018 at 03:04:46PM +0100, Jon Hunter wrote: > On 30/07/18 11:18, Mark Brown wrote: > > DSP modes only care about the rising edge of the LRCLK, the pulse can be > > any width without causing interoperability problems. > OK, thanks I was not able to find a spec that defines this, but I saw a > lot of codecs use a single bit clock width. So then equally making the > default '1' should also be fine. There's not really a spec for this, it's just what tends to be implemented. > I still do not like configuring the fsync width in this function. The > fsync width needs to be configured for both DSP modes and normal I2S > modes and so it seems it would be more appropriate to do this in the > hw_params function for this driver. You *could* just always use the I2S width, it's going to look odd when people use a scope but it will work most of the time.
On 2018-07-30 16:07, Mark Brown wrote: > On Mon, Jul 30, 2018 at 03:04:46PM +0100, Jon Hunter wrote: >> On 30/07/18 11:18, Mark Brown wrote: > >> > DSP modes only care about the rising edge of the LRCLK, the pulse can be >> > any width without causing interoperability problems. > >> OK, thanks I was not able to find a spec that defines this, but I saw >> a >> lot of codecs use a single bit clock width. So then equally making the >> default '1' should also be fine. > > There's not really a spec for this, it's just what tends to be > implemented. > >> I still do not like configuring the fsync width in this function. The >> fsync width needs to be configured for both DSP modes and normal I2S >> modes and so it seems it would be more appropriate to do this in the >> hw_params function for this driver. > > You *could* just always use the I2S width, it's going to look odd when > people use a scope but it will work most of the time. We did this as we were dealing with a legacy system in which we didn't know if this was important setting or not, so we tried to make the settings as close as possible to the original nvidia supplied source.
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 0b176ea24914..ff1996f215ed 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -265,6 +265,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; } +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_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x" + "slots: 0x%08x width: %d\n", + __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); + + pm_runtime_get_sync(dai->dev); + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); + + /* Set FSYNC width */ + mask = TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK; + val = (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT; + + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, mask, val); + pm_runtime_put(dai->dev); + + return 0; +} + static int tegra30_i2s_probe(struct snd_soc_dai *dai) { struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); @@ -279,6 +312,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = { .set_fmt = tegra30_i2s_set_fmt, .hw_params = tegra30_i2s_hw_params, .trigger = tegra30_i2s_trigger, + .set_tdm_slot = tegra30_i2s_set_tdm, }; static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {