Re: [PATCH] ASoC: codecs: lpass-va-macro: set the default codec version for sm8250

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On August 16, 2024 3:59:18 PM GMT+07:00, Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> wrote:
>
>
>On 15/08/2024 22:22, Dmitry Baryshkov wrote:
>> On August 16, 2024 4:07:10 AM GMT+07:00, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
>>> On August 15, 2024 11:49:03 PM GMT+07:00, srinivas.kandagatla@xxxxxxxxxx wrote:
>>>> From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
>>>> 
>>>> sm8250 and sc7280 have lpass codec version 1.0, as these are very old
>>>> platforms, they do not have a reliable way to get the codec version
>>> >from core_id registers.
>> 
>> I wrote that it looked good, but maybe you can also describe, why core_id registers are not reliable? Are they just not present on those platforms or is there any other issue?
>> 
>Sure, the comment is correct because the registers are available to read however the values of those registers are not fit for dynamically detecting the version, like what we do in the driver.

If I remember correctly, on sm8250 these registers read 0x0 /0xf /0x0. Are you saying that there are several revision that will have these values? Do we need to know that it is exactly 1.0 or 1.x is going to be enough?

>
>one of the reasons is that the codec evolved over time, I think starting from v2 it has values made more sense to determine the version info from these registers. This is also evident in the current code.
>
>
>Let me add this detail the commit log and send a v2.
>
>--srini
>> 
>>>> 
>>>> Add the version info into of_data, so that it does not need to use
>>>> core_id registers to get version number.
>>>> 
>>>> Fixes: 378918d59181 ("ASoC: codecs: lpass-macro: add helpers to get codec version")
>>>> Fixes: dbacef05898d ("ASoC: codec: lpass-rx-macro: prepare driver to accomdate new codec versions")
>>>> Fixes: 727de4fbc546 ("ASoC: codecs: lpass-wsa-macro: Correct support for newer v2.5 version")
>>> 
>>> Which commit introduced the issue? I think having just the first tag is enough.
>>> 
>>> LGTM otherwise.
>>> 
>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
>>>> ---
>>>> sound/soc/codecs/lpass-va-macro.c | 11 ++++++++++-
>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/sound/soc/codecs/lpass-va-macro.c b/sound/soc/codecs/lpass-va-macro.c
>>>> index 8454193ed22a..e95d1f29ef18 100644
>>>> --- a/sound/soc/codecs/lpass-va-macro.c
>>>> +++ b/sound/soc/codecs/lpass-va-macro.c
>>>> @@ -228,11 +228,13 @@ struct va_macro {
>>>> struct va_macro_data {
>>>> 	bool has_swr_master;
>>>> 	bool has_npl_clk;
>>>> +	int version;
>>>> };
>>>> 
>>>> static const struct va_macro_data sm8250_va_data = {
>>>> 	.has_swr_master = false,
>>>> 	.has_npl_clk = false,
>>>> +	.version = LPASS_CODEC_VERSION_1_0,
>>>> };
>>>> 
>>>> static const struct va_macro_data sm8450_va_data = {
>>>> @@ -1587,7 +1589,14 @@ static int va_macro_probe(struct platform_device *pdev)
>>>> 			goto err_npl;
>>>> 	}
>>>> 
>>>> -	va_macro_set_lpass_codec_version(va);
>>>> +	/**
>>>> +	 * old version of codecs do not have a reliable way to determine the
>>>> +	 * version from registers, get them from soc specific data
>>>> +	 */
>>>> +	if (data->version)
>>>> +		lpass_macro_set_codec_version(data->version);
>>>> +	else /* read version from register */
>>>> +		va_macro_set_lpass_codec_version(va);
>>>> 
>>>> 	if (va->has_swr_master) {
>>>> 		/* Set default CLK div to 1 */
>>> 
>>> 
>> 
>> 


-- 
With best wishes
Dmitry