Date:   Wed, 8 Feb 2023 17:50:34 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Lucas Tanure <lucas.tanure@collabora.com>
CC:     David Rhodes <david.rhodes@cirrus.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Mark Brown <broonie@kernel.org>,
        "Rob Herring" <robh+dt@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, <alsa-devel@alsa-project.org>,
        <devicetree@vger.kernel.org>, <patches@opensource.cirrus.com>,
        <linux-kernel@vger.kernel.org>, <kernel@collabora.com>
Subject: Re: [PATCH v3 3/4] ALSA: cs35l41: Add shared boost feature
Message-ID: <20230208175034.GA12852@ediswmail.ad.cirrus.com>
References: <20230208141839.1097377-1-lucas.tanure@collabora.com>
 <20230208141839.1097377-4-lucas.tanure@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230208141839.1097377-4-lucas.tanure@collabora.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Wed, Feb 08, 2023 at 02:18:38PM +0000, Lucas Tanure wrote:
> Shared boost allows two amplifiers to share a single boost circuit by
> communicating on the MDSYNC bus.
> The passive amplifier does not control the boost and receives data from
> the active amplifier.
> 
> Shared Boost is not supported in HDA Systems.
> Based on David Rhodes shared boost patches.
> 
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> ---
> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> +		if (ret == 0) {
> +			ret = -ETIMEDOUT;
> +		} else {
> +			regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> +			pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
> +			struct reg_sequence cs35l41_mdsync_up_seq[] = {
> +				{CS35L41_PWR_CTRL3,	pwr_ctrl3},
> +				{CS35L41_PWR_CTRL1,	0x00000000, 3000},
> +				{CS35L41_PWR_CTRL1,	0x00000001, 3000},
> +			};

I think this looks good as long as turning the DRE off wasn't an
important part of the sequence and was just accidentally
happening. Probably on David to confirm that one.

Although should really move the variable definition to the top of
the code block and put a blank line after it, to pass checkpatch.

Thanks,
Charles

> +			ret = regmap_multi_reg_write(cs35l45->regmap, cs35l41_mdsync_up_seq,
> +						     ARRAY_SIZE(cs35l41_mdsync_up_seq));