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
