Re: [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting Biju Das (2023-02-20 05:13:06)
> diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
> new file mode 100644
> index 000000000000..561cfad8a243
> --- /dev/null
> +++ b/drivers/clk/clk-versaclock3.c
> @@ -0,0 +1,1134 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Renesas Versaclock 3
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +#include <linux/clk.h>

Please remove this include after moving to 'struct clk_parent_data'.

> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define NUM_CONFIG_REGISTERS           37
> +
[...]
> +
> +static void vc3_clk_flags_parse_dt(struct device *dev, u32 *crt_clks)
> +{
> +       struct device_node *np = dev->of_node;
> +       struct property *prop;
> +       const __be32 *p;
> +       u32 i = 0;
> +       u32 val;
> +
> +       of_property_for_each_u32(np, "renesas,clock-flags", prop, p, val) {
> +               if (i >= ARRAY_SIZE(clk_out_data))
> +                       break;
> +               *crt_clks++ = val;
> +               i++;
> +       }
> +}
> +
> +static void vc3_fill_init_data(struct clk_init_data *init,
> +                              const struct vc3_clk_data *mux,
> +                              const struct clk_ops *ops,
> +                              u32 flags, int n,
> +                              const char **pll_parent_names,
> +                              const char **clkin_name)
> +{
> +       unsigned int i;
> +
> +       init->name = mux->name;
> +       init->ops = ops;
> +       init->flags = CLK_SET_RATE_PARENT;
> +       init->num_parents = n;
> +       for (i = 0; i < n; i++) {
> +               if (!mux->parent_names[i])
> +                       pll_parent_names[i] = clkin_name[0];
> +               else
> +                       pll_parent_names[i] = mux->parent_names[i];

Instead of string names please use clk_hw pointers or 'struct
clk_parent_data'.

> +       }
> +
> +       init->parent_names = pll_parent_names;
> +}
> +
> +static int vc3_clk_register(struct device *dev, struct vc3_driver_data *vc3,
> +                           struct vc3_hw_data *data, const void *clk_data,
> +                           struct clk_init_data *init, int n)

const init pointer?

> +{
> +       data->hw.init = init;
> +       data->num = n;
> +       data->vc3 = vc3;
> +       data->data = clk_data;
> +
> +       return devm_clk_hw_register(dev, &data->hw);
> +}
> +
> +static int vc3_probe(struct i2c_client *client)
> +{
> +       struct device *dev = &client->dev;
> +       u8 settings[NUM_CONFIG_REGISTERS];
> +       const char *pll_parent_names[3];
> +       struct vc3_driver_data *vc3;
> +       const char *clkin_name[1];
> +       struct clk_init_data init;
> +       u32 crit_clks[6] = {};
> +       struct clk *clk;
> +       int ret, i;
> +
> +       vc3 = devm_kzalloc(dev, sizeof(*vc3), GFP_KERNEL);
> +       if (!vc3)
> +               return -ENOMEM;
> +
> +       i2c_set_clientdata(client, vc3);
> +       vc3->client = client;
> +
> +       vc3->regmap = devm_regmap_init_i2c(client, &vc3_regmap_config);
> +       if (IS_ERR(vc3->regmap))
> +               return dev_err_probe(dev, PTR_ERR(vc3->regmap),
> +                                    "failed to allocate register map\n");
> +
> +       ret = of_property_read_u8_array(dev->of_node, "renesas,settings",
> +                                       settings, ARRAY_SIZE(settings));
> +       if (!ret) {
> +               /*
> +                * A raw settings array was specified in the DT. Write the
> +                * settings to the device immediately.
> +                */
> +               for  (i = 0; i < NUM_CONFIG_REGISTERS; i++) {
> +                       ret = regmap_write(vc3->regmap, i, settings[i]);
> +                       if (ret) {
> +                               dev_err(dev, "error writing to chip (%i)\n", ret);
> +                               return ret;
> +                       }
> +               }
> +       } else if (ret == -EOVERFLOW) {
> +               dev_err(&client->dev, "EOVERFLOW reg settings. ARRAY_SIZE: %zu",
> +                       ARRAY_SIZE(settings));
> +               return ret;
> +       }
> +
> +       /* Register clock ref */
> +       memset(&init, 0, sizeof(init));
> +
> +       clk = devm_clk_get(dev, "x1");
> +       if (PTR_ERR(clk) == -EPROBE_DEFER)
> +               return -EPROBE_DEFER;
> +
> +       if (!IS_ERR(clk)) {
> +               clkin_name[0] = __clk_get_name(clk);
> +       } else {
> +               clk = devm_clk_get(dev, "clkin");
> +               if (PTR_ERR(clk) == -EPROBE_DEFER)
> +                       return -EPROBE_DEFER;
> +
> +               if (!IS_ERR(clk))
> +                       clkin_name[0] = __clk_get_name(clk);
> +       }

Please use 'struct clk_parent_data' instead of clk consumer APIs.

> +
> +       if (IS_ERR_OR_NULL(clk))
> +               return dev_err_probe(&client->dev, -EINVAL, "no input clk\n");
> +
> +       /* Register pfd muxes */
> +       for (i = 0; i < ARRAY_SIZE(pfd_mux_data); i++) {
> +               vc3_fill_init_data(&init, &pfd_mux_data[i], &vc3_pfd_mux_ops,
> +                                  CLK_SET_RATE_PARENT, 2, pll_parent_names,
> +                                  clkin_name);
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd_mux[i],
> +                                      &pfd_mux_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       /* Register pfd's */
> +       for (i = 0; i < ARRAY_SIZE(pfd_data); i++) {
> +               if (i == VC3_PFD1)
> +                       pll_parent_names[0] = clkin_name[0];
> +               else
> +                       pll_parent_names[0] = pfd_mux_data[i - 1].name;
> +
> +               init.name = pfd_data[i].name;
> +               init.ops = &vc3_pfd_ops;
> +               init.flags = CLK_SET_RATE_PARENT;
> +               init.num_parents = 1;
> +               init.parent_names = pll_parent_names;
> +
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd[i],
> +                                      &pfd_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       /* Register pll's */
> +       for (i = 0; i < ARRAY_SIZE(pll_data); i++) {
> +               pll_parent_names[0] = pfd_data[i].name;
> +               init.name = pll_data[i].name;
> +               init.ops = &vc3_pll_ops;
> +               init.flags = CLK_SET_RATE_PARENT;
> +               init.num_parents = 1;
> +               init.parent_names = pll_parent_names;
> +
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_pll[i],
> +                                      &pll_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       /* Register divider muxes */
> +       for (i = 0; i < ARRAY_SIZE(div_mux_data); i++) {
> +               vc3_fill_init_data(&init, &div_mux_data[i], &vc3_div_mux_ops,
> +                                  CLK_SET_RATE_PARENT, 2, pll_parent_names,
> +                                  clkin_name);
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_div_mux[i],
> +                                      &div_mux_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       vc3_divider_type_parse_dt(dev, vc3);
> +       /* Register dividers */
> +       for (i = 0; i < ARRAY_SIZE(div_data); i++) {
> +               switch (i) {
> +               case VC3_DIV1:
> +                       pll_parent_names[0] = div_mux_data[VC3_DIV1_MUX].name;
> +                       break;
> +               case VC3_DIV2:
> +                       pll_parent_names[0] = pll_data[VC3_PLL1].name;
> +                       break;
> +               case VC3_DIV3:
> +                       pll_parent_names[0] = div_mux_data[VC3_DIV3_MUX].name;
> +                       break;
> +               case VC3_DIV4:
> +                       pll_parent_names[0] = div_mux_data[VC3_DIV4_MUX].name;
> +                       break;
> +               case VC3_DIV5:
> +                       pll_parent_names[0] = pll_data[VC3_PLL3].name;
> +                       break;
> +               }
> +
> +               init.name = div_data[i].name;
> +               init.ops = &vc3_div_ops;
> +               init.flags = CLK_SET_RATE_PARENT;
> +               init.num_parents = 1;
> +               init.parent_names = pll_parent_names;
> +
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_div[i],
> +                                      &div_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       /* Register clk muxes */
> +       for (i = 0; i < ARRAY_SIZE(clk_mux_data); i++) {
> +               vc3_fill_init_data(&init, &clk_mux_data[i], &vc3_clk_mux_ops,
> +                                  CLK_SET_RATE_PARENT, 2, pll_parent_names,
> +                                  clkin_name);
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_mux[i],
> +                                      &clk_mux_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       /* Register clk outputs */
> +       vc3_clk_flags_parse_dt(dev, crit_clks);
> +       for (i = 0; i < ARRAY_SIZE(clk_out_data); i++) {
> +               vc3_fill_init_data(&init, &clk_out_data[i], &vc3_clk_out_ops,
> +                                  crit_clks[i], 1, pll_parent_names,
> +                                  clkin_name);
> +               ret = vc3_clk_register(dev, vc3, &vc3->clk_out[i],
> +                                      &clk_out_data[i], &init, i);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "%s failed\n", init.name);
> +       }
> +
> +       ret = of_clk_add_hw_provider(client->dev.of_node, vc3_of_clk_get, vc3);

Can you use devm?

> +       if (ret)
> +               return dev_err_probe(dev, ret, "unable to add clk provider\n");
> +
> +       return ret;
> +}
> +
> +static void vc3_remove(struct i2c_client *client)
> +{
> +       of_clk_del_provider(client->dev.of_node);
> +}

Using devm means this whole function can be removed.