Re: [PATCH 2/4] ASoC: cdns: Add Cadence I2S-SC controller driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Tue, Sep 22, 2026 at 07:21:32PM +0800, joakim.zhang@cixtech.com wrote:
> From: Joakim Zhang <joakim.zhang@cixtech.com>
> 
> Add support for the Cadence I2S-SC controller found in the CIX SKY1
> audio subsystem.

>  
>  obj-$(CONFIG_SND_SOC)	+= snd-soc-core.o
> +obj-$(CONFIG_SND_SOC)	+= cdns/
>  obj-$(CONFIG_SND_SOC)	+= codecs/

This should be sorted in with the vendors, after bcm.

> +++ b/sound/soc/cdns/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_SND_SOC_CDNS_I2S_SC) += snd-soc-cdns-i2s-sc.o
> +snd-soc-cdns-i2s-sc-objs := cdns-i2s-sc.o

We've moved to module-y for module objects.

> +static int cdns_i2s_sc_probe(struct platform_device *pdev)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv;
> +	struct resource *res;
> +	void __iomem *base;
> +	int i, irq, ret;

> +	ret = devm_request_irq(&pdev->dev, irq, cdns_i2s_sc_isr, 0,
> +			       dev_name(&pdev->dev), i2s_sc_priv);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
> +		return ret;
> +	}

We request the interrupt before we enable clocks or anything, an
interrupt could be delivered as soon as we do the request and the
handler will try to interact with the hardware which I imagine might go
badly if it's unclocked or in reset (at least for some integrations).
It's generally easier to make this one of the last thing we do in probe,
once most things are ready.

> +static void cdns_i2s_sc_remove(struct platform_device *pdev)
> +{
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		pm_runtime_force_suspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +}

For CONFIG_PM=n forcing suspend won't do what you want, and when it's
enabled it'll also disable.  devm_pm_runtime_enable() might be easier.

> +static const struct dev_pm_ops cdns_i2s_sc_pm_ops = {
> +	SET_RUNTIME_PM_OPS(cdns_i2s_sc_runtime_suspend,
> +			   cdns_i2s_sc_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(cdns_i2s_sc_system_suspend,
> +				cdns_i2s_sc_system_resume)
> +};


RUNTIME_PM_OPS() and SYSTEM_SLEEP_PM_OPS() are the more modern things,
and avoid the need for __maybe_unused.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]