Re: [PATCH v10 03/38] clk: ep93xx: add DT support for Cirrus EP93xx

Stephen Boyd
2024-07-08T22:18:46Z

Quoting Nikita Shubin via B4 Relay (2024-06-17 02:36:37)
> diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c
> new file mode 100644
> index 000000000000..a0430a5ae4da
> --- /dev/null
> +++ b/drivers/clk/clk-ep93xx.c
> @@ -0,0 +1,834 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
[...]
> +
> +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);
> + u32 val;
> +
> + guard(spinlock_irqsave)(&priv->lock);

I thought guard() was most important when there were multiple exit paths
from a function, but OK.

> +
[...]
> +
> +static int ep93xx_plls_init(struct ep93xx_clk_priv *priv)
> +{
> + const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> + const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
[...]
> + if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> + clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> + else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> + clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> + else
> + clk_pll2_rate = 0;
> +
> + hw = devm_clk_hw_register_fixed_rate(dev, "pll2", "xtali", 0, clk_pll2_rate);

Please use clk_parent_data for topology descriptions.

> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + priv->fixed[EP93XX_CLK_PLL2] = hw;
> +
> + return 0;
> +}
> +
> +static int ep93xx_clk_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct ep93xx_regmap_adev *rdev = to_ep93xx_regmap_adev(adev);
> + struct clk_parent_data xtali = { .index = 0 };
> + struct clk_parent_data ddiv_pdata[3] = { };
> + unsigned int clk_spi_div, clk_usb_div;
> + struct clk_parent_data pdata = {};
> + struct device *dev = &adev->dev;
> + struct ep93xx_clk_priv *priv;
> + struct ep93xx_clk *clk;
> + struct clk_hw *hw;
> + unsigned int idx;
> + int ret;
> + u32 value;
> +
> + priv = devm_kzalloc(dev, struct_size(priv, reg, 10), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + spin_lock_init(&priv->lock);
> + priv->dev = dev;
> + priv->aux_dev = rdev;
> + priv->map = rdev->map;
> + priv->base = rdev->base;
> +
> + ret = ep93xx_plls_init(priv);
> + if (ret)
> + return ret;
> +
> + regmap_read(priv->map, EP93XX_SYSCON_CLKSET2, &value);
> + clk_usb_div = (value >> 28 & GENMASK(3, 0)) + 1;
> + hw = devm_clk_hw_register_fixed_factor(dev, "usb_clk", "pll2", 0, 1, clk_usb_div);

This one can use clk_hw to reference pll2.

> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + priv->fixed[EP93XX_CLK_USB] = hw;
> +
> + ret = ep93xx_uart_clock_init(priv);
> + if (ret)
> + return ret;
> +
> + ret = ep93xx_dma_clock_init(priv);
> + if (ret)
> + return ret;
> +
> + clk_spi_div = id->driver_data;
> + hw = devm_clk_hw_register_fixed_factor(dev, "ep93xx-spi.0", "xtali",

Are these clk names trying to match device names?

> + 0, 1, clk_spi_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + priv->fixed[EP93XX_CLK_SPI] = hw;
> +
> + /* PWM clock */
> + hw = devm_clk_hw_register_fixed_factor(dev, "pwm_clk", "xtali", 0, 1, 1);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + priv->fixed[EP93XX_CLK_PWM] = hw;
> +
> + /* USB clock */
> + hw = devm_clk_hw_register_gate(priv->dev, "ohci-platform", "usb_clk",
> + 0, priv->base + EP93XX_SYSCON_PWRCNT,
> + EP93XX_SYSCON_PWRCNT_USH_EN, 0,
> + &priv->lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +