Re: [PATCH 13/13] ASoC: qcom: Add Shikra QAIF support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/06/2026 12:37, Harendra Gautam wrote:
> diff --git a/sound/soc/qcom/qaif-shikra.c b/sound/soc/qcom/qaif-shikra.c
> new file mode 100644
> index 000000000000..e83564503087
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-shikra.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * qaif-shikra.c -- ALSA SoC CPU-Platform DAI driver for QTi QAIF
> + */
> +
> +#include <linux/module.h>
> +#include <sound/pcm.h>
> +#include <sound/soc.h>
> +#include <linux/pm.h>
> +#include "qaif.h"
> +
> +struct qaif_dmaidx_dai_map shikra_aif_dma_dai_map[] = {

Why is this global? And why not const?

I have doubts this passes builds without warnings.

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1 for gcc and clang. Most of these
commands (checks or W=1 build) can build specific targets, like some
directory, to narrow the scope to only your code. The code here looks
like it needs a fix. Feel free to get in touch if the warning is not clear.

> +		{ QAIF_MI2S_TDM_AIF0 },
> +		{ QAIF_MI2S_TDM_AIF1 },
> +		{ QAIF_MI2S_TDM_AIF2 },
> +		{ QAIF_MI2S_TDM_AIF3 }
> +};
> +

...

