linux-kernel - Re: [PATCH v2 4/4] ASoC: codecs: Add FourSemi FS2104/5S audio amplifier driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aG5Smt_DErSq5WjE@finisterre.sirena.org.uk>
Date: Wed, 9 Jul 2025 12:29:30 +0100
From: Mark Brown <broonie@...nel.org>
To: Nick Li <nick.li@...rsemi.com>
Cc: lgirdwood@...il.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, perex@...ex.cz, tiwai@...e.com,
	xiaoming.yang@...rsemi.com, danyang.zheng@...rsemi.com,
	like.xy@...mail.com, linux-sound@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] ASoC: codecs: Add FourSemi FS2104/5S audio
 amplifier driver

On Tue, Jul 08, 2025 at 07:29:01PM +0800, Nick Li wrote:

> The FS2104/5S are FourSemi digital audio amplifiers
> with I2C control. They are Inductor-Less, Stereo, Closed-Loop,
> Digital Input Class-D Power Amplifiers with Enhanced Signal Processing.

This looks broadly OK - there's a few more fairly small issues below in
addition to those that Krzysztof flagged.

> +static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)
> +{
> +	u16 vol[2];
> +	int ret;
> +
> +	if (fs210x->devid == FS2105S_DEVICE_ID) {

A swtich statement is better style here, it makes it easier to add more
variants.

> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {

_CLOCK_PROVIDER_MASK

> +	case SND_SOC_DAIFMT_CBC_CFC:
> +		/* Only supports slave mode */

consumer mode.

> +		dev_err(fs210x->dev, "Only supports slave mode\n");

consumer mode.

> +static int fs210x_dai_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	struct fs210x_priv *fs210x;
> +	int chn_num;
> +	int ret;
> +
> +	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
> +		return 0;

There's no configuration for capture?  Should the driver be setting
symmetric_rates and then not reconfiguring if active instead, it looks a
lot like the driver is relying on userspace starting playback before
capture to configure?

> +static int fs210x_pcm_volume_put(struct snd_kcontrol *kcontrol,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{

> +	fs210x->vol[0] = (u16)*pval;
> +	fs210x->vol[1] = (u16)*(pval + 1);
> +	dev_dbg(fs210x->dev, "Set volume: %d-%d\n",
> +		fs210x->vol[0], fs210x->vol[1]);
> +

The driver should return 1 if there's a change in the values and 0
otherwise (plus it can skip a bunch of work if there's nothing to do).

The mixer-test test will spot this for you.

> +static int fs210x_effect_scene_put(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{

> +	ret = fs210x_set_scene(fs210x, scene_id);
> +	if (ret)
> +		dev_err(fs210x->dev, "Failed to set scene: %d\n", ret);
> +
> +	mutex_unlock(&fs210x_mutex);
> +
> +	return ret;

Same issue with flagging changes.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)