| Message ID | 20190930165130.10642-2-ben.dooks@codethink.co.uk |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3,1/7] ASoC: tegra: add a TDM configuration callback | expand |
On 30/09/2019 17:51, 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> > [ben.dooks@codethink.co.uk: merge fix for power management] > [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > sound/soc/tegra/tegra30_i2s.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c > index ac6983c6bd72..7f9ef6abeae2 100644 > --- a/sound/soc/tegra/tegra30_i2s.c > +++ b/sound/soc/tegra/tegra30_i2s.c > @@ -254,6 +254,38 @@ 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, val; > + > + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d 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); I am not sure why this keeps changing. Why not set to 1 always? Jon
On 30/09/2019 21:46, Jon Hunter wrote: > > On 30/09/2019 17:51, 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> >> [ben.dooks@codethink.co.uk: merge fix for power management] >> [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> sound/soc/tegra/tegra30_i2s.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c >> index ac6983c6bd72..7f9ef6abeae2 100644 >> --- a/sound/soc/tegra/tegra30_i2s.c >> +++ b/sound/soc/tegra/tegra30_i2s.c >> @@ -254,6 +254,38 @@ 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, val; >> + >> + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d 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); > > I am not sure why this keeps changing. Why not set to 1 always? This is the total number of slots, not the width of the fsync which has been changed to 1 below this.
On 01/10/2019 12:00, Ben Dooks wrote: > On 30/09/2019 21:46, Jon Hunter wrote: >> >> On 30/09/2019 17:51, 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> >>> [ben.dooks@codethink.co.uk: merge fix for power management] >>> [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] >>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >>> --- >>> sound/soc/tegra/tegra30_i2s.c | 33 +++++++++++++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/sound/soc/tegra/tegra30_i2s.c >>> b/sound/soc/tegra/tegra30_i2s.c >>> index ac6983c6bd72..7f9ef6abeae2 100644 >>> --- a/sound/soc/tegra/tegra30_i2s.c >>> +++ b/sound/soc/tegra/tegra30_i2s.c >>> @@ -254,6 +254,38 @@ 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, val; >>> + >>> + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d >>> 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); >> >> I am not sure why this keeps changing. Why not set to 1 always? > > This is the total number of slots, not the width of the fsync > which has been changed to 1 below this. Ah yes indeed. Sorry, should have waited until this morning to look at this! Jon
On 30/09/2019 17:51, 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> > [ben.dooks@codethink.co.uk: merge fix for power management] > [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > sound/soc/tegra/tegra30_i2s.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c > index ac6983c6bd72..7f9ef6abeae2 100644 > --- a/sound/soc/tegra/tegra30_i2s.c > +++ b/sound/soc/tegra/tegra30_i2s.c > @@ -254,6 +254,38 @@ 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, val; > + > + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d 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 the fsync width to minimum of 1 clock width Please make sure you are consistent with your commenting style and you adhere to the kernel coding style. Also, I see a lot of ... ERROR: trailing whitespace #197: FILE: sound/soc/tegra/tegra30_i2s.c:258: + * Set up TDM^M$ ERROR: DOS line endings #198: FILE: sound/soc/tegra/tegra30_i2s.c:259: + */^M$ Jon
On 01/10/2019 14:40, Jon Hunter wrote: > > On 30/09/2019 17:51, 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> >> [ben.dooks@codethink.co.uk: merge fix for power management] >> [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> sound/soc/tegra/tegra30_i2s.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c >> index ac6983c6bd72..7f9ef6abeae2 100644 >> --- a/sound/soc/tegra/tegra30_i2s.c >> +++ b/sound/soc/tegra/tegra30_i2s.c >> @@ -254,6 +254,38 @@ 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, val; >> + >> + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d 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 the fsync width to minimum of 1 clock width > > Please make sure you are consistent with your commenting style and you > adhere to the kernel coding style. > > Also, I see a lot of ... > > ERROR: trailing whitespace > #197: FILE: sound/soc/tegra/tegra30_i2s.c:258: > + * Set up TDM^M$ > > ERROR: DOS line endings > #198: FILE: sound/soc/tegra/tegra30_i2s.c:259: > + */^M$ I'll go and check that later. I did assume my colleagues had done the relevant checks themselves...
On 01/10/2019 14:40, Jon Hunter wrote: > > On 30/09/2019 17:51, 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> >> [ben.dooks@codethink.co.uk: merge fix for power management] >> [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> sound/soc/tegra/tegra30_i2s.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c >> index ac6983c6bd72..7f9ef6abeae2 100644 >> --- a/sound/soc/tegra/tegra30_i2s.c >> +++ b/sound/soc/tegra/tegra30_i2s.c >> @@ -254,6 +254,38 @@ 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, val; >> + >> + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d 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 the fsync width to minimum of 1 clock width > > Please make sure you are consistent with your commenting style and you > adhere to the kernel coding style. > > Also, I see a lot of ... > > ERROR: trailing whitespace > #197: FILE: sound/soc/tegra/tegra30_i2s.c:258: > + * Set up TDM^M$ > > ERROR: DOS line endings > #198: FILE: sound/soc/tegra/tegra30_i2s.c:259: > + */^M$ ok, for me I am getting: > $ ./scripts/checkpatch.pl ./patches/0001-ASoC-tegra-add-a-TDM-configuration-callback.patch > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > #6: > Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' > > total: 0 errors, 1 warnings, 45 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. I don't see any warnings about the line-endings I will remove the comment about the function, it is fairly self explanatory.
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..7f9ef6abeae2 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,38 @@ 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, val; + + dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d 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 the fsync width to minimum of 1 clock width + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, 0x0); + 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); @@ -268,6 +300,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 = {