> +
> +static int shikra_qaif_init(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +	const struct qaif_variant *v = drvdata->variant;
> +	struct device *dev = &pdev->dev;
> +	int ret, i;
> +
> +	if (!v) {
> +		dev_err(dev, "No variant data\n");
> +		return -EINVAL;
> +	}
> +	if (v->num_clks == 0 || v->num_clks > QAIF_MAX_VARIANT_CLKS) {
> +		dev_err(dev, "Invalid clock count: %d\n", v->num_clks);
> +		return -EINVAL;
> +	}
> +	drvdata->clks = devm_kcalloc(dev, v->num_clks,
> +				     sizeof(*drvdata->clks), GFP_KERNEL);
> +	if (!drvdata->clks)
> +		return -ENOMEM;
> +
> +	drvdata->num_clks = v->num_clks;
> +
> +	for (i = 0; i < drvdata->num_clks; i++)
> +		drvdata->clks[i].id = v->clk_name[i];
> +
> +	ret = devm_clk_bulk_get(dev, drvdata->num_clks, drvdata->clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clocks %d\n", ret);

Is this probe path? If so, use dev_err_probe - please look at other
drivers how they are written.

Also, probe calls should be obvious, so often suffixed  "probe" not
"init". But maybe it's not probe path...

> +		return ret;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(drvdata->num_clks, drvdata->clks);
> +	if (ret) {
> +		dev_err(dev, "shikra clk_enable failed\n");
> +		return ret;
> +	}
> +


...

> +	/* AUDIO_CORE_QAIF_AUD_INTFa_BIT_WIDTH_CFG (0x4008 + 0x1000*a) */
> +	.aif_sample_width_rx					= REG_FIELD_ID(0x4008, 24, 28, 4, 0x1000),
> +	.aif_sample_width_tx					= REG_FIELD_ID(0x4008, 16, 20, 4, 0x1000),
> +	.aif_slot_width_rx						= REG_FIELD_ID(0x4008, 8, 12, 4, 0x1000),
> +	.aif_slot_width_tx						= REG_FIELD_ID(0x4008, 0, 4, 4, 0x1000),
> +
> +	/* AUDIO_CORE_QAIF_AUD_INTFa_FRAME_CFG (0x400C + 0x1000*a) */
> +	.aif_bits_per_lane						= REG_FIELD_ID(0x400C, 0, 9, 4, 0x1000),
> +
> +	/* AUDIO_CORE_QAIF_AUD_INTFa_ACTV_SLOT_EN_TX (0x4010 + 0x1000*a) */
> +	.aif_slot_en_tx_mask					= REG_FIELD_ID(0x4010, 0, 31, 4, 0x1000),
> +
> +	/* AUDIO_CORE_QAIF_AUD_INTFa_ACTV_SLOT_EN_RX (0x4030 + 0x1000*a) */
> +	.aif_slot_en_rx_mask					= REG_FIELD_ID(0x4030, 0, 31, 4, 0x1000),
> +
> +	/* AUDIO_CORE_QAIF_AUD_INTFa_LANE_CFG (0x4050 + 0x1000*a) */
> +	.aif_loopback_en						= REG_FIELD_ID(0x4050, 31, 31, 4, 0x1000),
> +	.aif_ctrl_data_oe						= REG_FIELD_ID(0x4050, 16, 16, 4, 0x1000),
> +	.aif_lane_en							= REG_FIELD_ID(0x4050, 8, 15, 4, 0x1000),
> +	.aif_lane_dir							= REG_FIELD_ID(0x4050, 0, 7, 4, 0x1000),
> +
> +	/* AUDIO_CORE_QAIF_AUD_INTFa_MI2S_CFG (0x4054 + 0x1000*a) */
> +	.aif_mono_mode_rx						= REG_FIELD_ID(0x4054, 1, 1, 4, 0x1000),
> +	.aif_mono_mode_tx						= REG_FIELD_ID(0x4054, 0, 0, 4, 0x1000),
> +
> +	/* AUDIO_CORE_QAIF_AUD_INTFa_CFG (0x4058 + 0x1000*a) */
> +	.aif_full_cycle_en						= REG_FIELD_ID(0x4058, 0, 0, 4, 0x1000),


Honestly, I find everything above completely unreadable.

> +
> +	.clk_name			= (const char*[]) {
> +							"lpass_config_clk",
> +							"lpass_core_axim_clk",
> +							"bus_clk"
> +						},

Same here.

> +	.num_clks			= 3,
> +
> +	.dai_driver			= shikra_qaif_cpu_dai_driver,
> +	.num_dai			= ARRAY_SIZE(shikra_qaif_cpu_dai_driver),
> +
> +	.dai_osr_clk_names		= (const char *[]) {
> +							"null"
> +							},
> +	.dai_bit_clk_names		= (const char *[]) {
> +							"aif_if0_ibit_clk",
> +							"aif_if1_ibit_clk",
> +							"aif_if2_ibit_clk",
> +							"aif_if3_ibit_clk"
> +							},
> +	.init					= shikra_qaif_init,
> +	.exit					= shikra_qaif_exit,
> +	.alloc_stream_dma_idx	= shikra_qaif_alloc_stream_dma_idx,
> +	.free_stream_dma_idx	= shikra_qaif_free_stream_dma_idx,	
> +	.get_dma_idx			= shikra_qaif_get_dma_idx,
> +};
> +
> +static const struct of_device_id shikra_qaif_cpu_device_id[] = {
> +	{.compatible = "qcom,shikra-qaif-cpu", .data = &shikra_qaif_data},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, shikra_qaif_cpu_device_id);
> +
> +static struct platform_driver shikra_qaif_cpu_platform_driver = {
> +	.driver = {
> +		.name = "shikra-qaif-cpu",
> +		.of_match_table = of_match_ptr(shikra_qaif_cpu_device_id),

You have warnings here. Drop of_match_ptr.

> +		.pm = &shikra_qaif_pm_ops,
> +	},
> +	.probe = asoc_qcom_qaif_cpu_platform_probe,
> +	.remove = asoc_qcom_qaif_cpu_platform_remove,
> +	.shutdown = asoc_qcom_qaif_cpu_platform_shutdown,
> +};
> +module_platform_driver(shikra_qaif_cpu_platform_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Audio Interface (QAIF) Shikra variant driver");
> +MODULE_AUTHOR("Harendra Gautam <harendra.gautam@oss.qualcomm.com>");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof