Received-SPF: pass (google.com: domain of linux-kernel+bounces-104297-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <dda0e6ba-4538-47a0-95e9-6adcfd4169a7@baylibre.com>
Date: Fri, 15 Mar 2024 12:01:12 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
Content-Language: en-US
To: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Matthias Brugger
 <matthias.bgg@gmail.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
 Lee Jones <lee@kernel.org>, Flora Fu <flora.fu@mediatek.com>,
 Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
 Sumit Semwal <sumit.semwal@linaro.org>,
 =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
 Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>,
 linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-mediatek@lists.infradead.org, linux-media@vger.kernel.org,
 dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
 Nicolas Belin <nbelin@baylibre.com>
References: <20240226-audio-i350-v1-0-4fa1cea1667f@baylibre.com>
 <20240226-audio-i350-v1-12-4fa1cea1667f@baylibre.com>
 <9891855d-2284-42e4-9d3a-35ba406540e8@sirena.org.uk>
 <c441a132-b16b-4244-a712-8971c902d4d7@baylibre.com>
 <ff3d2db1-697b-42c6-a0f2-74276e9fc098@sirena.org.uk>
From: Alexandre Mergnat <amergnat@baylibre.com>
In-Reply-To: <ff3d2db1-697b-42c6-a0f2-74276e9fc098@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 13/03/2024 18:23, Mark Brown wrote:
> On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote:
>> On 26/02/2024 17:09, Mark Brown wrote:
> 
>>>> +	case MT6357_ZCD_CON2:
>>>> +		regmap_read(priv->regmap, MT6357_ZCD_CON2, &reg);
>>>> +		priv->ana_gain[ANALOG_VOLUME_HPOUTL] =
>>>> +			(reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT;
>>>> +		priv->ana_gain[ANALOG_VOLUME_HPOUTR] =
>>>> +			(reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT;
>>>> +		break;
> 
>>> It would probably be less code and would definitely be clearer and
>>> simpler to just read the values when we need them rather than constatly
>>> keeping a cache separate to the register cache.
> 
>> Actually you must save the values because the gain selected by the user will
>> be override to do a ramp => volume_ramp(.....):
>> - When you switch on the HP, you start from gain=-40db to final_gain
>> (selected by user).
>> - When you switch off the HP, you start from final_gain (selected by user)
>> to gain=-40db.
> 
> You can just read the value back when you need to do a ramp?

You can't. Because you will read -40db when HP isn't playing sound. That 
is why the gain is saved into the struct.

Let me know, when you change de gain to do a ramp down (start from user 
gain to gain=-40db), next time for the ramp up, how/where do you find 
the user gain ?


> 
>> Also, the microphone's gain change when it's enabled/disabled.
> 
> I don't understand what this means?

When microphone isn't capturing, the gain read back from the register is 
0dB. I've put some logs in my code and do capture to show how it works:

root@i350-evk:~# arecord -D hw:mt8365evk,2,0 -r 48000 -c2 -f s32_le -d 
10 recorded_file.wav
[Mar15 09:31] mt8365-afe-pcm 11220000.audio-controller: 
mt8365_afe_fe_hw_params AWB period = 6000 rate = 48000 channels = 2
[  +0.000126] mt8365-afe-pcm 11220000.audio-controller: 
mt8365_dai_int_adda_prepare 'Capture' rate = 48000
[  +0.107688] mt6357-sound mt6357-sound: TOTO set mic to stored value
[ +10.072648] mt6357-sound mt6357-sound: TOTO set mic to 0dB

root@i350-evk:~# arecord -D hw:mt8365evk,2,0 -r 48000 -c2 -f s32_le -d 
10 recorded_file.wav
[Mar15 09:32] mt8365-afe-pcm 11220000.audio-controller: 
mt8365_afe_fe_hw_params AWB period = 6000 rate = 48000 channels = 2
[  +0.000133] mt8365-afe-pcm 11220000.audio-controller: 
mt8365_dai_int_adda_prepare 'Capture' rate = 48000
[  +0.109418] mt6357-sound mt6357-sound: TOTO set mic to stored value
[ +10.164197] mt6357-sound mt6357-sound: TOTO set mic to 0dB


> 
>>>> +	/* ul channel swap */
>>>> +	SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0),
> 
>>> On/off controls should end in Switch.
> 
>> Sorry, I don't understand your comment. Can you reword it please ?
> 
> See control-names.rst.  Run mixer-test on a card with this driver and
> fix all the issues it reports.

Ok the name is the issue for you AFAII.
This control isn't for on/off but swap Left and Right.
 From the codec documentation:
"Swaps audio UL L/R channel before UL SRC"
This control is overkill, I will remove it

I'm stuck to run mixer-test, please check the following message: 
https://lore.kernel.org/all/7ddad394-e880-4ef8-8591-cb803a2086ae@baylibre.com/


-- 
Regards,
Alexandre