Re: [PATCH v2 1/5] ASoC: codecs: va-macro: Rework version checking

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Thu Oct 9, 2025 at 3:36 PM BST, Prasad Kumpatla wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Open-code some of the registers to make the checks anywhere near human-
> readable. Error out if the version is unsupported or if the VA macro
> isn't supposed to be present within this LPASS instance (since we can
> check for that now).

[...]

> +static int va_macro_set_lpass_codec_version(struct va_macro *va)
>  {
> -	int core_id_0 = 0, core_id_1 = 0, core_id_2 = 0;
>  	int version = LPASS_CODEC_VERSION_UNKNOWN;
> +	u32 maj, min, step;
> +	u32 val;
>  
> -	regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_0, &core_id_0);
> -	regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_1, &core_id_1);
> -	regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_2, &core_id_2);
> +	regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_0, &val);
> +	maj = FIELD_GET(CORE_ID_0_REV_MAJ, val);
>  
> -	if ((core_id_0 == 0x01) && (core_id_1 == 0x0F))
> -		version = LPASS_CODEC_VERSION_2_0;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && core_id_2 == 0x01)
> +	regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_1, &val);
> +	if (!FIELD_GET(CORE_ID_1_HAS_VAMACRO, val)) {
> +		dev_err(va->dev, "This is not a VA macro instance\n");
> +		return -ENODEV;
> +	}
> +
> +	regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_2, &val);
> +	min = FIELD_GET(CORE_ID_2_REV_MIN, val);
> +	step = FIELD_GET(CORE_ID_2_REV_STEP, val);
> +
> +	if (maj == 1) {
>  		version = LPASS_CODEC_VERSION_2_0;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0E))
> -		version = LPASS_CODEC_VERSION_2_1;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x50 || core_id_2 == 0x51))
> -		version = LPASS_CODEC_VERSION_2_5;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x60 || core_id_2 == 0x61))
> -		version = LPASS_CODEC_VERSION_2_6;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x70 || core_id_2 == 0x71))
> -		version = LPASS_CODEC_VERSION_2_7;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x80 || core_id_2 == 0x81))
> -		version = LPASS_CODEC_VERSION_2_8;
> -	if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x90 || core_id_2 == 0x91))
> -		version = LPASS_CODEC_VERSION_2_9;
> -
> -	if (version == LPASS_CODEC_VERSION_UNKNOWN)
> -		dev_warn(va->dev, "Unknown Codec version, ID: %02x / %02x / %02x\n",
> -			 core_id_0, core_id_1, core_id_2);
> +	} else if (maj == 2) {
> +		switch (min) {
> +		case 0:
> +			version = LPASS_CODEC_VERSION_2_0;
> +			break;
> +		case 5:
> +			version = LPASS_CODEC_VERSION_2_5;
> +			break;
> +		case 6:
> +			version = LPASS_CODEC_VERSION_2_6;
> +			break;
> +		case 7:
> +			version = LPASS_CODEC_VERSION_2_7;
> +			break;
> +		case 8:
> +			version = LPASS_CODEC_VERSION_2_8;
> +			break;
> +		case 9:
> +			version = LPASS_CODEC_VERSION_2_9;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (version == LPASS_CODEC_VERSION_UNKNOWN) {
> +		dev_err(va->dev, "VA Macro v%u.%u.%u is not supported\n",
> +			maj, min, step);
> +		return -EOPNOTSUPP;
> +	}

Why?

As far as I understand the behaviour before this change is to continue
even with unsupported LPASS va macro version. IIRC when I enabled sound
on Kaanapali QRD device it worked even with unsupported version, it just
needed a fix to calm down the warning.

Why this needed to be changed to error out as unsupported now? Will there
be a permanent damage to hw/fw if we continue?
Shouldn't this print a warning that execution continues regardless but
with low expectations?

I kinda get why we should give up if va macro instance is not present
but not about change of behaviour here.


>  	lpass_macro_set_codec_version(version);
>  
>  	dev_dbg(va->dev, "LPASS Codec Version %s\n", lpass_macro_get_codec_version_string(version));
> +
> +	return 0;
>  }
>  
>  static int va_macro_probe(struct platform_device *pdev)
> @@ -1594,10 +1626,14 @@ static int va_macro_probe(struct platform_device *pdev)
>  	 * 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)
> +	if (data->version) {
>  		lpass_macro_set_codec_version(data->version);
> -	else /* read version from register */
> -		va_macro_set_lpass_codec_version(va);
> +	} else {
> +		/* read version from register */
> +		ret = va_macro_set_lpass_codec_version(va);
> +		if (ret)
> +			return ret;
> +	}

Thanks,
Alexey