Re: [PATCH v5 03/39] clk: ep93xx: add DT support for Cirrus EP93xx

Andy Shevchenko
2023-11-22T11:39:38Z

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