Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:
> From: rtanwar <rahul.tanwar@intel.com>
> 
> Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming
> Intel network processor SoC. It provides programming interfaces to control
> & configure all CPU & peripheral clocks. Add common clock framework based
> clock controller driver for CGU.

>  drivers/clk/intel/Kconfig       |  13 +
>  drivers/clk/intel/Makefile      |   4 +

Any plans what to do with existing x86 folder there?

> +++ b/drivers/clk/intel/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +config INTEL_LGM_CGU_CLK
> +	depends on COMMON_CLK
> +	select MFD_SYSCON
> +	select OF_EARLY_FLATTREE
> +	bool "Intel Clock Genration Unit support"

Is it for X86? Don't you need a dependency?

> +/*
> + * Calculate formula:
> + * rate = (prate * mult + (prate * frac) / frac_div) / div
> + */
> +static unsigned long
> +intel_pll_calc_rate(unsigned long prate, unsigned int mult,
> +		    unsigned int div, unsigned int frac, unsigned int frac_div)
> +{
> +	u64 crate, frate, rate64;
> +
> +	rate64 = prate;
> +	crate = rate64 * mult;
> +

> +	if (frac) {

This seems unnecessary.
I think you would like to check for frac_div instead?
Though I would rather to use frac = 0, frac_div = 1 and drop this conditional
completely.

> +		frate = rate64 * frac;
> +		do_div(frate, frac_div);
> +		crate += frate;
> +	}
> +	do_div(crate, div);
> +
> +	return (unsigned long)crate;
> +}

> +static struct clk_hw

> +*intel_clk_register_pll(struct intel_clk_provider *ctx,

* is part of type.

> +			const struct intel_pll_clk_data *list)
> +{
> +	struct clk_init_data init;
> +	struct intel_clk_pll *pll;
> +	struct device *dev = ctx->dev;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	init.ops = &intel_lgm_pll_ops;
> +	init.name = list->name;
> +	init.parent_names = list->parent_names;
> +	init.num_parents = list->num_parents;
> +
> +	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> +	if (!pll)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pll->map = ctx->map;
> +	pll->dev = ctx->dev;
> +	pll->reg = list->reg;
> +	pll->flags = list->flags;
> +	pll->type = list->type;
> +	pll->hw.init = &init;
> +

> +	hw = &pll->hw;

Seems redundant temporary variable.

> +	ret = clk_hw_register(dev, hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return hw;
> +}

> +void intel_clk_register_plls(struct intel_clk_provider *ctx,
> +			     const struct intel_pll_clk_data *list,
> +				unsigned int nr_clk)

Indentation issues.

> +{
> +	struct clk_hw *hw;
> +	int i;
> +
> +	for (i = 0; i < nr_clk; i++, list++) {
> +		hw = intel_clk_register_pll(ctx, list);
> +		if (IS_ERR(hw)) {

> +			dev_err(ctx->dev, "failed to register pll: %s\n",
> +				list->name);

Is it fatal or not?

> +			continue;
> +		}
> +
> +		intel_clk_add_lookup(ctx, hw, list->id);
> +	}

No error to return? Are all PLLs optional?

> +}

> +#endif				/* __INTEL_CLK_PLL_H */

One TAB is enough.

> +/*
> + *  Copyright (C) 2018 Intel Corporation.
> + *  Zhu YiXin <Yixin.zhu@intel.com>

On space after asterisk is enough.

> + */

> +#define to_intel_clk_divider(_hw) \
> +			container_of(_hw, struct intel_clk_divider, hw)

One TAB is enough.

> +	val >>= shift;
> +	val &= BIT(width) - 1;
> +
> +	return val;

Can be one line, though up to you.

> +	pr_debug("Add clk: %s, id: %u\n", clk_hw_get_name(hw), id);

Is this useful?

> +static struct clk_hw

> +*intel_clk_register_fixed(struct intel_clk_provider *ctx,

* is part of the type.

> +			  const struct intel_clk_branch *list)

> +static struct clk_hw

> +*intel_clk_register_fixed_factor(struct intel_clk_provider *ctx,

Ditto.

> +				 const struct intel_clk_branch *list)

> +static struct clk_hw

> +*intel_clk_register_gate(struct intel_clk_provider *ctx,

Ditto.

> +			 const struct intel_clk_branch *list)

> +/*
> + * Below table defines the pair's of regval & effective dividers.
> + * It's more efficient to provide an explicit table due to non-linear
> + * relation between values.
> + */
> +static const struct clk_div_table pll_div[] = {

Does val == 0 follows the table, i.e. makes div == 1?

> +	{ .val = 1, .div = 2 },
> +	{ .val = 2, .div = 3 },
> +	{ .val = 3, .div = 4 },
> +	{ .val = 4, .div = 5 },
> +	{ .val = 5, .div = 6 },
> +	{ .val = 6, .div = 8 },
> +	{ .val = 7, .div = 10 },
> +	{ .val = 8, .div = 12 },
> +	{ .val = 9, .div = 16 },
> +	{ .val = 10, .div = 20 },
> +	{ .val = 11, .div = 24 },
> +	{ .val = 12, .div = 32 },
> +	{ .val = 13, .div = 40 },
> +	{ .val = 14, .div = 48 },
> +	{ .val = 15, .div = 64 },
> +	{}
> +};

> +enum lgm_plls {
> +	PLL0CZ, PLL0B, PLL1, PLL2, PLLPP, LJPLL3, LJPLL4

At the end you may put comma just for slightly better maintenance.

> +};

> +static int __init intel_lgm_cgu_probe(struct platform_device *pdev)
> +{
> +	struct intel_clk_provider *ctx;
> +	struct regmap *map;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +

> +	if (!np)
> +		return -ENODEV;

Wouldn't the below fail?
That said, do you need this check at all?

> +
> +	map = syscon_node_to_regmap(np);
> +	if (IS_ERR(map))

> +		return -ENODEV;

Why shadow error code?

> +
> +	ctx = intel_clk_init(dev, map, CLK_NR_CLKS);
> +	if (IS_ERR(ctx))

> +		return -ENOMEM;

Ditto.

> +}

-- 
With Best Regards,
Andy Shevchenko