Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
To:     Abel Vesa <abel.vesa@nxp.com>,
        Andrey Smirnov <andrew.smirnov@gmail.com>,
        Anson Huang <anson.huang@nxp.com>,
        Dong Aisheng <aisheng.dong@nxp.com>,
        Fabio Estevam <fabio.estevam@nxp.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        Rob Herring <robh@kernel.org>,
        Sascha Hauer <kernel@pengutronix.de>
From:   Stephen Boyd <sboyd@kernel.org>
In-Reply-To: <1537785597-26499-5-git-send-email-abel.vesa@nxp.com>
Cc:     linux-imx@nxp.com, Abel Vesa <abelvesa@linux.com>,
        Abel Vesa <abel.vesa@nxp.com>, Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Michael Turquette <mturquette@baylibre.com>,
        open list <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>
References: <1537785597-26499-1-git-send-email-abel.vesa@nxp.com>
 <1537785597-26499-5-git-send-email-abel.vesa@nxp.com>
Message-ID: <153980589525.5275.9065338436630625817@swboyd.mtv.corp.google.com>
User-Agent: alot/0.7
Subject: Re: [PATCH v9 4/5] clk: imx: add imx composite clock
Date:   Wed, 17 Oct 2018 12:51:35 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Abel Vesa (2018-09-24 03:39:56)
> diff --git a/drivers/clk/imx/clk-composite.c b/drivers/clk/imx/clk-compos=
ite.c
> new file mode 100644
> index 0000000..4b03107
> --- /dev/null
> +++ b/drivers/clk/imx/clk-composite.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018 NXP
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>

Is this include used?

