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

[Nick Li <nick.li@foursemi.com>]

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