lkml.org 
[lkml]   [2020]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 04/10] ASoC: tegra: add Tegra210 based I2S driver
    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.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2020-02-21 14:22    [W:4.459 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site