| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201005121307.v6jpyeyfi4kxc2cl@gilmour.lan>
Date: Mon, 5 Oct 2020 14:13:07 +0200
From: Maxime Ripard <maxime@...no.tech>
To: Clément Péron <peron.clem@...il.com>
Cc: Chen-Yu Tsai <wens@...e.org>, Rob Herring <robh+dt@...nel.org>,
Mark Brown <broonie@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Marcus Cooper <codekipper@...il.com>,
Jernej Skrabec <jernej.skrabec@...l.net>,
alsa-devel@...a-project.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-sunxi@...glegroups.com, Samuel Holland <samuel@...lland.org>
Subject: Re: [PATCH v6 02/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote:
> As slots and slot_width can be set manually using set_tdm().
> These values are then kept in sun4i_i2s struct.
> So we need to check if these values are setted or not
> in the struct.
>
> Avoid to check for this logic in set_chan_cfg(). This will
> duplicate the same check instead pass the required values
> as params to set_chan_cfg().
>
> This will also avoid a bug when we will enable 20/24bits support,
> i2s->slot_width is not actually used in the lrck_period computation.
>
> Suggested-by: Samuel Holland <samuel@...lland.org>
> Signed-off-by: Clément Péron <peron.clem@...il.com>
> ---
> sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++----------------------
> 1 file changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index c5ccd423e6d3..1f577dbc20a6 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks {
> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> s8 (*get_sr)(const struct sun4i_i2s *, int);
> s8 (*get_wss)(const struct sun4i_i2s *, int);
> - int (*set_chan_cfg)(const struct sun4i_i2s *,
> - const struct snd_pcm_hw_params *);
> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width);
> int (*set_fmt)(const struct sun4i_i2s *, unsigned int);
> };
>
> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width)
> }
>
> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210);
> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> }
>
> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> - unsigned int slots = channels;
> unsigned int lrck_period;
>
> - if (i2s->slots)
> - slots = i2s->slots;
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210);
> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> case SND_SOC_DAIFMT_DSP_B:
> case SND_SOC_DAIFMT_LEFT_J:
> case SND_SOC_DAIFMT_RIGHT_J:
> - lrck_period = params_physical_width(params) * slots;
> + lrck_period = slot_width * slots;
> break;
>
> case SND_SOC_DAIFMT_I2S:
> - lrck_period = params_physical_width(params);
> + lrck_period = slot_width;
> break;
>
> default:
> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> }
>
> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> - unsigned int slots = channels;
> unsigned int lrck_period;
>
> - if (i2s->slots)
> - slots = i2s->slots;
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98);
> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210);
> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> case SND_SOC_DAIFMT_DSP_B:
> case SND_SOC_DAIFMT_LEFT_J:
> case SND_SOC_DAIFMT_RIGHT_J:
> - lrck_period = params_physical_width(params) * slots;
> + lrck_period = slot_width * slots;
> break;
>
> case SND_SOC_DAIFMT_I2S:
> - lrck_period = params_physical_width(params);
> + lrck_period = slot_width;
> break;
>
> default:
> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> if (i2s->slot_width)
> slot_width = i2s->slot_width;
>
> - ret = i2s->variant->set_chan_cfg(i2s, params);
> + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width);
Isn't slots and slot_width set to 0 here ?
And therefore, wouldn't we set lrck_period to 0 if we're using any
format but I2S?
More importantly, I'm not really convinced this needs to be done, and it
introduces some side effects that are not explained.
Maxime
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists