Hi Stephen Boyd,
Thanks for the feedback.
> -----Original Message-----
> From: Stephen Boyd <sboyd@xxxxxxxxxx>
> Sent: Monday, March 6, 2023 8:22 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Michael Turquette
> <mturquette@xxxxxxxxxxxx>
> Cc: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx; Geert
> Uytterhoeven <geert+renesas@xxxxxxxxx>; Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock
> driver
>
> 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'.
Agreed.
>
> > +#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'.
OK.
>
> > + }
> > +
> > + 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?
OK.
>
> > +{
> > + 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.
OK.
>
> > +
> > + 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?
OK.
>
> > + 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.
OK.
Will address this in next version.
Cheers,
Biju
