| 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: Tue, 12 Oct 2021 21:40:05 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Emil Renner Berthing <kernel@...il.dk>
Cc: linux-riscv <linux-riscv@...ts.infradead.org>,
devicetree <devicetree@...r.kernel.org>,
linux-clk <linux-clk@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
"open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Rob Herring <robh+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Marc Zyngier <maz@...nel.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Linus Walleij <linus.walleij@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Jiri Slaby <jirislaby@...nel.org>,
Maximilian Luz <luzmaximilian@...il.com>,
Sagar Kadam <sagar.kadam@...ive.com>,
Drew Fustini <drew@...gleboard.org>,
Anup Patel <anup.patel@....com>,
Atish Patra <atish.patra@....com>,
Matteo Croce <mcroce@...rosoft.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver
On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing <kernel@...il.dk> wrote:
>
> From: Geert Uytterhoeven <geert@...ux-m68k.org>
>
> Add a driver for the StarFive JH7100 clock generator.
...
> +config CLK_STARFIVE_JH7100
> + bool "StarFive JH7100 clock support"
> + depends on SOC_STARFIVE || COMPILE_TEST
> + depends on OF
Why? I haven't found a compile dependency, so you reduce the test
scope (when COMPILE_TEST=y).
...
You are using
bits.h
mod_devicetable.h
which are not here
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
...
> + value |= readl_relaxed(reg) & ~mask;
value is not masked, is it okay?
Usual pattern for this kind of operations is
value = (current & ~mask) | (value & mask);
> + writel_relaxed(value, reg);
...
> + if (div > max)
> + div = max;
> +
> + return div;
return min(div, max); ?
...
> + rate = parent / div;
> + if (rate < req->min_rate && div > 1) {
> + div -= 1;
> + rate = parent / div;
> + }
Seems like homegrown DIV_ROUND_UP() or so. Who will guarantee that
decreasing div by 1 will satisfy the conditional again?
...
> +#ifdef CONFIG_DEBUG_FS
Perhaps __maybe_unused?
> +#else
> +#define jh7100_clk_debug_init NULL
> +#endif
...
> + if (idx >= JH7100_CLK_END) {
> + dev_err(priv->dev, "%s: invalid index %u\n", __func__, idx);
__func__ means that the message has no value on its own. Make it
unique without using __func__, or drop completely.
> + return ERR_PTR(-EINVAL);
> + }
...
> + for (idx = 0; idx < JH7100_CLK_PLL0_OUT; idx++) {
> + struct clk_init_data init = {
> + .name = jh7100_clk_data[idx].name,
> + .ops = jh7100_clk_data[idx].ops,
> + .num_parents = ((jh7100_clk_data[idx].max & JH7100_CLK_MUX_MASK)
> + >> JH7100_CLK_MUX_SHIFT) + 1,
With temporary variable this can be better written, or consider
something like this
.num_parents =
((jh7100_clk_data[idx].max &
JH7100_CLK_MUX_MASK) >> JH7100_CLK_MUX_SHIFT) + 1,
> + .flags = jh7100_clk_data[idx].flags,
> + };
> + struct jh7100_clk *clk = &priv->reg[idx];
...
> + while (idx > 0)
> + clk_hw_unregister(&priv->reg[--idx].hw);
The
while (idx--)
clk_hw_unregister(&priv->reg[idx].hw);
is slightly better to read.
> + return ret;
> +}
...
> +static int __init clk_starfive_jh7100_init(void)
> +{
> + return platform_driver_probe(&clk_starfive_jh7100_driver,
> + clk_starfive_jh7100_probe);
> +}
> +
No need to have this blank line.
> +subsys_initcall(clk_starfive_jh7100_init);
Any explanation why subsys_initcall() is in use?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists