Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

[Stephen Boyd]

Quoting Dmitry Osipenko (2019-04-14 13:20:06)
> diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c
> new file mode 100644
> index 000000000000..35b67a9989c8
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-tegra20-emc.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bits.h>
> +#include <linux/clk/tegra.h>

Include clk-provider.h as this is a clk provider driver.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +#include "clk.h"
> +
> +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK     GENMASK(7, 0)
> +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK         GENMASK(31, 30)
[...]
> +
> +static const struct clk_ops tegra_clk_emc_ops = {
> +       .recalc_rate = emc_recalc_rate,
> +       .get_parent = emc_get_parent,
> +       .set_parent = emc_set_parent,
> +       .set_rate = emc_set_rate,
> +       .set_rate_and_parent = emc_set_rate_and_parent,
> +       .determine_rate = emc_determine_rate,
> +};
> +
> +void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb)

Why can't we have type safety on these function pointers?

> +{
> +       struct clk *clk = __clk_lookup("emc");
> +       struct tegra_clk_emc *emc;
> +       struct clk_hw *hw;
> +
> +       if (clk) {
> +               hw = __clk_get_hw(clk);
> +               emc = to_tegra_clk_emc(hw);
> +
> +               emc->round_cb = round_cb;
> +               emc->arg_cb = arg_cb;
> +       }
> +}
> +
> +bool tegra20_clk_emc_driver_available(void)
> +{
> +       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in
this driver somehow?

> +       struct tegra_clk_emc *emc;
> +       struct clk_hw *hw;
> +
> +       if (clk) {
> +               hw = __clk_get_hw(clk);

This gets further to the point, we don't prefer to see drivers use
__clk_get_hw() unless they absolutely need to. Maybe I don't understand
the driver structure, but it seems like maybe the driver that's
providing the callbacks could be the same driver that's registering
these clks, and thus everything could be inside one file so that we
don't have to pass around callbacks and clk_hw pointers? Commit text
says "this is how it's been" but that's not a reason to keep doing it.

> +               emc = to_tegra_clk_emc(hw);
> +