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: <61B23934F28A9F2C+aG-AXznikkHb2_jR@foursemi.com>
Date: Thu, 10 Jul 2025 16:57:03 +0800
From: Nick Li <nick.li@...rsemi.com>
To: Mark Brown <broonie@...nel.org>
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 Wed, Jul 09, 2025 at 12:29:30PM +0100, Mark Brown wrote:
> 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.

Good idea.

> 
> > +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> 
> _CLOCK_PROVIDER_MASK
> 
> > +	case SND_SOC_DAIFMT_CBC_CFC:
> > +		/* Only supports slave mode */
> 
> consumer mode.

You had metioned in v1, will fix next version.

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

Got it.

> 
> > +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?

In capture path:
The device can output EC reference signal for AEC,
it can't output the signal without playing audio(start device in playback).
We had set symmetric_rates = 1 in fs210x_dai

> 
> > +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.

BTW, what's the mixer-test? We tested it by using alsamixer and amixer.

> 
> > +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.

Got it, we will update what you mentioned in here and previous mail,
the next version may need to test for a few days, I'm sorry for my late response.

Thanks.

Best regards,
Nick