Message-ID: <d3b057408117a71bcd153f4a91bcdfe1.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20230320204042.980708-4-mmyangfl@gmail.com>
References: <20230320204042.980708-1-mmyangfl@gmail.com> <20230320204042.980708-4-mmyangfl@gmail.com>
Subject: Re: [PATCH v5 3/5] clk: hisilicon: Add complex clock for Hi3798
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     David Yang <mmyangfl@gmail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
To:     David Yang <mmyangfl@gmail.com>
Date:   Tue, 21 Mar 2023 10:31:12 -0700
User-Agent: alot/0.10
Precedence: bulk

Quoting David Yang (2023-03-20 13:40:36)
> @@ -59,6 +61,131 @@ static const struct hisi_fixed_rate_clock hi3798_fixe=
d_rate_clks[] =3D {
>         { HI3798_FIXED_250M, "250m", NULL, 0, 250000000, },
>  };
> =20
> +struct hi3798_complex_clock {
> +       unsigned int    id;
> +       const char      *name;
> +       const char      *parent_name;
> +       unsigned long   flags;
> +       unsigned long   offset;
> +       u32             mask;
> +       u32             value;
> +       const char      *alias;
> +};
> +
> +struct hi3798_clk_complex {
> +       struct clk_hw   hw;
> +       void __iomem    *reg;
> +       u32             mask;
> +       u32             value;
> +};
> +
> +#define to_complex_clk(_hw) container_of(_hw, struct hi3798_clk_complex,=
 hw)

Missing header include for container_of()

> +
> +static int hi3798_clk_complex_prepare(struct clk_hw *hw)
> +{
> +       struct hi3798_clk_complex *clk =3D to_complex_clk(hw);
> +       u32 val;
> +
> +       val =3D readl_relaxed(clk->reg);
> +       val &=3D ~(clk->mask);
> +       val |=3D clk->value;
> +       writel_relaxed(val, clk->reg);
> +
> +       return 0;
> +}
> +
> +static void hi3798_clk_complex_unprepare(struct clk_hw *hw)
> +{
> +       struct hi3798_clk_complex *clk =3D to_complex_clk(hw);
> +       u32 val;
> +
> +       val =3D readl_relaxed(clk->reg);
> +       val &=3D ~(clk->mask);
> +       writel_relaxed(val, clk->reg);
> +}
> +
> +static int hi3798_clk_complex_is_prepared(struct clk_hw *hw)
> +{
> +       struct hi3798_clk_complex *clk =3D to_complex_clk(hw);
> +       u32 val;
> +
> +       val =3D readl_relaxed(clk->reg);
> +       return (val & clk->mask) =3D=3D clk->value;
> +}
> +
> +static const struct clk_ops hi3798_clk_complex_ops =3D {
> +       .prepare =3D hi3798_clk_complex_prepare,
> +       .unprepare =3D hi3798_clk_complex_unprepare,
> +       .is_prepared =3D hi3798_clk_complex_is_prepared,
> +};
> +
> +static int hi3798_clk_register_complex(const struct hi3798_complex_clock=
 *clks, int nums,
> +                                      struct hisi_clock_data *data)
> +{
> +       void __iomem *base =3D data->base;
> +       int i;
> +       int ret;
> +
> +       for (i =3D 0; i < nums; i++) {
> +               struct hi3798_clk_complex *p_clk;
> +               struct clk *clk;
> +               struct clk_init_data init;
> +
> +               p_clk =3D kzalloc(sizeof(*p_clk), GFP_KERNEL);

Use devm?

> +               if (!p_clk) {
> +                       ret =3D -ENOMEM;
> +                       goto err_kzalloc;
> +               }
> +
> +               init.name =3D clks[i].name;
> +               init.ops =3D &hi3798_clk_complex_ops;
> +
> +               init.flags =3D 0;
> +               init.parent_names =3D
> +                       (clks[i].parent_name ? &clks[i].parent_name : NUL=
L);
> +               init.num_parents =3D (clks[i].parent_name ? 1 : 0);
> +
> +               p_clk->reg =3D base + clks[i].offset;
> +               p_clk->mask =3D clks[i].mask;
> +               p_clk->value =3D clks[i].value;
> +               p_clk->hw.init =3D &init;
> +
> +               clk =3D clk_register(NULL, &p_clk->hw);

Use devm? Also, please use devm_clk_hw_register()

> +               if (IS_ERR(clk)) {
> +                       kfree(p_clk);
> +err_kzalloc:
> +                       pr_err("%s: failed to register clock %s\n",
> +                              __func__, clks[i].name);
> +                       goto err;
> +               }
> +
> +               if (clks[i].alias)
> +                       clk_register_clkdev(clk, clks[i].alias, NULL);

Do you use this clkdev lookup? You have an OF clk provider. Hopefully
this can be removed.

> +
> +               data->clk_data.clks[clks[i].id] =3D clk;
> +       }
> +
> +       return 0;
> +
> +err:
> +       while (i--)
> +               clk_unregister(data->clk_data.clks[clks[i].id]);
> +
> +       return ret;
> +}
> +