Re: [PATCH v2 08/15] clock: milbeaut: Add Milbeaut M10V clock controller

[Stephen Boyd <sboyd@kernel.org>]

Quoting Sugaya Taichi (2019-02-08 04:27:17)
> diff --git a/drivers/clk/clk-milbeaut.c b/drivers/clk/clk-milbeaut.c
> new file mode 100644
> index 0000000..f798939
> --- /dev/null
> +++ b/drivers/clk/clk-milbeaut.c
> @@ -0,0 +1,626 @@
[....]
> +struct m10v_clk_div_fixed_data {
> +       const char      *name;
> +       const char      *parent_name;
> +       u8              div;
> +       u8              mult;
> +       int             onecell_idx;
> +};
> +struct m10v_clk_mux_factors {
> +       const char              *name;
> +       const char * const      *parent_names;
> +       u8                      num_parents;
> +       u32                     offset;
> +       u8                      shift;
> +       u8                      mask;
> +       u32                     *table;
> +       unsigned long           mux_flags;
> +       int                     onecell_idx;
> +};

Please add newlines between struct definitions. It also wouldn't hurt to
have kernel-doc on these.

> +
> +static const struct clk_div_table emmcclk_table[] = {
> +       { .val = 0, .div = 8 },
> +       { .val = 1, .div = 9 },
> +       { .val = 2, .div = 10 },
> +       { .val = 3, .div = 15 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table mclk400_table[] = {
> +       { .val = 1, .div = 2 },
> +       { .val = 3, .div = 4 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table mclk200_table[] = {
> +       { .val = 3, .div = 4 },
> +       { .val = 7, .div = 8 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table aclk400_table[] = {
> +       { .val = 1, .div = 2 },
> +       { .val = 3, .div = 4 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table aclk300_table[] = {
> +       { .val = 0, .div = 2 },
> +       { .val = 1, .div = 3 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table aclk_table[] = {
> +       { .val = 3, .div = 4 },
> +       { .val = 7, .div = 8 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table aclkexs_table[] = {
> +       { .val = 3, .div = 4 },
> +       { .val = 4, .div = 5 },
> +       { .val = 5, .div = 6 },
> +       { .val = 7, .div = 8 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table hclk_table[] = {
> +       { .val = 7, .div = 8 },
> +       { .val = 15, .div = 16 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table hclkbmh_table[] = {
> +       { .val = 3, .div = 4 },
> +       { .val = 7, .div = 8 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table pclk_table[] = {
> +       { .val = 15, .div = 16 },
> +       { .val = 31, .div = 32 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table rclk_table[] = {
> +       { .val = 0, .div = 8 },
> +       { .val = 1, .div = 16 },
> +       { .val = 2, .div = 24 },
> +       { .val = 3, .div = 32 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table uhs1clk0_table[] = {
> +       { .val = 0, .div = 2 },
> +       { .val = 1, .div = 3 },
> +       { .val = 2, .div = 4 },
> +       { .val = 3, .div = 8 },
> +       { .val = 4, .div = 16 },
> +       { .div = 0 },
> +};
> +static const struct clk_div_table uhs2clk_table[] = {
> +       { .val = 0, .div = 9 },
> +       { .val = 1, .div = 10 },
> +       { .val = 2, .div = 11 },
> +       { .val = 3, .div = 12 },
> +       { .val = 4, .div = 13 },
> +       { .val = 5, .div = 14 },
> +       { .val = 6, .div = 16 },
> +       { .val = 7, .div = 18 },
> +       { .div = 0 },
> +};

Same comment applies here. Newlines between tables please.

> +
> +static u32 spi_mux_table[] = {0, 1, 2};
> +static const char * const spi_mux_names[] = {
> +       M10V_SPI_PARENT0, M10V_SPI_PARENT1, M10V_SPI_PARENT2
> +};
> +
> +static u32 uhs1clk2_mux_table[] = {2, 3, 4, 8};
> +static const char * const uhs1clk2_mux_names[] = {
> +       M10V_UHS1CLK2_PARENT0, M10V_UHS1CLK2_PARENT1,
> +       M10V_UHS1CLK2_PARENT2, M10V_PLL6DIV2
> +};
> +
> +static u32 uhs1clk1_mux_table[] = {3, 4, 8};
> +static const char * const uhs1clk1_mux_names[] = {
> +       M10V_UHS1CLK1_PARENT0, M10V_UHS1CLK1_PARENT1, M10V_PLL6DIV2
> +};
> +
[...]
> +
> +static const struct m10v_clk_mux_factors m10v_mux_factor_data[] = {
> +       {"spi", spi_mux_names, ARRAY_SIZE(spi_mux_names),
> +               CLKSEL(8), 3, 7, spi_mux_table, 0, M10V_SPICLK_ID},
> +       {"uhs1clk2", uhs1clk2_mux_names, ARRAY_SIZE(uhs1clk2_mux_names),
> +               CLKSEL(1), 13, 31, uhs1clk2_mux_table, 0, -1},
> +       {"uhs1clk1", uhs1clk1_mux_names, ARRAY_SIZE(uhs1clk1_mux_names),
> +               CLKSEL(1), 8, 31, uhs1clk1_mux_table, 0, -1},
> +       {"nfclk", nfclk_mux_names, ARRAY_SIZE(nfclk_mux_names),
> +               CLKSEL(1), 22, 127, nfclk_mux_table, 0, M10V_NFCLK_ID},
> +};
> +
> +static u8 m10v_mux_get_parent(struct clk_hw *hw)
> +{
> +       struct clk_mux *mux = to_clk_mux(hw);
> +       u32 val;
> +
> +       val = clk_readl(mux->reg) >> mux->shift;

Please don't use clk_readl() unless you absolutely need it.

> +       val &= mux->mask;
> +
> +       return clk_mux_val_to_index(hw, mux->table, mux->flags, val);
> +}
> +
[...]
> +static struct clk_hw *m10v_clk_hw_register_divider(struct device *dev,
> +               const char *name, const char *parent_name, unsigned long flags,
> +               void __iomem *reg, u8 shift, u8 width,
> +               u8 clk_divider_flags, const struct clk_div_table *table,
> +               spinlock_t *lock, void __iomem *write_valid_reg)
> +{
> +       struct m10v_clk_divider *div;
> +       struct clk_hw *hw;
> +       struct clk_init_data init;
> +       int ret;
> +
> +       div = kzalloc(sizeof(*div), GFP_KERNEL);
> +       if (!div)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)

Is this used?

> +               init.ops = &m10v_clk_divider_ro_ops;
> +       else
> +               init.ops = &m10v_clk_divider_ops;
> +       init.flags = flags;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       div->reg = reg;
> +       div->shift = shift;
> +       div->width = width;
> +       div->flags = clk_divider_flags;
> +       div->lock = lock;
> +       div->hw.init = &init;
> +       div->table = table;
> +       div->write_valid_reg = write_valid_reg;
> +
> +       /* register the clock */
> +       hw = &div->hw;
> +       ret = clk_hw_register(dev, hw);
> +       if (ret) {
> +               kfree(div);
> +               hw = ERR_PTR(ret);
> +       }
> +
> +       return hw;
> +}
> +
> +static int m10v_clk_probe(struct platform_device *pdev)
> +{
[...]
> +       for (id = 0; id < ARRAY_SIZE(m10v_div_fixed_data); ++id) {
> +               const struct m10v_clk_div_fixed_data *dfd =
> +                               &m10v_div_fixed_data[id];
> +               const char *pn = dfd->parent_name ?
> +                               dfd->parent_name : parent_name;
> +               hw = clk_hw_register_fixed_factor(NULL, dfd->name,
> +                                       pn, 0, dfd->mult, dfd->div);
> +               if (dfd->onecell_idx >= 0)
> +                       m10v_clk_data->hws[dfd->onecell_idx] = hw;
> +       }
> +       for (id = 0; id < ARRAY_SIZE(m10v_mux_factor_data); ++id) {
> +               const struct m10v_clk_mux_factors *mfd =
> +                               &m10v_mux_factor_data[id];
> +               hw = m10v_clk_hw_register_mux(NULL, mfd->name,
> +                                       mfd->parent_names, mfd->num_parents,
> +                                       CLK_SET_RATE_PARENT,
> +                                       base + mfd->offset, mfd->shift,
> +                                       mfd->mask, mfd->mux_flags, mfd->table,
> +                                       &m10v_crglock);
> +               if (mfd->onecell_idx >= 0)
> +                       m10v_clk_data->hws[mfd->onecell_idx] = hw;
> +       }

Similar style nitpick here. Add newlines between for loops. It may also
make sense to make functions for each of those so that we don't need to
put all the local variables interspersed throughout the function in each
for loop.

> +
> +       for (id = 0; id < M10V_NUM_CLKS; id++) {
> +               if (IS_ERR(m10v_clk_data->hws[id]))
> +                       return PTR_ERR(m10v_clk_data->hws[id]);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id m10v_clk_dt_ids[] = {
> +       { .compatible = "socionext,milbeaut-m10v-ccu", },
> +       { },

Drop the , on the sentinel please. That way nothing can ever go after it
without causing a compilation error.

> +};