RE: [PATCH v2 4/5] clk: cix: add sky1 audss clock controller

[Joakim Zhang]

Hi Philipp,

> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: Friday, June 5, 2026 3:42 PM
> To: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>; mturquette@xxxxxxxxxxxx;
> sboyd@xxxxxxxxxx; bmasney@xxxxxxxxxx; robh@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; Gary Yang
> <gary.yang@xxxxxxxxxxx>
> Cc: cix-kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-
> clk@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 4/5] clk: cix: add sky1 audss clock controller
> 
> EXTERNAL EMAIL
> 
> CAUTION: Suspicious Email from unusual domain.
> 
> On Fr, 2026-06-05 at 11:22 +0800, joakim.zhang@xxxxxxxxxxx wrote:
> > From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> >
> > Add a platform driver for the Cix Sky1 Audio Subsystem (AUDSS)
> > internal clock controller. The driver binds to a cix,sky1-audss-clock
> > device tree node under the AUDSS syscon, obtains the parent regmap via
> > syscon_node_to_regmap(), and registers mux/divider/gate composite
> > clocks for DSP, SRAM, HDA, DMAC, watchdog, timer, mailbox and I2S
> > outputs. Six SoC-level audio reference clocks are brought up as inputs to the
> tree.
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> > ---
> >  drivers/clk/Kconfig              |    1 +
> >  drivers/clk/Makefile             |    1 +
> >  drivers/clk/cix/Kconfig          |   16 +
> >  drivers/clk/cix/Makefile         |    3 +
> >  drivers/clk/cix/clk-sky1-audss.c | 1129
> > ++++++++++++++++++++++++++++++
> >  5 files changed, 1150 insertions(+)
> >  create mode 100644 drivers/clk/cix/Kconfig  create mode 100644
> > drivers/clk/cix/Makefile  create mode 100644
> > drivers/clk/cix/clk-sky1-audss.c
> >
> [...]
> > diff --git a/drivers/clk/cix/clk-sky1-audss.c
> > b/drivers/clk/cix/clk-sky1-audss.c
> > new file mode 100644
> > index 000000000000..899452d5ed14
> > --- /dev/null
> > +++ b/drivers/clk/cix/clk-sky1-audss.c
> > @@ -0,0 +1,1129 @@
> [...]
> > +/* register sky1 audio subsystem clocks */ static int
> > +sky1_audss_clk_probe(struct platform_device *pdev) {
> > +     const struct sky1_audss_clks_devtype_data *devtype_data;
> > +     struct sky1_audss_clks_priv *priv;
> > +     struct device_node *parent_np;
> > +     struct device *dev = &pdev->dev;
> > +     struct reset_control *rst_noc;
> > +     struct clk_hw **clk_table;
> > +     struct regmap *regmap_cru;
> > +     int i, ret;
> > +
> > +     parent_np = of_get_parent(pdev->dev.of_node);
> > +     regmap_cru = syscon_node_to_regmap(parent_np);
> > +     of_node_put(parent_np);
> > +     if (IS_ERR(regmap_cru))
> > +             return dev_err_probe(dev, PTR_ERR(regmap_cru),
> > +                                  "unable to get audss cru regmap");
> > +
> > +     devtype_data = device_get_match_data(dev);
> > +     if (!devtype_data)
> > +             return -ENODEV;
> > +
> > +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     spin_lock_init(&priv->lock);
> > +
> > +     priv->clk_data = devm_kzalloc(&pdev->dev,
> > +                                   struct_size(priv->clk_data, hws, AUDSS_MAX_CLKS),
> > +                                   GFP_KERNEL);
> > +     if (!priv->clk_data)
> > +             return -ENOMEM;
> > +
> > +     priv->clk_data->num = AUDSS_MAX_CLKS;
> > +     clk_table = priv->clk_data->hws;
> > +
> > +     priv->dev = dev;
> > +     priv->regmap_cru = regmap_cru;
> > +     priv->devtype_data = devtype_data;
> > +
> > +     ret = sky1_audss_clks_get(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     rst_noc = devm_reset_control_get(dev, NULL);
> 
> Please use devm_reset_control_get_exclusive() directly.
OK

> [...]
> > +static int __maybe_unused sky1_audss_clk_runtime_suspend(struct
> > +device *dev) {
> > +     struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
> > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> >devtype_data;
> > +     unsigned long flags;
> > +     int i;
> > +
> > +     spin_lock_irqsave(&priv->lock, flags);
> > +     for (i = 0; i < devtype_data->reg_save_size; i++)
> > +             regmap_read(priv->regmap_cru,
> > +                         devtype_data->reg_save[i][0], &devtype_data-
> >reg_save[i][1]);
> > +     spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +     sky1_audss_clks_disable(priv);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused sky1_audss_clk_runtime_resume(struct device
> > +*dev) {
> > +     struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
> > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> >devtype_data;
> > +     unsigned long flags;
> > +     int i, ret;
> > +
> > +     ret = sky1_audss_clks_enable(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to enable clocks\n");
> > +             return ret;
> > +     }
> > +
> > +     reset_control_deassert(priv->rst_noc);
> 
> Deasserted on resume but not asserted on suspend, is this on purpose?
No, there was no specific purpose. It was because I overlooked it and will add it in v3.

Thanks,
Joakim