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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6038 bytes --]

On Thu, Jul 03, 2025 at 11:56:37AM +0800, Nick wrote:

> +++ b/sound/soc/codecs/fs210x.c
> @@ -0,0 +1,1616 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * fs210x.c -- Driver for the FS2104/5S Audio Amplifier
> + *
> + * Copyright (C) 2016-2025 Shanghai FourSemi Semiconductor Co.,Ltd.

Please make the entire comment a C++ one so things look more
intentional.

> +#define FS210X_DRV_VERSION		"v1.0.6"

We generally don't do versions for components within the kernel, nobody
is going to update it going forward.

> +static int fs210x_reg_read(struct fs210x_priv *fs210x,
> +			   u8 reg, u16 *pval)

> +	if (pval)
> +		*pval = (u16)val;
> +

If this cast is needed that's a bit worrying.

> +	dev_dbg(fs210x->dev, "RR: %02x %04x\n", reg, val);

We do have a lot of logging support in regmap which is more
controllable.

> +static int fs210x_reg_dump(struct fs210x_priv *fs210x)
> +{

This is duplicating regmap's debugfs.

> +	} else if (pkg->cmd == FS_CMD_DELAY) {
> +		if (pkg->regv.val > FS_CMD_DELAY_MS_MAX)
> +			return -ENOTSUPP;
> +		delay_us = pkg->regv.val * 1000;
> +		usleep_range(delay_us, delay_us + 50);

Just use fsleep(), it'll use the most sensible delay type for the delay.
In general this applies to all delays in the driver, but especially with
a variable delay like this.

> +static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)

> +	ret  = fs210x_reg_write(fs210x, FS210X_39H_LVOLCTRL, vol[0]);
> +	ret |= fs210x_reg_write(fs210x, FS210X_3AH_RVOLCTRL, vol[1]);
> +

This looks pretty generic, why is it not using a standard control type?

> +	ret = fs210x_set_pcm_volume(fs210x);

The driver should use the device defaults rather than having to 

> +static void fs210x_sdz_pin_set(struct fs210x_priv *fs210x, bool active)
> +{
> +	if (!fs210x || !fs210x->gpio_sdz)
> +		return;

Shouldn't this be integrated with the chip init/reset?

> +static int fs210x_mute(struct fs210x_priv *fs210x, bool mute)
> +{
> +	int ret;
> +
> +	if (mute) {
> +		cancel_delayed_work_sync(&fs210x->fault_check_work);
> +		cancel_delayed_work_sync(&fs210x->start_work);
> +		mutex_lock(&fs210x_mutex);
> +		ret = fs210x_dev_stop(fs210x);
> +		mutex_unlock(&fs210x_mutex);
> +		return ret;
> +	}

Mute is expected to be a really fast operation, this is stopping the
device entirely and fiddling about with locks (which were held where?).
This looks like the device just doesn't support mute.

> +	/*
> +	 * According to the power up/down sequence of FS210x,
> +	 * the FS210x requests the I2S clock has been present
> +	 * and stable(>= 2ms) before it playing.
> +	 */
> +	if (fs210x->clk_bclk) {
> +		mutex_lock(&fs210x_mutex);
> +		ret = fs210x_dev_play(fs210x);
> +		mutex_unlock(&fs210x_mutex);
> +	} else {

This is definitely not appropriate for mute, it should be in the power
management flow - either set_bias_level() or a DAPM widget.

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

consumer mode.

> +static int fs210x_dai_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)

> +	dev_info(fs210x->dev, "hw params: %d-%d-%d\n",
> +		 fs210x->srate, chn_num, fs210x->bclk);

No dev_info() prints in the normal playback/record flow, that just spams
the logs too easily.

> +	/* The FS2105S can't support 16kHz sample rate. */
> +	if (fs210x->devid == FS2105S_DEVICE_ID && fs210x->srate == 16000)
> +		return -ENOTSUPP;

This should be reported in constraints too.

> +	if (!(status & FS210X_05H_AMPS_MASK))
> +		dev_err(fs210x->dev, "Amplifier unready\n");

Does this get triggered during the normal start/stop flow?

> +	schedule_delayed_work(&fs210x->fault_check_work,
> +			      msecs_to_jiffies(FS210X_FAULT_CHECK_INTERVAL_MS));

Might be good to have this tunable from sysfs.

> +static int fs210x_pcm_volume_put(struct snd_kcontrol *kcontrol,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct fs210x_priv *fs210x;
> +	long *pval;
> +	int ret;
> +
> +	ret = fs210x_get_drvdata_from_kctrl(kcontrol, &fs210x);
> +	if (ret || !fs210x->dev) {
> +		pr_err("pcm_volume_put: fs210x is null\n");
> +		return -EINVAL;
> +	}
> +

_put() callbacks should return 1 when the control changes values, though
as indicated above I'm not clear this needs to be a custom control at
all.  Please run mixer-test against your driver, it will check for this
and other issues.

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

> +	/*
> +	 * FS210x has scene(s) as below:
> +	 * init scene: id = 0(It's set in fs210x_init_chip() only)
> +	 * effect scene(s): id = 1~N (optional)
> +	 * scene_id = effect_index + 1.
> +	 */
> +	scene_id = ucontrol->value.integer.value[0] + 1;
> +	if (fs210x->is_suspended) {
> +		fs210x->scene_id = scene_id;
> +		mutex_unlock(&fs210x_mutex);
> +		return 0;
> +	}

This doesn't validate the passed value at all.

> +static int fs210x_probe(struct snd_soc_component *cmpnt)
> +{
> +	struct fs210x_priv *fs210x;
> +	int ret;

> +	INIT_DELAYED_WORK(&fs210x->fault_check_work, fs210x_fault_check_work);
> +	INIT_DELAYED_WORK(&fs210x->start_work, fs210x_start_work);
> +
> +	fs210x_get_bclk(fs210x, cmpnt);

This sort of initialisation and resource acquisition that doesn't
require any audio stuff should be done at the bus level so that we don't
register with ASoC until all the inputs are ready.

> +#ifdef CONFIG_PM
> +static int fs210x_suspend(struct snd_soc_component *cmpnt)
> +{
> +	struct fs210x_priv *fs210x;
> +	int ret;
> +
> +	fs210x = snd_soc_component_get_drvdata(cmpnt);
> +	if (!fs210x || !fs210x->dev)
> +		return -EINVAL;
> +
> +	cancel_delayed_work_sync(&fs210x->start_work);
> +	cancel_delayed_work_sync(&fs210x->fault_check_work);
> +
> +	mutex_lock(&fs210x_mutex);

We don't need to prevent new work being scheduled?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]