On 08/26, Paul Burton wrote:
>
> drivers/clk/Kconfig | 9 ++++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-boston.c | 131
> +++++++++++++++++++++++++++++++++++++++++++++++
Maybe a vendor subdirectory is appropriate? imgtec?
> +
> +struct clk_boston_state {
> + struct clk *clk[BOSTON_CLK_COUNT];
> + struct clk_boston clk_boston[BOSTON_CLK_COUNT];
> + struct clk_onecell_data onecell_data[BOSTON_CLK_COUNT];
> +};
> +
> +static const char *clk_names[BOSTON_CLK_COUNT] = {
const char * const?
> + [BOSTON_CLK_SYS] = "sys",
> + [BOSTON_CLK_CPU] = "cpu",
> +};
> +
> +#define BOSTON_PLAT_MMCMDIV 0x30
> +# define BOSTON_PLAT_MMCMDIV_CLK0DIV (0xff << 0)
> +# define BOSTON_PLAT_MMCMDIV_INPUT (0xff << 8)
> +# define BOSTON_PLAT_MMCMDIV_MUL (0xff << 16)
> +# define BOSTON_PLAT_MMCMDIV_CLK1DIV (0xff << 24)
> +
> +static struct clk_boston *to_clk_boston(struct clk_hw *hw)
> +{
> + return container_of(hw, struct clk_boston, hw);
> +}
> +
> +static uint32_t ext_field(uint32_t val, uint32_t mask)
Please use u32 instead of uint32_t in drivers.
> +{
> + return (val & mask) >> (ffs(mask) - 1);
> +}
> +
> +static unsigned long clk_boston_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_boston *state = to_clk_boston(hw);
> + uint32_t in_rate, mul, div;
> + uint mmcmdiv;
unsigned int?
> + int err;
> +
> + err = regmap_read(state->regmap, BOSTON_PLAT_MMCMDIV, &mmcmdiv);
> + if (err)
> + return 0;
> +
> + in_rate = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_INPUT);
This sounds like a parent rate? Should there be another clk
created for that so that parent_rate in this function is useful?
> + mul = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_MUL);
> +
> + switch (state->id) {
> + case BOSTON_CLK_SYS:
> + div = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_CLK0DIV);
> + break;
> + case BOSTON_CLK_CPU:
> + div = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_CLK1DIV);
Why not put the CLK0DIV or CLK1DIV offset in state->id instead?
That way this function just read in_rate, mul, and div and then
does the math?
> + break;
> + default:
> + return 0;
> + }
> +
> + return (in_rate * mul * 1000000) / div;
Is this always fixed at boot? It may be easier to populate fixed
rate clks during probe with the rate calculated there. Then there
aren't any clk_ops to implement.
> +}
> +
> +static const struct clk_ops clk_boston_ops = {
> + .recalc_rate = clk_boston_recalc_rate,
> +};
> +
> +static void __init clk_boston_setup(struct device_node *np)
> +{
> + struct clk_boston_state *state;
> + struct clk_init_data init;
> + struct regmap *regmap;
> + int i, err;
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return;
> +
> + regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
> + if (IS_ERR(regmap)) {
> + pr_err("failed to find regmap\n");
> + return;
> + }
> +
> + for (i = 0; i < BOSTON_CLK_COUNT; i++) {
> + memset(&init, 0, sizeof(init));
> + init.flags = CLK_IS_BASIC;
Please drop this flag unless you really need it for something. As
far as I know CLK_IS_BASIC is just for OMAP code.
> + init.name = clk_names[i];
> + init.ops = &clk_boston_ops;
> +
> + state->clk_boston[i].hw.init = &init;
> + state->clk_boston[i].id = i;
> + state->clk_boston[i].regmap = regmap;
> +
> + state->clk[i] = clk_register(NULL, &state->clk_boston[i].hw);
Please use clk_hw_register() instead.
> + if (IS_ERR(state->clk[i])) {
> + pr_err("failed to register clock: %ld\n",
> + PTR_ERR(state->clk[i]));
> + return;
> + }
> + }
> +
> + state->onecell_data->clks = state->clk;
> + state->onecell_data->clk_num = BOSTON_CLK_COUNT;
> +
> + err = of_clk_add_provider(np, of_clk_src_onecell_get,
> + state->onecell_data);
Please use of_clk_add_hw_provider() instead.
> + if (err)
> + pr_err("failed to add DT provider: %d\n", err);
> +}
> +CLK_OF_DECLARE(clk_boston, "img,boston-clock", clk_boston_setup);
Please make this into a platform driver.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
