lkml.org 
[lkml]   [2019]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
    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


    \
     
     \ /
      Last update: 2019-08-28 17:10    [W:3.686 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site