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

[Joakim Zhang (张强庆)]

Hello,

> -----Original Message-----
> From: Ajay Kumar Nandam <ajay.nandam@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, October 7, 2026 5:29 PM
> To: Joakim Zhang (张强庆) <joakim.zhang@xxxxxxxxxxx>; lgirdwood@xxxxxxxxx;
> broonie@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> perex@xxxxxxxx; tiwai@xxxxxxxx; p.zabel@xxxxxxxxxxxxxx
> Cc: cix-kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-
> sound@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/4] ASoC: cdns: Add Cadence I2S-SC controller driver
> 
> [你通常不会收到来自 ajay.nandam@xxxxxxxxxxxxxxxx 的电子邮件。请访问
> https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要;]
> 
> EXTERNAL EMAIL
> 
> On 9/28/2026 11:06 AM, 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.
> >
> > The controller provides full-duplex stereo playback and capture in
> > standard I2S, left/right-justified and DSP modes, with mono operation
> > mapped onto the left audio channel. It also supports TDM operation
> > with up to 16 slots per frame, where the PCM channels map onto the
> > active slots of the stream direction while the bit clock covers the
> > whole frame.
> >
> > The transmitter and receiver share the transceiver enable sequence, so
> > the start/stop state machine is serialized with a spinlock against
> > independently triggered playback and capture PCMs. The driver
> > registers the dmaengine PCM helper, selects the audio reference clock
> > parent for the 8 kHz or 11.025 kHz sample rate family and keeps the
> > minimum functional-clock to bit-clock ratio required for safe clock
> > domain crossing. Runtime and system suspend/resume restore the
> > registers through the regcache.
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> > ---
> 
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <sound/dmaengine_pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +
> 
> > +static int cdns_i2s_sc_clks_enable(struct cdns_i2s_sc_priv
> > +*i2s_sc_priv) {
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(i2s_sc_priv->clk_hst);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(i2s_sc_priv->clk_i2s);
> > +     if (ret)
> > +             clk_disable_unprepare(i2s_sc_priv->clk_hst);
> > +
> > +     return ret;
> > +}
> > +
> > +static void cdns_i2s_sc_clks_disable(struct cdns_i2s_sc_priv
> > +*i2s_sc_priv) {
> > +     clk_disable_unprepare(i2s_sc_priv->clk_i2s);
> > +     clk_disable_unprepare(i2s_sc_priv->clk_hst);
> > +}
> > +
> > +static void cdns_i2s_sc_rxtx_common_config(struct cdns_i2s_sc_priv
> > +*i2s_sc_priv, bool on) {
> > +     if (on) {
> > +             /* Full-duplex mode enable */
> > +             regmap_update_bits(i2s_sc_priv->regmap, I2S_CTRL_FDX,
> > +                                I2S_CTRL_FDX_FULL_DUPLEX,
> > +I2S_CTRL_FDX_FULL_DUPLEX);
> > +
> > +             if (i2s_sc_priv->is_tdm_mode) {
> > +                     /* TDM mode enable */
> > +                     regmap_update_bits(i2s_sc_priv->regmap, I2S_TDM_CTRL,
> > +                                        I2S_TDM_CTRL_TDM_EN,
> > + I2S_TDM_CTRL_TDM_EN);
> > +
> > +                     /* Number of supported audio channels in TDM mode */
> > +                     regmap_update_bits(i2s_sc_priv->regmap, I2S_TDM_CTRL,
> > +                                        I2S_TDM_CTRL_CHN_NO,
> > +                                        FIELD_PREP(I2S_TDM_CTRL_CHN_NO,
> > +
> > + i2s_sc_priv->tdm_config.slots - 1));
> 
> This driver uses FIELD_PREP() in several register programming paths, but it does not
> include <linux/bitfield.h>. A local targeted build with
> 
>   M=sound/soc/cdns W=1 fails with:
> 
>   error: implicit declaration of function 'FIELD_PREP'
> 
> Please add the explicit bitfield.h include. After adding that include locally, cdns-i2s-
> sc.c gets past this compile error.

Yes, will update in v3.

> > +
> > +                     /* TDM mode channels enable */
> > +                     regmap_update_bits(i2s_sc_priv->regmap, I2S_TDM_CTRL,
> I2S_TDM_CTRL_CHN_EN,
> > +                                        FIELD_PREP(I2S_TDM_CTRL_CHN_EN,
> > +                                                   i2s_sc_priv->tdm_config.rx_mask |
> > +                                                   i2s_sc_priv->tdm_config.tx_mask));
> > +             }
> 
> > +     ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret,
> > +                                  "Failed to register dmaengine
> > + component\n");
> > +
> > +     pm_runtime_get_noresume(&pdev->dev);
> > +     pm_runtime_set_active(&pdev->dev);
> > +     ret = devm_pm_runtime_enable(&pdev->dev);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret, "Failed to enable
> > + runtime PM\n");
> > +
> > +     ret = cdns_i2s_sc_clks_enable(i2s_sc_priv);
> > +     if (ret) {
> > +             dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
> > +             pm_runtime_put_noidle(&pdev->dev);
> > +             return ret;
> > +     }
> > +
> 
> After cdns_i2s_sc_clks_enable() succeeds, the platform_get_irq() failure path
> returns directly without disabling the clocks or dropping the runtime PM usage
> count. The devm_request_irq() failure path has a similar issue: it calls
> pm_runtime_put_noidle(), but that will not run the runtime suspend callback, so the
> clocks enabled manually above remain prepared/enabled.

Yes, thanks, will update in v3.

Joakim