lkml.org 
[lkml]   [2020]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver
    From
    Date
    Quoting Daniel Palmer (2020-11-14 05:50:41)
    > F: include/dt-bindings/gpio/msc313-gpio.h
    > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
    > index c715d4681a0b..a002f2605fa3 100644
    > --- a/drivers/clk/Kconfig
    > +++ b/drivers/clk/Kconfig
    > @@ -370,6 +370,7 @@ source "drivers/clk/ingenic/Kconfig"
    > source "drivers/clk/keystone/Kconfig"
    > source "drivers/clk/mediatek/Kconfig"
    > source "drivers/clk/meson/Kconfig"
    > +source "drivers/clk/mstar/Kconfig"
    > source "drivers/clk/mvebu/Kconfig"
    > source "drivers/clk/qcom/Kconfig"
    > source "drivers/clk/renesas/Kconfig"

    This looks good.

    > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
    > index da8fcf147eb1..b758aae17ab8 100644
    > --- a/drivers/clk/Makefile
    > +++ b/drivers/clk/Makefile
    > @@ -124,3 +124,4 @@ endif
    > obj-$(CONFIG_ARCH_ZX) += zte/
    > obj-$(CONFIG_ARCH_ZYNQ) += zynq/
    > obj-$(CONFIG_COMMON_CLK_ZYNQMP) += zynqmp/
    > +obj-$(CONFIG_ARCH_MSTARV7) += mstar/

    This is in the wrong place. It looks to be sorted based on the path
    name, so mstar/ comes much earlier in this file.

    > diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig
    > new file mode 100644
    > index 000000000000..23765edde3af
    > --- /dev/null
    > +++ b/drivers/clk/mstar/Kconfig
    > @@ -0,0 +1,5 @@
    > +# SPDX-License-Identifier: GPL-2.0-only
    > +config MSTAR_MSC313_MPLL
    > + bool
    > + select REGMAP
    > + select REGMAP_MMIO
    > diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile
    > new file mode 100644
    > index 000000000000..f8dcd25ede1d
    > --- /dev/null
    > +++ b/drivers/clk/mstar/Makefile
    > @@ -0,0 +1,6 @@
    > +# SPDX-License-Identifier: GPL-2.0
    > +#
    > +# Makefile for mstar specific clk
    > +#
    > +
    > +obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.o
    > diff --git a/drivers/clk/mstar/clk-msc313-mpll.c b/drivers/clk/mstar/clk-msc313-mpll.c
    > new file mode 100644
    > index 000000000000..c1e2fe0fc412
    > --- /dev/null
    > +++ b/drivers/clk/mstar/clk-msc313-mpll.c
    > @@ -0,0 +1,177 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * MStar MSC313 MPLL driver
    > + *
    > + * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp>
    > + */
    > +
    > +#include <linux/platform_device.h>
    > +#include <linux/of.h>
    > +#include <linux/clk-provider.h>
    > +#include <linux/clkdev.h>

    Please remove unused includes

    > +#include <linux/of_address.h>
    > +#include <linux/module.h>

    Isn't it builtin? This include should be removed.

    > +#include <linux/regmap.h>
    > +
    > +#define REG_CONFIG1 0x8
    > +#define REG_CONFIG2 0xc
    > +
    > +static const struct regmap_config msc313_mpll_regmap_config = {
    > + .reg_bits = 16,
    > + .val_bits = 16,
    > + .reg_stride = 4,
    > +};
    > +
    > +static const struct reg_field config1_loop_div_first = REG_FIELD(REG_CONFIG1, 8, 9);
    > +static const struct reg_field config1_input_div_first = REG_FIELD(REG_CONFIG1, 4, 5);
    > +static const struct reg_field config2_output_div_first = REG_FIELD(REG_CONFIG2, 12, 13);
    > +static const struct reg_field config2_loop_div_second = REG_FIELD(REG_CONFIG2, 0, 7);
    > +
    > +static const unsigned int output_dividers[] = {
    > + 2, 3, 4, 5, 6, 7, 10
    > +};
    > +
    > +#define NUMOUTPUTS (ARRAY_SIZE(output_dividers) + 1)
    > +
    > +struct msc313_mpll {
    > + struct clk_hw clk_hw;
    > + struct regmap_field *input_div;
    > + struct regmap_field *loop_div_first;
    > + struct regmap_field *loop_div_second;
    > + struct regmap_field *output_div;
    > + struct clk_hw_onecell_data *clk_data;
    > +};
    > +
    > +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw)
    > +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1]

    I'd rather not have this macro. It's confusing given that to_foo()
    macros are usually a container_of() invocation. Given that it's only
    used in the registration/unregistration loops please inline it and use a
    local variable.

    > +
    > +static unsigned long msc313_mpll_recalc_rate(struct clk_hw *hw,
    > + unsigned long parent_rate)
    > +{
    > + struct msc313_mpll *mpll = to_mpll(hw);
    > + unsigned int input_div, output_div, loop_first, loop_second;
    > + unsigned long output_rate;
    > +
    > + regmap_field_read(mpll->input_div, &input_div);
    > + regmap_field_read(mpll->output_div, &output_div);
    > + regmap_field_read(mpll->loop_div_first, &loop_first);
    > + regmap_field_read(mpll->loop_div_second, &loop_second);
    > +
    > + output_rate = parent_rate / (1 << input_div);
    > + output_rate *= (1 << loop_first) * max(loop_second, 1U);
    > + output_rate /= max(output_div, 1U);
    > +
    > + return output_rate;
    > +}
    > +
    > +static const struct clk_ops msc313_mpll_ops = {
    > + .recalc_rate = msc313_mpll_recalc_rate,

    Weird double indent here.

    > +};
    > +
    > +static int msc313_mpll_probe(struct platform_device *pdev)
    > +{
    > + void __iomem *base;
    > + struct msc313_mpll *mpll;
    > + struct clk_init_data clk_init;
    > + struct device *dev = &pdev->dev;
    > + struct regmap *regmap;
    > + const char *parents[1], *outputnames[NUMOUTPUTS];
    > + const int numparents = ARRAY_SIZE(parents);
    > + int ret, i;
    > +
    > + if (of_clk_parent_fill(dev->of_node, parents, numparents) != numparents)
    > + return -EINVAL;
    > +
    > + if (of_property_read_string_array(pdev->dev.of_node, "clock-output-names",

    Hopefully this isn't required.

    > + outputnames, NUMOUTPUTS) != NUMOUTPUTS)
    > + return -EINVAL;
    > +
    > + mpll = devm_kzalloc(dev, sizeof(*mpll), GFP_KERNEL);
    > + if (!mpll)
    > + return -ENOMEM;
    > +
    > + base = devm_platform_ioremap_resource(pdev, 0);
    > + if (IS_ERR(base))
    > + return PTR_ERR(base);
    > +
    > + regmap = devm_regmap_init_mmio(dev, base, &msc313_mpll_regmap_config);
    > + if (IS_ERR(regmap))
    > + return PTR_ERR(regmap);
    > +
    > + mpll->input_div = devm_regmap_field_alloc(dev, regmap, config1_input_div_first);
    > + if (IS_ERR(mpll->input_div))
    > + return PTR_ERR(mpll->input_div);
    > + mpll->output_div = devm_regmap_field_alloc(dev, regmap, config2_output_div_first);
    > + if (IS_ERR(mpll->output_div))
    > + return PTR_ERR(mpll->output_div);
    > + mpll->loop_div_first = devm_regmap_field_alloc(dev, regmap, config1_loop_div_first);
    > + if (IS_ERR(mpll->loop_div_first))
    > + return PTR_ERR(mpll->loop_div_first);
    > + mpll->loop_div_second = devm_regmap_field_alloc(dev, regmap, config2_loop_div_second);
    > + if (IS_ERR(mpll->loop_div_second))
    > + return PTR_ERR(mpll->loop_div_second);
    > +
    > + mpll->clk_data = devm_kzalloc(dev, struct_size(mpll->clk_data, hws,
    > + ARRAY_SIZE(output_dividers)), GFP_KERNEL);
    > + if (!mpll->clk_data)
    > + return -ENOMEM;
    > +
    > + clk_init.name = outputnames[0];
    > + clk_init.ops = &msc313_mpll_ops;
    > + clk_init.num_parents = 1;
    > + clk_init.parent_names = parents;
    > + mpll->clk_hw.init = &clk_init;
    > +
    > + ret = devm_clk_hw_register(dev, &mpll->clk_hw);
    > + if (ret)
    > + return ret;
    > +
    > + mpll->clk_data->num = NUMOUTPUTS;
    > + mpll->clk_data->hws[0] = &mpll->clk_hw;
    > +
    > + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) {
    > + to_divider_hw(mpll, i) = clk_hw_register_fixed_factor(dev,
    > + outputnames[i + 1], outputnames[0], 0, 1, output_dividers[i]);
    > + if (IS_ERR(to_divider_hw(mpll, i))) {
    > + ret = PTR_ERR(to_divider_hw(mpll, i));
    > + goto unregister_dividers;
    > + }
    > + }
    > +
    > + platform_set_drvdata(pdev, mpll);
    > +
    > + return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
    > + mpll->clk_data);
    > +
    > +unregister_dividers:
    > + for (i--; i >= 0; i--)
    > + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
    > + return ret;
    > +}
    > +
    > +static int msc313_mpll_remove(struct platform_device *pdev)
    > +{
    > + struct msc313_mpll *mpll = platform_get_drvdata(pdev);
    > + int i;
    > +
    > + for (i = 0; i < ARRAY_SIZE(output_dividers); i++)
    > + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));

    Maybe add a devm_ for this if it doesn't exist.

    > +
    > + return 0;
    > +}
    > +
    > +static const struct of_device_id msc313_mpll_of_match[] = {
    > + { .compatible = "mstar,msc313-mpll", },
    > + {}
    > +};
    > +MODULE_DEVICE_TABLE(of, msc313_mpll_of_match);

    Drop this. It isn't a module.

    > +
    > +static struct platform_driver msc313_mpll_driver = {
    > + .driver = {
    > + .name = "mstar-msc313-mpll",
    > + .of_match_table = msc313_mpll_of_match,
    > + },
    > + .probe = msc313_mpll_probe,
    > + .remove = msc313_mpll_remove,
    > +};
    > +builtin_platform_driver(msc313_mpll_driver);

    \
     
     \ /
      Last update: 2020-12-20 05:41    [W:3.615 / U:0.612 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site