linux-kernel - Re: [PATCH 4/7] ASoC: rockchip: add Serial Audio Interface (SAI) driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20e4754b-ea9a-404d-b529-ec44a7263cbf@kernel.org>
Date: Thu, 6 Mar 2025 08:37:54 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Sugar Zhang <sugar.zhang@...k-chips.com>
Cc: Luca Ceresoli <luca.ceresoli@...tlin.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>, kernel@...labora.com,
 linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH 4/7] ASoC: rockchip: add Serial Audio Interface (SAI)
 driver

On 05/03/2025 22:24, Nicolas Frattaroli wrote:
...

> +
> +static int rockchip_sai_runtime_resume(struct device *dev)
> +{
> +	struct rk_sai_dev *sai = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	int ret;
> +
> +	dev_dbg(dev, "Runtime resuming device!\n");

Drop probe entry/exit messages. Core already gives you tracing for this.

> +
> +	ret = clk_prepare_enable(sai->hclk);
> +	if (ret)
> +		goto err_hclk;
> +
> +	ret = clk_prepare_enable(sai->mclk);
> +	if (ret)
> +		goto err_mclk;
> +
> +	regcache_cache_only(sai->regmap, false);
> +	regcache_mark_dirty(sai->regmap);
> +	ret = regcache_sync(sai->regmap);
> +	if (ret)
> +		goto err_regmap;
> +
> +	if (sai->quirks & QUIRK_ALWAYS_ON && sai->is_master_mode) {
> +		spin_lock_irqsave(&sai->xfer_lock, flags);
> +		regmap_update_bits(sai->regmap, SAI_XFER,
> +				   SAI_XFER_CLK_MASK |
> +				   SAI_XFER_FSS_MASK,
> +				   SAI_XFER_CLK_EN |
> +				   SAI_XFER_FSS_EN);
> +		spin_unlock_irqrestore(&sai->xfer_lock, flags);
> +	}
> +

...

> +
> +	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register PCM: %d\n", ret);
> +		goto err_runtime_suspend;
> +	}
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +					      &rockchip_sai_component,
> +					      dai, 1);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register component: %d\n", ret);
> +		goto err_runtime_suspend;
> +	}
> +
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_put(&pdev->dev);
> +
> +	/*
> +	 * runtime_resume already enabled our hclk again, so we need to also
> +	 * get rid of the manual enable we did earlier to balance the counts.
> +	 */

Your way of handling this is extra confusing. You rely on some other
methods to poke the enable/disable count. It is rather expected that
each function handles this fully, so it disables what it have enabled.
You must not rely on PM runtime to do something with clocks which you
now unwind.

> +	clk_disable_unprepare(sai->hclk);
> +
> +	return 0;
> +
> +err_runtime_suspend:
> +	/* If we're !CONFIG_PM, we get -ENOSYS and disable manually */
> +	if (pm_runtime_put(&pdev->dev))
> +		rockchip_sai_runtime_suspend(&pdev->dev);
> +err_disable_hclk:
> +	clk_disable_unprepare(sai->hclk);
> +
> +	return ret;
> +}
> +

_device_id tables are supposed to be around probe, not beginning of the
file.

> +static void rockchip_sai_remove(struct platform_device *pdev)
> +{
> +#ifndef CONFIG_PM
> +	rockchip_sai_runtime_suspend(&pdev->dev);> +#endif
> +}
> +
> +static const struct dev_pm_ops rockchip_sai_pm_ops = {
> +	SET_RUNTIME_PM_OPS(rockchip_sai_runtime_suspend, rockchip_sai_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};
> +
> +static struct platform_driver rockchip_sai_driver = {
> +	.probe = rockchip_sai_probe,
> +	.remove = rockchip_sai_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = of_match_ptr(rockchip_sai_match),

Drop of_match_ptr, you have warning here.

> +		.pm = &rockchip_sai_pm_ops,
> +	},
> +};
> +module_platform_driver(rockchip_sai_driver);
> +
> +MODULE_DESCRIPTION("Rockchip SAI ASoC Interface");
> +MODULE_AUTHOR("Sugar Zhang <sugar.zhang@...k-chips.com>");
> +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@...labora.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


> +MODULE_DEVICE_TABLE(of, rockchip_sai_match);


Best regards,
Krzysztof