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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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