Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
> Enabling the active/passive shared boosts involves writing the MDSYNC UP
> register sequence, which cannot be performed before receiving the PLL
> lock signal.
> 
> Due to improper error handling, it was not obvious the wait operation
> times out and, consequently, the shared boost gets never enabled.
> 
> Further investigations revealed the signal is triggered while
> snd_pcm_start() is executed, right after receiving the
> SNDRV_PCM_TRIGGER_START command, which happens long after the
> SND_SOC_DAPM_PRE_PMU event handler is invoked as part of
> snd_pcm_prepare().  That is where cs35l41_global_enable() is called
> from.
> 
> Increasing the wait duration doesn't help, as it only causes an
> unnecessary delay in the invocation of snd_pcm_start().  Moving the wait
> and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START
> callback is not a solution either, since they would be executed in an
> IRQ-off atomic context.
> 
> Solve the issue by deferring the processing to a workqueue task, which
> allows to correctly wait for the signal and then safely proceed with
> the required regmap operations.
> 
> Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
> ---

Thanks for looking at this apologies this was missed in the
initial review of the patch.

> +int cs35l41_mdsync_up(struct regmap *regmap)
> +{
> +	struct reg_sequence cs35l41_mdsync_up_seq[] = {
> +		{CS35L41_PWR_CTRL3, 0},
> +		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
> +		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
> +	};
> +	unsigned int pwr_ctrl3, int_status;
> +	int ret;
> +
> +	regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> +	pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
> +	cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
> +
> +	ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
> +				     ARRAY_SIZE(cs35l41_mdsync_up_seq));
> +	if (ret < 0)
> +		return ret;

Is this now safe? By pulling this out into a worker thread, it is
no longer under the DAPM lock, which makes me worry this can race
with the other uses of PWR_CTRL3 which could theoretically change
state between when you read the reg and when you write it.

> @@ -1243,33 +1289,27 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
>  		cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
>  		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
>  					     ARRAY_SIZE(cs35l41_mdsync_down_seq));
> -		if (ret || !enable)
> +		if (ret)
>  			break;
>  
> -		if (!pll_lock)
> -			return -EINVAL;
> -
> -		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> -		if (ret == 0) {
> -			dev_err(dev, "Timed out waiting for pll_lock\n");
> -			return -ETIMEDOUT;
> +		if (enable) {
> +			if (mdsync_up_work) {
> +				/* Call cs35l41_mdsync_up() after receiving PLL lock signal */
> +				schedule_work(mdsync_up_work);
> +			} else {
> +				dev_err(dev, "MDSYNC UP work not provided\n");
> +				ret = -EINVAL;
> +			}
> +			break;

One question I might also have would be does a worker thread make
more sense or would it be simpler to do the mdsync power up
directly in response to the PLL lock IRQ?

Thanks,
Charles