Re: [PATCH v8 03/10] clk: eyeq5: add platform driver, and init routine at of_clk_init()

[Andy Shevchenko]

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