On Tue, 12 Oct 2021 at 23:21, Andy Shevchenko [off-list ref] wrote:

On Tue, Oct 12, 2021 at 11:08 PM Emil Renner Berthing [off-list ref] wrote:

quoted

On Tue, 12 Oct 2021 at 17:40, Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing [off-list ref] wrote:

...

quoted

quoted

quoted

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

quoted

quoted

quoted

+       writel_relaxed(value, reg);

...

quoted

quoted

quoted

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

Will add.

...

quoted

quoted

quoted

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

It could, but I see other drivers avoiding the code bloat when debugfs
is not enabled, so I thought I'd copy that.

quoted

quoted

quoted

+#else
+#define jh7100_clk_debug_init NULL
+#endif

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

+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