lkml.org 
[lkml]   [2021]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver
    On Tue, 12 Oct 2021 at 17:40, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
    >
    > On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
    > >
    > > From: Geert Uytterhoeven <geert@linux-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).

    My thinking was that it can't ever be loaded on a !OF system, but
    you're right it'll just restrict compile testing. I'll remove, thanks.

    > ...
    >
    > 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);

    This function is only ever called with constants, already masked
    values or the parent number from the clk framework, so it should be
    ok.

    > > + 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?

    Maths unless I'm mistaken: div = DIV_ROUND_UP(parent, target), so in
    rational numbers
    div - 1 < parent / target
    But the target is clamped by min_rate and max_rate, so
    min_rate <= target < parent / (div - 1) = rate

    Sorry, re-using the rate varable for both the target and result is
    confusing. I'll fix that.

    > ...
    >
    > > +#ifdef CONFIG_DEBUG_FS
    >
    > Perhaps __maybe_unused?

    I can definitely use __maybe_unused for the function declaration, but
    then I'll need a conditional every time clk_ops.debug_init needs to be
    initialized to either the function or NULL depending on
    CONFIG_DEBUG_FS below. Is that better?

    > > +#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.

    It's not something I'll insist hard on, but I must admit I disagree.
    To me the above looks like cartoon characters running off a cliff and
    back. As a middle ground could we maybe do this?

    while (idx)
    clk_hw_unregister(&priv->reg[--idx].hw);

    > > + 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?

    TBH I just inherited that from Geert's first mock driver and never
    thought to question it. What would be a better alternative to try?

    Thanks!
    /Emil

    \
     
     \ /
      Last update: 2021-10-12 22:09    [W:5.410 / U:0.696 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site