Re: [PATCH v5 3/5] clk: hisilicon: Add complex clock for Hi3798

[Stephen Boyd <sboyd@kernel.org>]

Quoting David Yang (2023-03-20 13:40:36)
> @@ -59,6 +61,131 @@ static const struct hisi_fixed_rate_clock hi3798_fixed_rate_clks[] = {
>         { HI3798_FIXED_250M, "250m", NULL, 0, 250000000, },
>  };
>  
> +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 = to_complex_clk(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(clk->reg);
> +       val &= ~(clk->mask);
> +       val |= clk->value;
> +       writel_relaxed(val, clk->reg);
> +
> +       return 0;
> +}
> +
> +static void hi3798_clk_complex_unprepare(struct clk_hw *hw)
> +{
> +       struct hi3798_clk_complex *clk = to_complex_clk(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(clk->reg);
> +       val &= ~(clk->mask);
> +       writel_relaxed(val, clk->reg);
> +}
> +
> +static int hi3798_clk_complex_is_prepared(struct clk_hw *hw)
> +{
> +       struct hi3798_clk_complex *clk = to_complex_clk(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(clk->reg);
> +       return (val & clk->mask) == clk->value;
> +}
> +
> +static const struct clk_ops hi3798_clk_complex_ops = {
> +       .prepare = hi3798_clk_complex_prepare,
> +       .unprepare = hi3798_clk_complex_unprepare,
> +       .is_prepared = 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 = data->base;
> +       int i;
> +       int ret;
> +
> +       for (i = 0; i < nums; i++) {
> +               struct hi3798_clk_complex *p_clk;
> +               struct clk *clk;
> +               struct clk_init_data init;
> +
> +               p_clk = kzalloc(sizeof(*p_clk), GFP_KERNEL);

Use devm?

> +               if (!p_clk) {
> +                       ret = -ENOMEM;
> +                       goto err_kzalloc;
> +               }
> +
> +               init.name = clks[i].name;
> +               init.ops = &hi3798_clk_complex_ops;
> +
> +               init.flags = 0;
> +               init.parent_names =
> +                       (clks[i].parent_name ? &clks[i].parent_name : NULL);
> +               init.num_parents = (clks[i].parent_name ? 1 : 0);
> +
> +               p_clk->reg = base + clks[i].offset;
> +               p_clk->mask = clks[i].mask;
> +               p_clk->value = clks[i].value;
> +               p_clk->hw.init = &init;
> +
> +               clk = 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] = clk;
> +       }
> +
> +       return 0;
> +
> +err:
> +       while (i--)
> +               clk_unregister(data->clk_data.clks[clks[i].id]);
> +
> +       return ret;
> +}
> +