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.
