On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> support for other platforms from Mobileye.
>
> It handles 10 read-only PLLs derived from the main crystal on board. It
If you wrap 'It' to the next line, overall text will look better.
> exposes a table-based divider clock used for OSPI. Other platform
> clocks are not configurable and therefore kept as fixed-factor
> devicetree nodes.
>
> Two PLLs are required early on and are therefore registered at
> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
Ditto for 'the'
> UARTs.
...
> +config COMMON_CLK_EYEQ5
> + bool "Clock driver for the Mobileye EyeQ5 platform"
> + depends on OF
Since it's a functional dependency, why not allow compile test without OF being
enabled?
> + depends on MACH_EYEQ5 || COMPILE_TEST
> + default MACH_EYEQ5
> + help
> + This driver provides the clocks found on the Mobileye EyeQ5 SoC. Its
> + registers live in a shared register region called OLB. It provides 10
> + read-only PLLs derived from the main crystal clock which must be constant
> + and one divider clock based on one PLL.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
+ errno.h (yes, you need both)
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
+ overflow.h
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
...
> +struct eq5c_pll {
> + int index;
Index can be negative? Any comment about this case?
> + const char *name;
> + u32 reg; /* next 8 bytes are r0 and r1 */
Not sure this comments gives any clarification to a mere reader of the code.
Perhaps you want to name this as reg64 (at least it will show that you have
8 bytes, but I have no clue what is the semantic relationship between r0 and
r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> +};
...
> +static int eq5c_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
It's used only once. Why not just use dev->of_node there?
> + void __iomem *base_plls, *base_ospi;
> + struct clk_hw *hw;
> + int i;
> +
> + /* Return potential error from eq5c_init(). */
> + if (IS_ERR(eq5c_clk_data))
> + return PTR_ERR(eq5c_clk_data);
> + /* Return an error if eq5c_init() did not get called. */
> + else if (!eq5c_clk_data)
Redundant 'else'
> + return -EINVAL;
I didn't get. If eq5c_init() was finished successfully, why do you need to
seems repeat what it already done? What did I miss?
> + base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> + if (IS_ERR(base_plls))
> + return PTR_ERR(base_plls);
> +
> + base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
> + if (IS_ERR(base_ospi))
> + return PTR_ERR(base_ospi);
> +
> + for (i = 0; i < ARRAY_SIZE(eq5c_plls); i++) {
> + const struct eq5c_pll *pll = &eq5c_plls[i];
> + unsigned long mult, div, acc;
> + u32 r0, r1;
> + int ret;
> +
> + r0 = readl(base_plls + pll->reg);
> + r1 = readl(base_plls + pll->reg + sizeof(r0));
> +
> + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> + if (ret) {
> + dev_warn(dev, "failed parsing state of %s\n", pll->name);
> + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> + continue;
> + }
> +
> + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
> + pll->name, "ref", 0, mult, div, acc);
> + eq5c_clk_data->hws[pll->index] = hw;
> + if (IS_ERR(hw))
> + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> + pll->name);
Missed return statement?
> + }
> +
> + hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME,
> + eq5c_clk_data->hws[EQ5C_PLL_PER], 0,
> + base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0,
> + eq5c_ospi_div_table, NULL);
> + eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw;
> + if (IS_ERR(hw))
> + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> + EQ5C_OSPI_DIV_CLK_NAME);
Ditto.
> + return 0;
> +}
> +static void __init eq5c_init(struct device_node *np)
> +{
> + void __iomem *base_plls, *base_ospi;
> + int index_plls, index_ospi;
> + int i, ret;
Why is i signed?
> + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
> + GFP_KERNEL);
> + if (!eq5c_clk_data) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + eq5c_clk_data->num = EQ5C_NB_CLKS;
> +
> + /*
> + * Mark all clocks as deferred. We register some now and others at
> + * platform device probe.
> + */
> + for (i = 0; i < EQ5C_NB_CLKS; i++)
> + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> + index_plls = of_property_match_string(np, "reg-names", "plls");
> + if (index_plls < 0) {
> + ret = index_plls;
> + goto err;
> + }
Better pattern is to avoid the output pollution in the error case. Hence
ret = of_property_match_string(np, "reg-names", "plls");
if (ret < 0)
goto err;
index_plls = ret;
> + index_ospi = of_property_match_string(np, "reg-names", "ospi");
> + if (index_ospi < 0) {
> + ret = index_ospi;
> + goto err;
> + }
Ditto.
> + base_plls = of_iomap(np, index_plls);
> + base_ospi = of_iomap(np, index_ospi);
> + if (!base_plls || !base_ospi) {
> + ret = -ENODEV;
> + goto err;
> + }
> + for (i = 0; i < ARRAY_SIZE(eq5c_early_plls); i++) {
> + const struct eq5c_pll *pll = &eq5c_early_plls[i];
> + unsigned long mult, div, acc;
> + struct clk_hw *hw;
> + u32 r0, r1;
> +
> + r0 = readl(base_plls + pll->reg);
> + r1 = readl(base_plls + pll->reg + sizeof(r0));
> +
> + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> + if (ret) {
> + pr_warn("failed parsing state of %s\n", pll->name);
> + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> + continue;
> + }
> +
> + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
> + np, pll->name, "ref", 0, mult, div, acc);
> + eq5c_clk_data->hws[pll->index] = hw;
> + if (IS_ERR(hw))
> + pr_err("failed registering %s: %ld\n",
%pe ?
> + pll->name, PTR_ERR(hw));
Is the error not critical? Is it fine? How is it supposed to work at such
circumstances?
> + }
> +
> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, eq5c_clk_data);
> + if (ret) {
> + pr_err("failed registering clk provider: %d\n", ret);
> + goto err;
> + }
> +
> + return;
> +
> +err:
> + kfree(eq5c_clk_data);
> + /* Signal to platform driver probe that we failed init. */
> + eq5c_clk_data = ERR_PTR(ret);
> +}
> +
> +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
--
With Best Regards,
Andy Shevchenko
