Re: [PATCH v2 07/11] ASoC: qcom: Add QAIF regmap, DT parsing and platform init

[Krzysztof Kozlowski]

On Wed, Jul 01, 2026 at 04:27:53PM +0530, Harendra Gautam wrote:
> +/**
> + * asoc_qcom_qaif_cpu_platform_probe - Probe the QAIF CPU and platform driver
> + * @pdev: Platform device
> + *
> + * Initialises the QAIF regmap, parses DT, sets up clocks and registers
> + * the CPU DAI component and PCM platform.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int asoc_qcom_qaif_cpu_platform_probe(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata;
> +	struct resource *res;
> +	const struct qaif_variant *variant;
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *match;
> +	int ret, i, dai_id, idx;
> +	bool variant_init_done = false;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	match = of_match_device(dev->driver->of_match_table, dev);
> +	if (!match || !match->data)
> +		return -EINVAL;
> +
> +	drvdata->variant = match->data;
> +	variant = drvdata->variant;
> +
> +	ret = of_qaif_parse_aif_intf_cfg(dev, drvdata);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to parse aif interfaces\n");
> +
> +	drvdata->audio_qaif =
> +			devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drvdata->audio_qaif))
> +		return PTR_ERR(drvdata->audio_qaif);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	audio_qaif_regmap_config.max_register = resource_size(res);

Why are you modifiying static variable? So if second instance is probed,
how is this supposed to work?

> +	drvdata->audio_qaif_map =
> +		devm_regmap_init_mmio(dev, drvdata->audio_qaif,
> +				      &audio_qaif_regmap_config);
> +	if (IS_ERR(drvdata->audio_qaif_map))
> +		return PTR_ERR(drvdata->audio_qaif_map);
> +
> +	ret = of_qaif_cdc_dma_clks_parse(dev, drvdata);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get cdc dma clocks\n");
> +
> +	if (variant->clk_init) {
> +		ret = variant->clk_init(pdev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to initialize variant\n");
> +		variant_init_done = true;
> +	}
> +
> +	for (i = 0; i < variant->num_dai; i++) {
> +		dai_id = variant->dai_driver[i].id;
> +		if (is_cif_dma_port(dai_id))
> +			continue;
> +		idx = variant->get_dma_idx(dai_id);
> +		if (idx < 0)
> +			continue;
> +
> +		drvdata->mi2s_bit_clk[idx] = devm_clk_get(dev,
> +							  variant->dai_bit_clk_names[idx]);
> +		if (IS_ERR(drvdata->mi2s_bit_clk[idx])) {
> +			ret = PTR_ERR(drvdata->mi2s_bit_clk[idx]);

Wrong syntax. Please use dev_err_probe.


> +			dev_err_probe(dev, ret, "error getting %s\n",
> +				      variant->dai_bit_clk_names[idx]);
> +			goto err;
> +		}
> +	}
> +
> +	ret = qaif_aif_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "error init aif bitfields\n");
> +		goto err;
> +	}
> +
> +	ret = qaif_aif_cfg_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "error init aif intfctl fields\n");
> +		goto err;
> +	}
> +
> +	ret = qaif_cif_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "error init cif bitfields\n");
> +		goto err;
> +	}

Why do you need to enable the clocks before all this init? Do you
actually access the device here (like its registers)?

> +
> +	ret = devm_snd_soc_register_component(dev,
> +					      &qaif_cpu_comp_driver,
> +					      variant->dai_driver,
> +					      variant->num_dai);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "error registering cpu driver\n");
> +		goto err;
> +	}
> +
> +	ret = asoc_qcom_qaif_platform_register(pdev);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "error registering platform driver\n");
> +		goto err;
> +	}
> +err:
> +	if (ret && variant_init_done && variant->clk_deinit)
> +		variant->clk_deinit(pdev);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_probe);
> +
> +/**
> + * asoc_qcom_qaif_cpu_platform_remove - Remove the QAIF CPU and platform driver
> + * @pdev: Platform device
> + */
> +void asoc_qcom_qaif_cpu_platform_remove(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +
> +	if (drvdata->variant->clk_deinit)
> +		drvdata->variant->clk_deinit(pdev);
> +}
> +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_remove);
> +
> +/**
> + * asoc_qcom_qaif_cpu_platform_shutdown - Shutdown the QAIF CPU and platform driver
> + * @pdev: Platform device
> + */
> +void asoc_qcom_qaif_cpu_platform_shutdown(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +
> +	if (drvdata->variant->clk_deinit)
> +		drvdata->variant->clk_deinit(pdev);
> +}
> +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_shutdown);

I do not understand why do you need wrappers over single call of single
driver. You do not have different variants. And even if you had
different variants, I claim they will have exactly the same clk init and
deinit.

Again, this looks awfully like sending us a dowsntream driver which is a
known antipattern of upstreaming.


Best regards,
Krzysztof