Re: [U-Boot] [PATCH V3 3/4] clk: imx: add i.MX8M composite clk support

On Fri, Aug 9, 2019 at 5:02 AM Peng Fan peng.fan@nxp.com wrote:

• • The clk are not binded to a dev, because it is part of composite clk

s/binded/bound

• • use composite clk to get dev
• */

+static ulong imx8m_clk_composite_divider_set_rate(struct clk *clk,

•                                             unsigned long rate)
  

+{

•   struct clk_divider *divider = (struct clk_divider *)to_clk_divider(clk);
  

•   struct clk_composite *composite = (struct clk_composite *)clk->data;
  

•   ulong parent_rate = clk_get_parent_rate(&composite->clk);
  

•   int prediv_value;
  

•   int div_value;
  

•   int ret;
  

•   u32 val;
  

•   ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
  

•                                              &prediv_value, &div_value);
  

•   if (ret)
  

•           return -EINVAL;
  

What about returning ret directly instead?

+struct clk *imx8m_clk_composite_flags(const char *name,

•                                 const char * const *parent_names,
  

•                                 int num_parents, void __iomem *reg,
  

•                                 unsigned long flags)
  

+{

•   struct clk *clk = ERR_PTR(-ENOMEM);
  

•   struct clk_divider *div = NULL;
  

•   struct clk_gate *gate = NULL;
  

•   struct clk_mux *mux = NULL;
  

Why all these NULL assignments?

•   mux = kzalloc(sizeof(*mux), GFP_KERNEL);
  

•   if (!mux)
  

•           goto fail;
  

It would be better like this:

mux = kzalloc(sizeof(*mux), GFP_KERNEL); if (!mux) return ERR_PTR(-ENOMEM);

•   mux->reg = reg;
  

•   mux->shift = PCG_PCS_SHIFT;
  

•   mux->mask = PCG_PCS_MASK;
  

•   mux->num_parents = num_parents;
  

•   mux->flags = flags;
  

•   mux->parent_names = parent_names;
  

•   div = kzalloc(sizeof(*div), GFP_KERNEL);
  

•   if (!div)
  

•           goto fail;
  

•   div->reg = reg;
  

•   div->shift = PCG_PREDIV_SHIFT;
  

•   div->width = PCG_PREDIV_WIDTH;
  

•   div->flags = CLK_DIVIDER_ROUND_CLOSEST | flags;
  

•   gate = kzalloc(sizeof(*gate), GFP_KERNEL);
  

•   if (!gate)
  

•           goto fail;
  

•   gate->reg = reg;
  

•   gate->bit_idx = PCG_CGC_SHIFT;
  

•   gate->flags = flags;
  

•   clk = clk_register_composite(NULL, name,
  

•                                parent_names, num_parents,
  

•                                &mux->clk, &clk_mux_ops, &div->clk,
  

•                                &imx8m_clk_composite_divider_ops,
  

•                                &gate->clk, &clk_gate_ops, flags);
  

•   if (IS_ERR(clk))
  

•           goto fail;
  

•   return clk;
  

+fail:

•   kfree(gate);
  

•   kfree(div);
  

•   kfree(mux);
  

•   return ERR_CAST(clk);
  

ERR_CAST(clk) is only valid when for the IS_ERR(clk) path.

Please rework the error handling.