> +
> +#include "clk.h"
> +
> +#define PCG_PREDIV_SHIFT       16
> +#define PCG_PREDIV_WIDTH       3
> +#define PCG_PREDIV_MAX         8
> +
> +#define PCG_DIV_SHIFT          0
> +#define PCG_DIV_WIDTH          6
> +#define PCG_DIV_MAX            64
> +
> +#define PCG_PCS_SHIFT          24
> +#define PCG_PCS_MASK           0x7
> +
> +#define PCG_CGC_SHIFT          28
> +
> +static unsigned long imx_clk_composite_divider_recalc_rate(struct clk_hw=
 *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct clk_divider *divider =3D to_clk_divider(hw);
> +       unsigned long prediv_rate;
> +       unsigned int prediv_value;
> +       unsigned int div_value;
> +
> +       prediv_value =3D clk_readl(divider->reg) >> divider->shift;
> +       prediv_value &=3D clk_div_mask(divider->width);
> +
> +       prediv_rate =3D divider_recalc_rate(hw, parent_rate, prediv_value,
> +                                               NULL, divider->flags,
> +                                               divider->width);
> +
> +       div_value =3D clk_readl(divider->reg) >> PCG_DIV_SHIFT;
> +       div_value &=3D clk_div_mask(PCG_DIV_WIDTH);
> +
> +       return divider_recalc_rate(hw, prediv_rate, div_value, NULL,
> +                                  divider->flags, PCG_DIV_WIDTH);
> +}
> +
> +static int imx_clk_composite_compute_dividers(unsigned long rate,
> +                                               unsigned long parent_rate,
> +                                               int *prediv, int *postdiv)
> +{
> +       int div1, div2;
> +       int error =3D INT_MAX;
> +       int ret =3D -EINVAL;
> +
> +       /* default values */
> +       *prediv =3D 1;
> +       *postdiv =3D 1;
> +
> +       for (div1 =3D 1; div1 <=3D PCG_PREDIV_MAX; div1++) {
> +               for (div2 =3D 1; div2 <=3D PCG_DIV_MAX; div2++) {
> +                       int new_error =3D ((parent_rate / div1) / div2) -=
 rate;
> +
> +                       if (abs(new_error) < abs(error)) {
> +                               *prediv =3D div1;
> +                               *postdiv =3D div2;
> +                               error =3D new_error;
> +                               ret =3D 0;
> +                       }
> +               }
> +       }
> +       return ret;
> +}
> +
> +static long imx_clk_composite_divider_round_rate(struct clk_hw *hw,
> +                                               unsigned long rate,
> +                                               unsigned long *prate)
> +{
> +       int prediv_value;
> +       int div_value;
> +
> +       imx_clk_composite_compute_dividers(rate, *prate,
> +                                               &prediv_value, &div_value=
);
> +
> +       rate =3D DIV_ROUND_UP_ULL((u64)*prate, prediv_value);
> +       rate =3D DIV_ROUND_UP_ULL((u64)rate, div_value);
> +
> +       return rate;

Looks the same as another patch, maybe it is?

Anyway, same nitpick about returning the DIV_ROUND_UP_ULL() result.

> +}
> +
> +static int imx_clk_composite_divider_set_rate(struct clk_hw *hw,
> +                                       unsigned long rate,
> +                                       unsigned long parent_rate)
> +{
> +       struct clk_divider *divider =3D to_clk_divider(hw);
> +       unsigned long flags =3D 0;
> +       int prediv_value;
> +       int div_value;
> +       int ret =3D 0;
> +       u32 val;
> +
> +       ret =3D imx_clk_composite_compute_dividers(rate, parent_rate,
> +                                               &prediv_value, &div_value=
);
> +       if (ret)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(divider->lock, flags);
> +
> +       val =3D clk_readl(divider->reg);
> +       val &=3D ~((clk_div_mask(divider->width) << divider->shift) |
> +                       (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT));
> +
> +       val |=3D (u32)(prediv_value  - 1) << divider->shift;
> +       val |=3D (u32)(div_value - 1) << PCG_DIV_SHIFT;
> +       clk_writel(val, divider->reg);

Please don't use clk_writel().

> +
> +       spin_unlock_irqrestore(divider->lock, flags);
> +
> +       return ret;
> +}
> +
> +static const struct clk_ops imx_clk_composite_divider_ops =3D {
> +       .recalc_rate =3D imx_clk_composite_divider_recalc_rate,
> +       .round_rate =3D imx_clk_composite_divider_round_rate,
> +       .set_rate =3D imx_clk_composite_divider_set_rate,
> +};
> +
> +struct clk *imx_clk_composite_flags(const char *name,
> +                                       const char **parent_names,
> +                                       int num_parents, void __iomem *re=
g,
> +                                       unsigned long flags)
> +{
> +       struct clk_hw *mux_hw =3D NULL, *div_hw =3D NULL, *gate_hw =3D NU=
LL;
> +       struct clk_divider *div =3D NULL;
> +       struct clk_gate *gate =3D NULL;
> +       struct clk_mux *mux =3D NULL;
> +       struct clk *clk =3D ERR_PTR(-ENOMEM);
> +
> +       mux =3D kzalloc(sizeof(*mux), GFP_KERNEL);
> +       if (!mux)
> +               goto fail;
> +
> +       mux_hw =3D &mux->hw;
> +       mux->reg =3D reg;
> +       mux->shift =3D PCG_PCS_SHIFT;
> +       mux->mask =3D PCG_PCS_MASK;
> +
> +       div =3D kzalloc(sizeof(*div), GFP_KERNEL);
> +       if (!div)
> +               goto fail;
> +
> +       div_hw =3D &div->hw;
> +       div->reg =3D reg;
> +       div->shift =3D PCG_PREDIV_SHIFT;
> +       div->width =3D PCG_PREDIV_WIDTH;
> +       div->lock =3D &imx_ccm_lock;
> +       div->flags =3D CLK_DIVIDER_ROUND_CLOSEST;
> +
> +       gate =3D kzalloc(sizeof(*gate), GFP_KERNEL);
> +       if (!gate)
> +               goto fail;
> +
> +       gate_hw =3D &gate->hw;
> +       gate->reg =3D reg;
> +       gate->bit_idx =3D PCG_CGC_SHIFT;
> +
> +       clk =3D clk_register_composite(NULL, name, parent_names, num_pare=
nts,
> +                                       mux_hw, &clk_mux_ops, div_hw,
> +                                       &imx_clk_composite_divider_ops, g=
ate_hw,
> +                                       &clk_gate_ops, flags);

Didn't I already review this? I'd prefer we move this to using clk_hw
based APIs and then return the clk pointer if needed.

> +       if (IS_ERR(clk))
> +               goto fail;
> +
> +       return clk;
> +
> +fail:
> +       kfree(gate);
> +       kfree(div);
> +       kfree(mux);
> +       return clk;
> +}