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

[Fabio Estevam]

On Fri, Aug 9, 2019 at 5:02 AM Peng Fan <peng.fan at 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.