Re: [PATCH v6 04/40] clk: ep93xx: add DT support for Cirrus EP93xx

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

On Tue, Dec 12, 2023 at 11:20:21AM +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

...

> +#define EP93XX_I2SCLKDIV_SDIV		(1 << 16)

BIT() ?

...

> +static u8 ep93xx_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> +	struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> +	u32 val;
> +
> +	regmap_read(priv->map, clk->reg, &val);
> +
> +	val &= EP93XX_SYSCON_CLKDIV_MASK;
> +
> +	switch (val) {
> +	case EP93XX_SYSCON_CLKDIV_ESEL:
> +		return 1; /* PLL1 */
> +	case EP93XX_SYSCON_CLKDIV_MASK:
> +		return 2; /* PLL2 */

> +	default:
> +		break;
> +	};
> +
> +	return 0; /* XTALI */

You may return directly from default.

> +}

...

> +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);

Why not guard() ?

> +	regmap_read(priv->map, clk->reg, &val);
> +	val &= ~(EP93XX_SYSCON_CLKDIV_MASK);
> +	val |= index > 0 ? EP93XX_SYSCON_CLKDIV_ESEL : 0;
> +	val |= index > 1 ? EP93XX_SYSCON_CLKDIV_PSEL : 0;
> +
> +	ep93xx_clk_write(priv, clk->reg, val);
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}

...

> +static bool is_best(unsigned long rate, unsigned long now,
> +		     unsigned long best)
> +{
> +	return abs_diff(rate, now) < abs_diff(rate, best);

Have you included necessary header for this?

> +}

...

> +static int ep93xx_mux_determine_rate(struct clk_hw *hw,
> +				struct clk_rate_request *req)
> +{
> +	unsigned long best_rate = 0, actual_rate, mclk_rate;
> +	unsigned long rate = req->rate;

> +	struct clk_hw *parent_best = NULL;

Strictly speaking you don't need an assignment here as you can compare the loop
variable value against the maximum. But I don't know how heave the respective
CLk call is and if it has no side-effects due to operations inside the loop body.

> +	unsigned long parent_rate_best;
> +	unsigned long parent_rate;
> +	int div, pdiv;
> +	unsigned int i;
> +
> +	/*
> +	 * Try the two pll's and the external clock

Either comma + 'b' or missing period.

> +	 * Because the valid predividers are 2, 2.5 and 3, we multiply
> +	 * all the clocks by 2 to avoid floating point math.
> +	 *
> +	 * This is based on the algorithm in the ep93xx raster guide:
> +	 * http://be-a-maverick.com/en/pubs/appNote/AN269REV1.pdf
> +	 *
> +	 */
> +	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 (!in_range(div, 1, 127))

Same header as for abs_diff()?

> +				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;
> +			}
> +		}

(1)

> +	}
> +
> +	if (!parent_best)
> +		return -EINVAL;
> +
> +	req->best_parent_rate = parent_rate_best;
> +	req->best_parent_hw = parent_best;
> +	req->rate = best_rate;
> +
> +	return 0;
> +}

...

> +	mclk_rate = parent_rate * 2;
> +
> +	for (pdiv = 4; pdiv <= 6; pdiv++) {
> +		div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv);
> +		if (!in_range(div, 1, 127))
> +			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);
> +		}
> +	}

Looks very similar to (1). Can be deduplicated?

...

> +	/*
> +	 * Clear old dividers
> +	 * Bit 7 is reserved bit in all ClkDiv registers

Missing periods.

> +	 */

...

> +static unsigned long calc_pll_rate(u64 rate, u32 config_word)
> +{
> +	rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1;	/* X1FBD */
> +	rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1;	/* X2FBD */
> +	do_div(rate, (config_word & GENMASK(4, 0)) + 1);	/* X2IPD */

> +	rate >>= ((config_word >> 16) & GENMASK(1, 0));		/* PS */

Outer parentheses are redundant.

> +	return rate;
> +}

...

> +	/*
> +	 * EP93xx SSP clock rate was doubled in version E2. For more information
> +	 * see:
> +	 *     http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf

Can you point to the specific section? Like

	 * see section 1.2.3 "Foo bar":

> +	 */

...

> +	/* touchscreen/adc clock */

ADC

...

> +	/*
> +	 * On reset PDIV and VDIV is set to zero, while PDIV zero
> +	 * means clock disable, VDIV shouldn't be zero.
> +	 * So we set both video and is2 dividers to minimum.

i2s?

> +	 * ENA - Enable CLK divider.
> +	 * PDIV - 00 - Disable clock
> +	 * VDIV - at least 2
> +	 */

-- 
With Best Regards,
Andy Shevchenko