| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 22 Nov 2023 13:39:33 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Nikita Shubin <nikita.shubin@...uefel.me>
Cc: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org,
linux-clk@...r.kernel.org,
Alexander Sverdlin <alexander.sverdlin@...il.com>
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
Powered by blists - more mailing lists