Re: [PATCH 10/13] ASoC: qcom: Add QAIF regmap, DT parsing and platform init

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/06/2026 12:37, Harendra Gautam wrote:
> +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;
> +
> +	dev_dbg(dev, "%s\n", __func__);

NAK, see further. This is clear, old known antipattern. Don't send such
code anymore.

> +	drvdata = devm_kzalloc(dev, sizeof(struct qaif_drv_data), GFP_KERNEL);

sizeof(*), and with third argument - lack of dev_err_probe - means you
just sent us old junk vendor code.

That's waste of the time - we will have to point you all standard
issues, we solved already years ago.

No. Drop all this code and start from current kernel code as your base
so you won't be repeating same old poor patterns.

> +	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 = (const struct qaif_variant *)match->data;

Why do you need to cast?

> +	variant = drvdata->variant;
> +	if (!variant) {
> +		dev_err(dev, "No variant data\n");

Is it a possible condition? Probably no, so why printing messages?

> +		return -EINVAL;
> +	}
> +
> +	ret = of_qaif_parse_aif_intf_cfg(dev, drvdata);
> +	if (ret) {
> +		dev_err(dev, "Failed to parse aif interfaces: %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	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);
> +
> +	drvdata->audio_qaif_map = devm_regmap_init_mmio(dev, drvdata->audio_qaif,
> +				&audio_qaif_regmap_config);

Your code is barely readable.

> +	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) {
> +		dev_err(dev, "failed to get cdc dma clocks %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (variant->init) {
> +		ret = variant->init(pdev);
> +		if (ret) {
> +			dev_err(dev, "error initializing variant: %d\n", ret);
> +			return ret;
> +		}
> +		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])) {
> +			dev_err(dev,
> +				"error getting %s: %ld\n",
> +				variant->dai_bit_clk_names[idx],
> +				PTR_ERR(drvdata->mi2s_bit_clk[idx]));
> +			ret = PTR_ERR(drvdata->mi2s_bit_clk[idx]);

Heh....

> +			goto err;
> +		}
> +	}
> +
> +	ret = qaif_aif_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> +	if (ret) {
> +		dev_err(dev, "error init cif bitfield: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = qaif_aif_cfg_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> +	if (ret) {
> +		dev_err(dev, "error init aif_intfctl field: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = qaif_cif_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> +	if (ret) {
> +		dev_err(dev, "error init cif bitfield: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = devm_snd_soc_register_component(dev, &qaif_cpu_comp_driver,
> +								variant->dai_driver,
> +								variant->num_dai);
> +	if (ret) {
> +		dev_err(dev, "error registering cpu driver: %d\n", ret);

Why aren't you using modern style? dev_err_probe?

> +		goto err;
> +	}
> +
> +	ret = asoc_qcom_qaif_platform_register(pdev);
> +	if (ret) {
> +		dev_err(dev, "error registering platform driver: %d\n", ret);
> +		goto err;
> +	}
> +	dev_dbg(&pdev->dev, "%s: QAIF CPU-Platform Driver Registered Successfully\n", __func__);

Drop. This does not look like useful printk message. Drivers should be
silent on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79

Especially trivial probe successes are useless - core handles it.

> +err:
> +	if (ret && variant_init_done && variant->exit)
> +		variant->exit(pdev);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_probe);

Every exported function should have kerneldoc.

> +
> +void asoc_qcom_qaif_cpu_platform_remove(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +
> +	if (drvdata->variant->exit)
> +		drvdata->variant->exit(pdev);
> +}
> +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_remove);
> +




Best regards,
Krzysztof