Quoting Sivaprakash Murugesan (2020-02-27 01:55:18) > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index 15cdcdc..37e4ce2 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -89,6 +89,14 @@ config APQ_MMCC_8084 > Say Y if you want to support multimedia devices such as display, > graphics, video encode/decode, camera, etc. > > +config IPQ_APSS_6018 > + tristate "IPQ6018 APSS Clock Controller" > + select IPQ_GCC_6018 > + help > + Support for APSS clock controller on ipq6018 devices. The > + APSS clock controller supports frequencies higher than 800Mhz. supports CPU frequencies? It's not clear what APSS is to a lot of people out there. > + Say Y if you want to support higher frequencies on ipq6018 devices. support CPU frequency scaling on ipq6018? > + > config IPQ_GCC_4019 > tristate "IPQ4019 Global Clock Controller" > help > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > new file mode 100644 > index 0000000..04b8962 > --- /dev/null > +++ b/drivers/clk/qcom/apss-ipq6018.c > @@ -0,0 +1,210 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/kernel.h> > +#include <linux/err.h> > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> Are these two includes needed at all? > +#include <linux/clk-provider.h> > +#include <linux/regmap.h> > + > +#include <linux/reset-controller.h> > +#include <dt-bindings/clock/qcom,apss-ipq6018.h> > + > +#include "common.h" > +#include "clk-regmap.h" > +#include "clk-pll.h" > +#include "clk-rcg.h" > +#include "clk-branch.h" > +#include "clk-alpha-pll.h" > +#include "clk-regmap-divider.h" > +#include "clk-regmap-mux.h" > +#include "reset.h" > + > +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) } This can be removed. It's common in clk-rcg.h now > + > +static struct clk_branch apcs_alias0_core_clk = { > + .halt_reg = 0x004c, > + .clkr = { > + .enable_reg = 0x004c, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "apcs_alias0_core_clk", > + .parent_hws = (const struct clk_hw *[]){ > + &apcs_alias0_clk_src.clkr.hw }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, Please add a comment about why CLK_IS_CRITICAL is here. Presumably in the case that a cpufreq driver doesn't probe and claim this clk? > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > + [...] > + > +static int __init apss_ipq6018_init(void) > +{ > + return platform_driver_register(&apss_ipq6018_driver); > +} > +core_initcall(apss_ipq6018_init); > + > +static void __exit apss_ipq6018_exit(void) > +{ > + platform_driver_unregister(&apss_ipq6018_driver); > +} > +module_exit(apss_ipq6018_exit); Any reason this can't just be module_platform_driver()?