RE: [PATCH v9 2/4] clk: cix: add sky1 audss clock controller

[Joakim Zhang]

Hello Brian,

Thanks for your kindly review.

> -----Original Message-----
> From: Brian Masney <bmasney@xxxxxxxxxx>
> Sent: Friday, July 17, 2026 7:57 AM
> To: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> Cc: mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxx; robh@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; cix-kernel-
> upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v9 2/4] clk: cix: add sky1 audss clock controller

[...]
> > diff --git a/drivers/clk/cix/clk-sky1-audss.c
> > b/drivers/clk/cix/clk-sky1-audss.c
> > new file mode 100644
> > index 000000000000..5f9133232eb1
> > --- /dev/null
> > +++ b/drivers/clk/cix/clk-sky1-audss.c
> > @@ -0,0 +1,1205 @@
> > +// SPDX-License-Identifier: GPL-2.0-only // Copyright 2026 Cix
> > +Technology Group Co., Ltd.
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> 
> You can drop this include since you are only using of_device_id. You will get it
> via two different chains:
> 
> <linux/platform_device.h>
>         -> <linux/device.h>
>         -> <linux/device/driver.h>
>         -> <linux/device-id/of.h>
> 
>  linux/clk-provider.h -> linux/of.h -> linux/device-id/of.h
> (Pasted from a review on a different series a little bit ago.)
Fine, will drop of_device.h.


[...]
> > +/* 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 *dev = &pdev->dev;
> > +     struct clk_hw **clk_table;
> > +     void __iomem *base;
> > +     int i, ret;
> > +
> > +     devtype_data = device_get_match_data(dev);
> > +     if (!devtype_data)
> > +             return -ENODEV;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     spin_lock_init(&priv->lock);
> > +
> > +     priv->clk_data = devm_kzalloc(dev,
> > +                                   struct_size(priv->clk_data, hws,
> SKY1_AUDSS_NUM_CLKS),
> > +                                   GFP_KERNEL);
> > +     if (!priv->clk_data)
> > +             return -ENOMEM;
> > +
> > +     priv->clk_data->num = SKY1_AUDSS_NUM_CLKS;
> > +     clk_table = priv->clk_data->hws;
> > +
> > +     base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     priv->regmap_cru = devm_regmap_init_mmio(dev, base,
> &sky1_audss_regmap_config);
> > +     if (IS_ERR(priv->regmap_cru))
> > +             return dev_err_probe(dev, PTR_ERR(priv->regmap_cru),
> > +                                  "failed to initialize regmap\n");
> > +
> > +     priv->dev = dev;
> > +     priv->devtype_data = devtype_data;
> > +
> > +     priv->rst_noc = devm_reset_control_get_exclusive(dev, NULL);
> > +     if (IS_ERR(priv->rst_noc))
> > +             return dev_err_probe(dev, PTR_ERR(priv->rst_noc),
> > +                                  "failed to get audss noc reset");
> > +
> > +     reset_control_assert(priv->rst_noc);
> > +
> > +     reset_control_deassert(priv->rst_noc);
> > +
> > +     pm_runtime_get_noresume(dev);
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     platform_set_drvdata(pdev, priv);
> > +
> > +     ret = sky1_audss_clks_get(priv);
> > +     if (ret)
> > +             goto err_pm;
> > +
> > +     ret = sky1_audss_clks_enable(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to enable clocks\n");
> > +             goto err_pm;
> > +     }
> > +
> > +     ret = sky1_audss_clks_set_rate(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to set clocks rate\n");
> > +             goto fail_clks_set;
> 
> So starting here... Is there a double put below? I'll flag it below.
> 
> > +     }
> > +
> > +     /* audio_clk4 clock fixed divider */
> > +     clk_table[CLK_AUD_CLK4_DIV2] =
> > +             devm_clk_hw_register_fixed_factor(dev,
> > +                                               "audio_clk4_div2",
> > +                                               "audio_clk4",
> > +                                               0,
> > +                                               1, 2);
> > +     if (IS_ERR(clk_table[CLK_AUD_CLK4_DIV2])) {
> > +             ret = PTR_ERR(clk_table[CLK_AUD_CLK4_DIV2]);
> > +             dev_err(dev, "failed to register clock %d, ret:%d\n",
> CLK_AUD_CLK4_DIV2, ret);
> > +             goto fail_fixed_clk;
> > +     }
> > +
> > +     clk_table[CLK_AUD_CLK4_DIV4] =
> > +             devm_clk_hw_register_fixed_factor(dev,
> > +                                               "audio_clk4_div4",
> > +                                               "audio_clk4",
> > +                                               0,
> > +                                               1, 4);
> > +     if (IS_ERR(clk_table[CLK_AUD_CLK4_DIV4])) {
> > +             ret = PTR_ERR(clk_table[CLK_AUD_CLK4_DIV4]);
> > +             dev_err(dev, "failed to register clock %d, ret:%d\n",
> CLK_AUD_CLK4_DIV4, ret);
> > +             goto fail_fixed_clk;
> > +     }
> > +
> > +     /* audio_clk5 clock fixed divider */
> > +     clk_table[CLK_AUD_CLK5_DIV2] =
> > +             devm_clk_hw_register_fixed_factor(dev,
> > +                                               "audio_clk5_div2",
> > +                                               "audio_clk5",
> > +                                               0,
> > +                                               1, 2);
> > +     if (IS_ERR(clk_table[CLK_AUD_CLK5_DIV2])) {
> > +             ret = PTR_ERR(clk_table[CLK_AUD_CLK5_DIV2]);
> > +             dev_err(dev, "failed to register clock %d, ret:%d\n",
> CLK_AUD_CLK5_DIV2, ret);
> > +             goto fail_fixed_clk;
> > +     }
> > +
> > +     for (i = 0; i < devtype_data->clk_cfg_size; i++) {
> > +             clk_table[devtype_data->clk_cfg[i].id] =
> > +                     sky1_audss_clk_register(dev,
> > +                                             devtype_data->clk_cfg[i].name,
> > +                                             devtype_data->clk_cfg[i].parent_names,
> > +                                             devtype_data->clk_cfg[i].num_parents,
> > +                                             priv->regmap_cru,
> > +                                             devtype_data->clk_cfg[i].mux_table,
> > +                                             devtype_data->clk_cfg[i].mux_cfg,
> > +                                             devtype_data->clk_cfg[i].div_cfg,
> > +                                             devtype_data->clk_cfg[i].gate_cfg,
> > +                                             devtype_data->clk_cfg[i].flags,
> > +                                             &priv->lock);
> > +             if (IS_ERR(clk_table[devtype_data->clk_cfg[i].id])) {
> > +                     ret = PTR_ERR(clk_table[devtype_data->clk_cfg[i].id]);
> > +                     dev_err(dev, "failed to register clock %d, ret:%d\n",
> > +                             devtype_data->clk_cfg[i].id, ret);
> > +                     goto fail_fixed_clk;
> > +             }
> > +     }
> > +
> > +     ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv-
> >clk_data);
> > +     if (ret) {
> > +             dev_err(dev, "failed to add clock provider: %d\n", ret);
> > +             goto fail_register;
> > +     }
> > +
> > +     ret = sky1_audss_reset_controller_register(dev);
> > +     if (ret)
> > +             goto fail_register;
> > +
> > +     pm_runtime_put_sync(dev);
> > +
> > +     return 0;
> > +
> > +fail_register:
> > +fail_fixed_clk:
> > +fail_clks_set:
> 
> Collapse these 3 labels into 1.
> 
> > +     pm_runtime_put_sync(dev);
> > +err_pm:
> > +     pm_runtime_put_noidle(dev);
> 
> So above where I flagged the 'goto fail_clks_set', is this a double put?
On the double-put concern: fail_* paths run put_sync() (usage 1->0) then fall through to put_noidle(). put_noidle() usesatomic_add_unless(..., -1, 0), so it is a no-op when the counter is already 0. But it still seems rather strange, I will refine the sequence to avoid it.


