Messages in this thread | | | Date | Wed, 22 Nov 2023 13:39:33 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v5 03/39] clk: ep93xx: add DT support for Cirrus EP93xx |
| |
On Wed, Nov 22, 2023 at 11:59:41AM +0300, Nikita Shubin wrote: > Rewrite EP93xx clock driver located in arch/arm/mach-ep93xx/clock.c > trying to do everything the device tree way: > > - provide clock acces via of > - drop clk_hw_register_clkdev > - drop init code and use module_auxiliary_driver
...
> +static int ep93xx_clk_enable(struct clk_hw *hw) > +{ > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > + unsigned long flags; > + u32 val;
> + spin_lock_irqsave(&priv->lock, flags);
Have you considered to use cleanup.h and hence guard()() operator here (and elsewhere)? Why not?
> + regmap_read(priv->map, clk->reg, &val); > + val |= BIT(clk->bit_idx); > + > + ep93xx_clk_write(priv, clk->reg, val); > + > + spin_unlock_irqrestore(&priv->lock, flags); > + > + return 0; > +}
...
> +static int ep93xx_mux_set_parent_lock(struct clk_hw *hw, u8 index) > +{ > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > + unsigned long flags; > + u32 val; > + > + if (index >= 3) > + return -EINVAL; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + regmap_read(priv->map, clk->reg, &val); > + val &= ~(EP93XX_SYSCON_CLKDIV_MASK);
> + if (index) { > + val |= EP93XX_SYSCON_CLKDIV_ESEL; > + val |= (index - 1) ? EP93XX_SYSCON_CLKDIV_PSEL : 0; > + }
Maybe
val |= index > 0 ? EP93XX_SYSCON_CLKDIV_ESEL : 0; val |= index > 1 ? EP93XX_SYSCON_CLKDIV_PSEL : 0;
(at least to me it looks better to understand than the original code).
> + ep93xx_clk_write(priv, clk->reg, val); > + > + spin_unlock_irqrestore(&priv->lock, flags); > + > + return 0; > +}
...
> + for (i = 0; i < clk_hw_get_num_parents(hw); i++) { > + struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i); > + > + parent_rate = clk_hw_get_rate(parent); > + mclk_rate = parent_rate * 2; > + > + /* Try each predivider value */ > + for (pdiv = 4; pdiv <= 6; pdiv++) { > + div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv);
> + if (div < 1 || div > 127)
in_range() ?
> + continue; > + > + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv * div); > + if (is_best(rate, actual_rate, best_rate)) { > + best_rate = actual_rate; > + parent_rate_best = parent_rate; > + parent_best = parent; > + } > + } > + }
...
> + regmap_read(priv->map, clk->reg, &val); > + pdiv = ((val >> EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) & 0x03);
Too many parentheses. Why not GENMASK(1, 0) ?
> + div = val & GENMASK(6, 0); > + if (!div) > + return 0;
...
> + for (pdiv = 4; pdiv <= 6; pdiv++) { > + div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv); > + if (div < 1 || div > 127)
in_range() ?
> + continue; > + > + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv * div); > + if (abs(actual_rate - rate) < rate_err) { > + npdiv = pdiv - 3; > + ndiv = div; > + rate_err = abs(actual_rate - rate); > + } > + }
...
> + /* Clear old dividers */
Not sure if additional comment is needed to explain what is magical about bit 7...
> + val &= ~(GENMASK(9, 0) & ~BIT(7));
...
> + regmap_read(priv->map, clk->reg, &val); > + val &= ~clk->mask; > + for (i = 0; i < clk->num_div; i++) > + if (rate == DIV_ROUND_CLOSEST(parent_rate, clk->div[i])) {
> + val |= i << clk->shift;
This is an invariant to the loop...
> + break; > + } > + > + if (i == clk->num_div) > + return -EINVAL;
...can be moved here, right?
> + ep93xx_clk_write(priv, clk->reg, val);
...
> + priv->fixed[EP93XX_CLK_UART] = clk_hw_register_fixed_factor(NULL, "uart", "xtali", > + 0, 1, clk_uart_div);
I would format like
priv->fixed[EP93XX_CLK_UART] = clk_hw_register_fixed_factor(NULL, "uart", "xtali", 0, 1, clk_uart_div);
...
> + struct clk_parent_data xtali = { .index = 0 };
struct clk_parent_data xtali = { };
will suffice.
...
> + struct clk_parent_data pdata = {};
Please, keep the style consistent, either all with or without the inner space.
...
> + hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, "hclk", pll1, > + 0, 1, clk_h_div);
Seems you have already long lines, why not on one line here?
> + if (IS_ERR(hw)) > + return PTR_ERR(hw); > + > + priv->fixed[EP93XX_CLK_HCLK] = hw;
> + hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, "pclk", hw, > + 0, 1, clk_p_div);
And here?
> + if (IS_ERR(hw)) > + return PTR_ERR(hw);
...
> + clk_usb_div = (((value >> 28) & GENMASK(3, 0)) + 1);
Too many parentheses.
...
> + * So i set both dividers to minimum.
s/i/we/ ?
...
> + /* ENA - Enable CLK divider. */ > + /* PDIV - 00 - Disable clock */ > + /* VDIV - at least 2 */ > + /* Check and enable video clk registers */
Hmm... Why it can't be unified under a single multi-line comment?
-- With Best Regards, Andy Shevchenko
| |