Quoting Shubhrajyoti Datta (2020-08-28 06:39:53)

Currently the set rate granularity is to integral divisors.
Add support for the fractional divisors.
Only the first output0 is fractional in the hardware.

Signed-off-by: Shubhrajyoti Datta <redacted>

Getting closer.

Please remove extra parenthesis. And is this mult_frac()?

+}
+
+static int clk_wzrd_dynamic_reconfig_f(struct clk_hw *hw, unsigned long rate,
+                                      unsigned long parent_rate)
+{
+       int err;
+       u32 value, pre;
+       unsigned long rate_div, f, clockout0_div;
+       struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+       void __iomem *div_addr = divider->base + divider->offset;
+
+       rate_div = ((parent_rate * 1000) / rate);
+       clockout0_div = rate_div / 1000;
+
+       pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
+       f = (u32)(pre - (clockout0_div * 1000));
+       f = f & WZRD_CLKOUT_FRAC_MASK;
+
+       value = ((f << WZRD_CLKOUT_DIVIDE_WIDTH) | (clockout0_div &
+                       WZRD_CLKOUT_DIVIDE_MASK));

Please split this to multiple lines.

+
+       /* Set divisor and clear phase offset */
+       writel(value, div_addr);
+       writel(0x0, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
+
+       /* Check status register */
+       err= readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, value,
+                               value & WZRD_DR_LOCK_BIT_MASK,
+                               WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
+       if (err)
+               return err;
+
+       /* Initiate reconfiguration */
+       writel(WZRD_DR_BEGIN_DYNA_RECONF,
+              divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+       /* Check status register */
+       err= readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, value,
+                               value & WZRD_DR_LOCK_BIT_MASK,
+                               WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
+
+       return err;

Just return readl_poll_timeout() please.

+}
+
+static long clk_wzrd_round_rate_f(struct clk_hw *hw, unsigned long rate,
+                                 unsigned long *prate)
+{
+       return rate;

Can every rate be supported? This function is supposed to tell the clk
framework what rate will be achieved if we call clk_set_rate() with
'rate' passed to this function. Almost always returning 'rate' is not
the case.

+
+static const struct clk_ops clk_wzrd_clk_divider_ops_f = {
+       .round_rate = clk_wzrd_round_rate_f,
+       .set_rate = clk_wzrd_dynamic_reconfig_f,
+       .recalc_rate = clk_wzrd_recalc_ratef,
+};
+
+static struct clk *clk_wzrd_register_divf(struct device *dev,
+                                         const char *name,
+                                         const char *parent_name,
+                                         unsigned long flags,
+                                         void __iomem *base, u16 offset,
+                                         u8 shift, u8 width,
+                                         u8 clk_divider_flags,
+                                         const struct clk_div_table *table,
+                                         spinlock_t *lock)
+{
+       struct clk_wzrd_divider *div;
+       struct clk_hw *hw;
+       struct clk_init_data init;
+       int ret;
+
+       if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {

Is this used? It's a rockchip specific flag mostly so probably not?

+               if (width + shift > 16) {
+                       pr_warn("divider value exceeds LOWORD field\n");
+                       return ERR_PTR(-EINVAL);
+               }
+       }
+
+       /* allocate the divider */

Please remove useless comments like this.

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

+               init.ops = &clk_divider_ro_ops;
+       else
+               init.ops = &clk_wzrd_clk_divider_ops_f;
+
+       init.flags = flags;
+       init.parent_names = (parent_name ? &parent_name : NULL);
+       init.num_parents = (parent_name ? 1 : 0);

Do you have cases where there isn't a parent? Hopefully not, so this
can be simplified.

+       /* struct clk_divider assignments */

Drop this comment?

+       div->base = base;
+       div->offset = offset;
+       div->shift = shift;
+       div->width = width;
+       div->flags = clk_divider_flags;
+       div->lock = lock;
+       div->hw.init = &init;
+       div->table = table;
+
+       /* register the clock */

Drop this comment?

+       hw = &div->hw;
+       ret = clk_hw_register(dev, hw);

Any reason we can't use devm_clk_hw_register() here?

Use two lines please. Read into variable on one line, shift on another.

+
+       mult = ((reg * 1000) + reg_f);

Please remove extra parenthesis.

I wonder if a do-while loop with flags set to ONE_BASED and ALLOW_ZERO
could work and then flags gets overwritten to be just DIVIDE_SHIFT? Then
we don't have to duplicate the registration line.