Message-ID: <0370941d-63eb-4676-8a74-b8afef524376@sirena.org.uk>
Date: Fri, 4 Jul 2025 15:37:32 +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,
	linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/4] ASoC: codecs: Add FourSemi FS2104/5S audio
 amplifier driver

On Fri, Jul 04, 2025 at 07:12:47PM +0800, Nick Li wrote:
> On Thu, Jul 03, 2025 at 03:59:34PM +0100, Mark Brown wrote:
> > On Thu, Jul 03, 2025 at 11:56:37AM +0800, Nick wrote:

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

> 1. We use regmap as REGMAP_NONE, because most of the registers settings
>    are in the firmware, if we use a standard control,the driver shouldn't
>    cache the registers after suspending the device(it will be reset).

You can exclude registers from a cache, including excluding most of
them, or manually handle registers over suspend/resume.  It's generally
better to share the userspace facing interfaces.

> 2. The volume registers of FS2104 and FS2105S are different,
>    if we us a stardard control, we need two controls,
>    and register it by checking the device type.
> so we customize the volume control.

If the different devices have different controls you should just
register different controls.

> > > +	ret = fs210x_set_pcm_volume(fs210x);

> > The driver should use the device defaults rather than having to 

> The volume contorl can be used to set different volumes,
> the volume will be masked in fs210x->vol[2],
> we restore the volume when the driver resumes(reinitializes) the deivce.

You're not just restoring the values on resume, you're also overwriting
them on probe.

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

> 1. We implement this function(reset and wait times) to clarify that
>    pulling up/down the SDZ/reset pin must to wait enougth delay time.

That doesn't really answer the question?

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

> 1. Because the device uses BCLK clock as the clock source,
>    we need to start the device in the life cycle of the clock,
>    also we need to start device after the PLL setting(set in dai->hw_params)
>    so we start the device in here: dai->mute_stream(unmute)

All the power management happens after hw_params(), this isn't an issue.

> 2. If the SOC(s) doesn't have the clock(bclk) for us to configure,
>    It meams no clock bclk defined in the DTS,
>    and the clock is activated in dai->trigger start usually,
>    so we will use a delay work to start the device in here.

> Any good ideas about satisfying this power up/down sequence?

There's not great options here, and you're going to loose the start of
playback especially with devices that don't start clocking until audio
starts.  You really need the CPU vendors you're working with to
implement SND_SOC_DAIFMT_CONT or expose their clocks via the clock API
but not all hardware is able to do this.  I think given how limited your
hardware is here you really need something in trigger() or some new
callback that runs later than that, the delayed work you've got there is
trying to fudge things to run after trigger.

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

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

> It will get triggered when:
> 1. BCLK clock is closed before stoping device
> 2. BCLK clock is opened after starting device
> We should avoid these power up/down sequence, it may cause pop noise,
> If they happens, it should be reported and fixed?

If they don't happen in normal operation it's fine to have them, the
concern was that this would be triggered during normal operation as part
of the startup or shutdown sequence.

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

> > Might be good to have this tunable from sysfs.

> Good idea, or set the interval times by the DTS property.
> We are considering adding a DTS property:
> foursemi,monitor-period-ms

I suspect the DT people won't like that since it's more of a tuning
thing.

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

> Could you please explain more details to help me understand and test this case?

What if for example playback is starting up at the same time as the
system enters suspend - the CODEC startup might get run after the
delayed work is cancelled but before the lock is taken.

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

Powered by blists - more mailing lists