RE: [PATCH v8 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: Wednesday, July 8, 2026 12:36 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 v8 2/4] clk: cix: add sky1 audss clock controller
> 
> EXTERNAL EMAIL
> 
> Hi Joakim,
> 
> Thanks for the patch.
> 
> On Tue, Jun 30, 2026 at 08:44:11PM +0800, joakim.zhang@xxxxxxxxxxx wrote:
> > From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> >
> > Add a platform driver for the Cix Sky1 AUDSS CRU. The driver maps the
> > CRU registers and registers mux, divider and gate clocks for DSP,
> > SRAM, HDA, DMAC, I2S, mailbox, watchdog and timer blocks.
> >
> > Four SoC-level audio reference clocks are enabled as inputs to the
> > internal clock tree. The driver releases the AUDSS NOC reset, enables
> > runtime PM and instantiates the auxiliary reset device.
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> 
> [snip]
> 
> > +static struct clk_hw *sky1_audss_clk_register(struct device *dev,
> > +                                           const char *name,
> > +                                           const char * const *parent_names,
> > +                                           int num_parents,
> > +                                           struct regmap *regmap,
> > +                                           const u32 *mux_table,
> > +                                           struct muxdiv_cfg *mux_cfg,
> > +                                           struct muxdiv_cfg *div_cfg,
> > +                                           struct gate_cfg *gate_cfg,
> > +                                           unsigned long flags,
> > +                                           spinlock_t *lock) {
> > +     const struct clk_ops *sky1_mux_ops = NULL;
> > +     const struct clk_ops *sky1_div_ops = NULL;
> > +     const struct clk_ops *sky1_gate_ops = NULL;
> > +     struct clk_hw *hw = ERR_PTR(-ENOMEM);
> > +     struct sky1_clk_divider *sky1_div = NULL;
> > +     struct sky1_clk_gate *sky1_gate = NULL;
> > +     struct sky1_clk_mux *sky1_mux = NULL;
> 
> Reverse Christmas tree order please.
OK

> > +
> > +     if (mux_cfg->offset >= 0) {
> > +             sky1_mux = devm_kzalloc(dev, sizeof(*sky1_mux), GFP_KERNEL);
> > +             if (!sky1_mux)
> > +                     return ERR_PTR(-ENOMEM);
> > +
> > +             sky1_mux->mux.reg = NULL;
> > +             sky1_mux->mux.shift = mux_cfg->shift;
> > +             sky1_mux->mux.mask = BIT(mux_cfg->width) - 1;
> > +             sky1_mux->mux.flags = mux_cfg->flags;
> > +             sky1_mux->mux.table = mux_table;
> > +             sky1_mux->mux.lock = lock;
> > +             sky1_mux_ops = &sky1_audss_clk_mux_ops;
> > +             sky1_mux->regmap = regmap;
> > +             sky1_mux->offset = mux_cfg->offset;
> > +     }
> > +
> > +     if (div_cfg->offset >= 0) {
> > +             sky1_div = devm_kzalloc(dev, sizeof(*sky1_div), GFP_KERNEL);
> > +             if (!sky1_div)
> > +                     return ERR_PTR(-ENOMEM);
> > +
> > +             sky1_div->div.reg = NULL;
> > +             sky1_div->div.shift = div_cfg->shift;
> > +             sky1_div->div.width = div_cfg->width;
> > +             sky1_div->div.flags = div_cfg->flags |
> CLK_DIVIDER_POWER_OF_TWO;
> > +             sky1_div->div.lock = lock;
> > +             sky1_div_ops = &sky1_audss_clk_divider_ops;
> > +             sky1_div->regmap = regmap;
> > +             sky1_div->offset = div_cfg->offset;
> > +     }
> > +
> > +     if (gate_cfg->offset >= 0) {
> > +             sky1_gate = devm_kzalloc(dev, sizeof(*sky1_gate), GFP_KERNEL);
> > +             if (!sky1_gate)
> > +                     return ERR_PTR(-ENOMEM);
> > +
> > +             sky1_gate->gate.reg = NULL;
> > +             sky1_gate->gate.bit_idx = gate_cfg->shift;
> > +             sky1_gate->gate.flags = gate_cfg->flags;
> > +             sky1_gate->gate.lock = lock;
> > +             sky1_gate_ops = &sky1_audss_clk_gate_ops;
> > +             sky1_gate->regmap = regmap;
> > +             sky1_gate->offset = gate_cfg->offset;
> > +     }
> > +
> > +     hw = clk_hw_register_composite(dev, name, parent_names,
> num_parents,
> > +                                    sky1_mux ? &sky1_mux->mux.hw : NULL,
> sky1_mux_ops,
> > +                                    sky1_div ? &sky1_div->div.hw : NULL, sky1_div_ops,
> > +                                    sky1_gate ? &sky1_gate->gate.hw : NULL,
> sky1_gate_ops,
> > +                                    flags);
> 
> Please use devm_clk_hw_register_composite_pdata() to make sure that
> everything is cleaned up in the expected order relative to the other
> devm_*() calls.
OK

> Please go through the Sashiko feedback at
> https://sashiko.dev/#/message/20260630125936.E186A1F000E9%40smtp.kerne
> l.org
> and I'll do do a thorough review on the next version.
OK, I will fix the feedback which I think is reasonable.

Thanks,
Joakim