Message-ID: <efe290ba-32b0-8eed-c901-21af38622496@opensource.cirrus.com>
Date:   Mon, 20 Feb 2023 13:40:26 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v5 0/4] Add CS35L41 shared boost feature
Content-Language: en-US
To:     Lucas Tanure <lucas.tanure@collabora.com>,
        David Rhodes <david.rhodes@cirrus.com>,
        Charles Keepax <ckeepax@opensource.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>
CC:     <alsa-devel@alsa-project.org>, <devicetree@vger.kernel.org>,
        <patches@opensource.cirrus.com>, <linux-kernel@vger.kernel.org>,
        <kernel@collabora.com>
References: <20230210091942.10866-1-lucas.tanure@collabora.com>
From:   David Rhodes <drhodes@opensource.cirrus.com>
In-Reply-To: <20230210091942.10866-1-lucas.tanure@collabora.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Apologies for the formatting mistake. Resending the previous reply.

On 2/12/23 03:28, Lucas Tanure wrote:
 > On 11-02-2023 17:06, Charles Keepax wrote:
 >> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
 >>> On 10-02-2023 13:43, Charles Keepax wrote:
 >>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
 >>>>> +    {CS35L41_MDSYNC_EN,        0x00001000},
 >>>> David's internal patch appears to set 0x3000 on the active side,
 >>>> not sure where that difference snuck in, or which is the correct
 >>>> value. Your settings appear to make logical sense to me though, TX
 >>>> on the active side, RX on the passive side.
 >>> And as the patch sets TX and RX in the same chip I changed to follow
 >>> the documentation.
 >>
 >> Yeah I mean I suspect this is sensible, unless there is some
 >> reason the controller side also needs to have RX enabled. Perhaps
 >> for feedback or something from the passive side, but I imagine
 >> this is just a typo in the original patch.
 >
 > Ok, but the other side doesn't have both RX and TX enabled.
 > If the active side needed RX to receive information for the other 
side, the passive one would need TX enabled too.
 > So if a feedback is necessary, both channels on both sides would be 
enabled, not one channel in one side and both on the other.
 >
Both amps need to transmit their boost targets to the MDSYNC bus. The 
active amp needs to receive the combined boost target from the MDSYNC 
bus. That is why the active amp should enable both RX and TX, and the 
passive amp only needs to enable TX. It is not simply a unidirectional 
flow of data from one amp to the other.

Sorry if the documentation has been mismatched or confusing at times. 
It's taken me a while to gather the right understanding of how this all 
works.


On 2/10/23 03:19, 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. GPIO1 
should be

Not quite correct. I would suggest: "Shared boost allows two amplifiers 
to share a single boost circuit by communicating on the MDSYNC bus. The 
active amplifier controls the boost circuit using combined data from 
both amplifiers."


On 2/10/23 08:39, Lucas Tanure wrote:
 >
 > This write here is to select the boost control source, which for the 
active should be "Class H tracking value".

Active should use the MDSYNC value. Otherwise it will not provide boost 
to the passive amp when there is only audio on the passive amp's channel.


I believe there is another change needed for the Deck, to handle the 
'legacy' property names instead of bst-type?

Other than the changes needed to the reg_sequences this looks good.


Thanks,

David