| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 Feb 2019 21:13:07 +0900
From: "Sugaya, Taichi" <sugaya.taichi@...ionext.com>
To: Stephen Boyd <sboyd@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Michael Turquette <mturquette@...libre.com>,
Takao Orito <orito.takao@...ionext.com>,
Kazuhiro Kasai <kasai.kazuhiro@...ionext.com>,
Shinji Kanematsu <kanematsu.shinji@...ionext.com>,
Jassi Brar <jaswinder.singh@...aro.org>,
Masami Hiramatsu <masami.hiramatsu@...aro.org>
Subject: Re: [PATCH v2 08/15] clock: milbeaut: Add Milbeaut M10V clock
controller
Hi,
Thank you for your comments.
On 2019/02/23 8:57, Stephen Boyd wrote:
> 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.
>
I got it.
>> +
>> +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.
>
OK.
>> +
>> +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.
>
OK, I try to use "readl()" simply.
>> + 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?
>
Now there are no factors to use "CLK_DIVIDER_READ_ONLY".
I will drop this statement and ro_ops too.
>> + 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.
>
Yeah OK, I think these statements are bit of ugly too..
I try to make it easy to see.
>> +
>> + 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.
>
OK, drop it.
Thanks,
Sugaya Taichi
>> +};
Powered by blists - more mailing lists