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

[Peng Fan]

> 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 at 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.