Quoting Michael Tretter (2020-11-15 23:55:28) > diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c > index 725e646aa726..cedc8b7859f7 100644 > --- a/drivers/soc/xilinx/xlnx_vcu.c > +++ b/drivers/soc/xilinx/xlnx_vcu.c > @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu) > return xvcu_pll_enable(xvcu); > } > > +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev, > + const char *name, > + const char * const *parent_names, > + u8 num_parents, > + struct clk_hw *parent_default, > + void __iomem *reg, > + spinlock_t *lock) > +{ > + unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT; > + u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO | Why u8? > + CLK_DIVIDER_ROUND_CLOSEST; > + struct clk_hw *mux = NULL; > + struct clk_hw *divider = NULL; > + struct clk_hw *gate = NULL; > + char *name_mux; > + char *name_div; > + int err; > + > + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux"); > + if (!name_mux) { > + err = -ENOMEM; > + goto err; > + } > + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents, > + flags, reg, 0, 1, 0, lock); > + if (IS_ERR(mux)) { > + err = PTR_ERR(divider); > + goto err; > + } > + clk_hw_set_parent(mux, parent_default); Can this be done from assigned-clock-parents binding? > + > + name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div"); > + if (!name_div) { > + err = -ENOMEM; > + goto err; > + } > + divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags, > + reg, 4, 6, divider_flags, > + lock); > + if (IS_ERR(divider)) { > + err = PTR_ERR(divider); > + goto err; > + } > + > + gate = clk_hw_register_gate_parent_hw(dev, name, divider, > + CLK_SET_RATE_PARENT, reg, 12, 0, > + lock); > + if (IS_ERR(gate)) { > + err = PTR_ERR(gate); > + goto err; > + } > + > + return gate; > + > +err: > + if (!IS_ERR_OR_NULL(gate)) Would be nicer to have some more goto labels and skip the IS_ERR_OR_NULL checks. > + clk_hw_unregister_gate(gate); > + if (!IS_ERR_OR_NULL(divider)) > + clk_hw_unregister_divider(divider); > + if (!IS_ERR_OR_NULL(mux)) > + clk_hw_unregister_divider(mux); > + > + return ERR_PTR(err); > +} > + > +static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw) > +{ > + struct clk_hw *gate = hw; > + struct clk_hw *divider; > + struct clk_hw *mux; > + > + if (!gate) > + return; > + > + divider = clk_hw_get_parent(gate); > + clk_hw_unregister_gate(gate); > + if (!divider) > + return; > + > + mux = clk_hw_get_parent(divider); > + clk_hw_unregister_mux(mux); > + if (!divider) > + return; > + > + clk_hw_unregister_divider(divider); > +} > + > +static DEFINE_SPINLOCK(venc_core_lock); > +static DEFINE_SPINLOCK(venc_mcu_lock); Any reason to not allocate these spinlocks too? > + > +static int xvcu_register_clock_provider(struct xvcu_device *xvcu) > +{ > + struct device *dev = xvcu->dev; > + const char *parent_names[2]; > + struct clk_hw *parent_default; > + struct clk_hw_onecell_data *data; > + struct clk_hw **hws; > + void __iomem *reg_base = xvcu->vcu_slcr_ba; > + > + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + data->num = CLK_XVCU_NUM_CLOCKS; > + hws = data->hws; > + > + xvcu->clk_data = data; > + > + parent_default = xvcu->pll; > + parent_names[0] = "dummy"; What is "dummy"? > + parent_names[1] = clk_hw_get_name(parent_default); Can we use the new way of specifying clk parents from DT or by using direct pointers instead of using string names? The idea is to get rid of clk_hw_get_name() eventually. > + > + hws[CLK_XVCU_ENC_CORE] = > + xvcu_clk_hw_register_leaf(dev, "venc_core_clk", > + parent_names, > + ARRAY_SIZE(parent_names), > + parent_default, > + reg_base + VCU_ENC_CORE_CTRL, > + &venc_core_lock); > + hws[CLK_XVCU_ENC_MCU] = > + xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk", > + parent_names, > + ARRAY_SIZE(parent_names), > + parent_default, > + reg_base + VCU_ENC_MCU_CTRL, > + &venc_mcu_lock); > +