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

[Joakim Zhang (张强庆)]

Hello Mark,

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Tuesday, September 22, 2026 10:35 PM
> To: Joakim Zhang (张强庆) <joakim.zhang@xxxxxxxxxxx>
> Cc: lgirdwood@xxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; p.zabel@xxxxxxxxxxxxxx;
> cix-kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-
> sound@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/4] ASoC: cdns: Add Cadence I2S-SC controller driver
> 
> On Tue, Sep 22, 2026 at 07:21:32PM +0800, joakim.zhang@xxxxxxxxxxx wrote:
> > From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> >
> > 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.
OK.

> > +++ 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.
OK.

> > +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.
Make sense.

> > +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.
Will update to devm_pm_runtime_enable().

> > +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.
Yes.

Thanks,
Joakim