Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211012134027.684712-1-kernel@esmil.dk> <20211012134027.684712-7-kernel@esmil.dk>
 <CAHp75Vetqi=FMoRxfXHL+d1DhAXgLr+5e7ss1M_Rkhqa07H5Jg@mail.gmail.com> <CANBLGcyCpSrxQi7pxLWQLkgbVGAKxJMXhoVbF_DdYDiv5_YJXQ@mail.gmail.com>
In-Reply-To: <CANBLGcyCpSrxQi7pxLWQLkgbVGAKxJMXhoVbF_DdYDiv5_YJXQ@mail.gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 13 Oct 2021 00:20:53 +0300
Message-ID: <CAHp75VcM97vcrNOG_T==E0JyZ_RT_n8MCiVtfmYQM4g8kVtegg@mail.gmail.com>
Subject: Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver
To:     Emil Renner Berthing <kernel@esmil.dk>
Cc:     linux-riscv <linux-riscv@lists.infradead.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-clk <linux-clk@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Rob Herring <robh+dt@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Linus Walleij <linus.walleij@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        Sagar Kadam <sagar.kadam@sifive.com>,
        Drew Fustini <drew@beagleboard.org>,
        Anup Patel <anup.patel@wdc.com>,
        Atish Patra <atish.patra@wdc.com>,
        Matteo Croce <mcroce@microsoft.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 12, 2021 at 11:08 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> 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:

...

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

Up to you, but I think it's better to have a usual pattern.

> > > +       writel_relaxed(value, reg);

...

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

Also needs a comment, I believe.

...

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

Actually, why can't you always initialize the field? Shouldn't CLK
core take care about this conditional?

> > > +#else
> > > +#define jh7100_clk_debug_init NULL
> > > +#endif

...

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

My point is exactly in having the common pattern for error paths, i.e.

  while (counter--)
    ...bla-bla-bla...

Your second approach is better, but I think that proposed by me is even better.

...

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

At least add a comment to explain the choice.

-- 
With Best Regards,
Andy Shevchenko