Message-ID: <88CC983A5550253C+aGuGe7pQvIBPclfz@foursemi.com>
Date: Mon, 7 Jul 2025 16:34: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,
	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 03:37:32PM +0100, Mark Brown wrote:
> 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.

OK, we will update the volume control to:
SOC_DOUBLE_TLV(...)
and use the regmap to cache the volumes.

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

OK.

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

Yes, the volume will be set to the default value at the first time of initialization on probe,
and it may be updated by volume control later.
It's a good way to use the regmap cache(REGCACHE_RBTREE) to cache the volumes.

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

It's ok to integrate it into the chip init/reset.

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

We will evaluate the way to reduce the cost time of starting/stoping device:
Do PM by DAPM widget or set_bias_on.

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

We will start the start_work in dai->trigger if there is no clock bclk
for us to control, and disable fading in/out in firmware to reduce
the time consumption if it is needed(As a backup plan).

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

We can remove the registers dumping, and set the AMPS status to dev_dbg level.

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

OK, we add a sysfs node for this.

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

Understood, thanks a lot.

We evaluate to remove the resume/suspend(cold start up/down device):
The power consumptions between register settings and pins controlling are very close,
and it can help us to reduce the time consumption.

Best regards,
Nick

Powered by blists - more mailing lists