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

Subject: Re: [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

Fix in V4.

...

• • 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?

As wrote in commit log. The code was imported from Linux, I would not modify this.

...

+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?

Explained above.

...

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

•   if (!mux)
  

•           goto fail;
  

It would be better like this:

Ditto

   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.

Ditto.

Thanks, Peng.