[2/4] ASoC: tegra: Add a TDM configuration callback
diff mbox series

Message ID 20180727125931.9794-3-jorge.sanjuan@codethink.co.uk
State New
Headers show
Series
  • ASoC: Tegra30 TDM support
Related show

Commit Message

Jorge Sanjuan July 27, 2018, 12:59 p.m. UTC
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(+)

Comments

Mark Brown July 30, 2018, 8:49 a.m. UTC | #1
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?
Ben Dooks July 30, 2018, 9:04 a.m. UTC | #2
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
Jon Hunter July 30, 2018, 9:31 a.m. UTC | #3
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
Mark Brown July 30, 2018, 10:18 a.m. UTC | #4
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.
Jon Hunter July 30, 2018, 2:04 p.m. UTC | #5
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
Jon Hunter July 30, 2018, 2:15 p.m. UTC | #6
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
Mark Brown July 30, 2018, 3:07 p.m. UTC | #7
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.
Ben Dooks July 30, 2018, 5:39 p.m. UTC | #8
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.

Patch
diff mbox series

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 = {