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

[Stephen Boyd <sboyd@kernel.org>]

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