> > +     pm_runtime_disable(dev);
> > +     return ret;
> > +}
> > +
> > +static void sky1_audss_clk_remove(struct platform_device *pdev) {
> > +     struct device *dev = &pdev->dev;
> > +
> > +     if (!pm_runtime_status_suspended(dev))
> > +             pm_runtime_force_suspend(dev);
> > +
> > +     pm_runtime_disable(dev);
> 
> I am fairly certain that remove() is called before the devm handlers are
> invoked. The clocks will be shutdown, but there's a window where they are still
> visible to consumers. I think you should use
> devm_add_action_or_reset() so that the cleanup is called in the expected order.
Agreed. remove() runs before devres_release_all(), so force_suspend() tears down the hardware while the OF clock provider is still visible. I'll move the runtime PM cleanup into a devm_add_action_or_reset() registered before the clocks/provider, and drop it from remove().

> > +}
> > +
> > +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 reset_control_assert(priv->rst_noc);
> > +}
> > +
> > +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 = reset_control_deassert(priv->rst_noc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = sky1_audss_clks_enable(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to enable clocks\n");
> > +             reset_control_assert(priv->rst_noc);
> > +             return ret;
> > +     }
> > +
> > +     spin_lock_irqsave(&priv->lock, flags);
> > +     for (i = 0; i < devtype_data->reg_save_size; i++)
> > +             regmap_write(priv->regmap_cru,
> > +                          devtype_data->reg_save[i][0],
> > + devtype_data->reg_save[i][1]);
> 
> Does it matter what order the registers are written on resume?
No, it doesn't matter.

I will send the v10 soon.

Thanks,
Joakim