21 Feb
2020
21 Feb
'20
3:21 p.m.
On Thu, Feb 20, 2020 at 12:04:46PM +0530, Sameer Pujar wrote:
@@ -0,0 +1,938 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- tegra210_i2s.c - Tegra210 I2S driver
All C++ please.
+static void tegra210_i2s_set_data_offset(struct tegra210_i2s *i2s,
unsigned int data_offset)+{
- unsigned int mask = I2S_CTRL_DATA_OFFSET_MASK;
- unsigned int shift = I2S_DATA_SHIFT;
- unsigned int reg;
- reg = TEGRA210_I2S_TX_CTRL;
- regmap_update_bits(i2s->regmap, reg, mask, data_offset << shift);
- reg = TEGRA210_I2S_RX_CTRL;
- regmap_update_bits(i2s->regmap, reg, mask, data_offset << shift);
The way this is written is *weird*, especially the use of reg - it'd probably be clearer to just use the values directly rather than have these intermediate temporary values.
+static int tegra210_i2s_get_control(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *compnt = snd_soc_kcontrol_component(kcontrol);
- struct tegra210_i2s *i2s = snd_soc_component_get_drvdata(compnt);
- long *uctl_val = &ucontrol->value.integer.value[0];
- if (strstr(kcontrol->id.name, "Loopback"))
*uctl_val = i2s->loopback;- else if (strstr(kcontrol->id.name, "Sample Rate"))
*uctl_val = i2s->srate_override;- else if (strstr(kcontrol->id.name, "FSYNC Width"))
*uctl_val = i2s->fsync_width;- else if (strstr(kcontrol->id.name, "Playback Audio Bit Format"))
*uctl_val = i2s->audio_fmt_override[I2S_RX_PATH];- else if (strstr(kcontrol->id.name, "Capture Audio Bit Format"))
*uctl_val = i2s->audio_fmt_override[I2S_TX_PATH];
Same issue as the DMIC driver, these really shouldn't be exposed to userspace as regular controls.
- /*
* For playback I2S RX-CIF and for capture TX-CIF is used.* With reference to AHUB, for I2S, SNDRV_PCM_STREAM_CAPTURE stream is* actually for playback.*/- path = (substream->stream == SNDRV_PCM_STREAM_CAPTURE) ?
I2S_RX_PATH : I2S_TX_PATH;
Please write normal conditional statements, it makes things easier to read.