On Tue, Oct 24, 2017 at 07:33:12AM -0700, Stephen Boyd wrote:
On 09/18, Subhransu S. Prusty wrote:
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c new file mode 100644 index 000000000000..769ece306f58 --- /dev/null +++ b/sound/soc/intel/skylake/skl-ssp-clk.c @@ -0,0 +1,288 @@ +/*
- skl-ssp-clk.c - ASoC skylake ssp clock driver
- Copyright (C) 2017 Intel Corp
- Author: Jaikrishna Nemallapudi jaikrishnax.nemallapudi@intel.com
- Author: Subhransu S. Prusty subhransu.s.prusty@intel.com
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; version 2 of the License.
- This program is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- General Public License for more details.
- */
+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/clk.h>
Is this include used?
It is not used. Will remove.
+#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include "skl-ssp-clk.h"
+#define to_skl_clk(_hw) container_of(_hw, struct skl_clk, hw)
+struct skl_clk_parent {
- struct clk_hw *hw;
- struct clk_lookup *lookup;
+};
+struct skl_clk {
- struct clk_hw hw;
- struct clk_lookup *lookup;
- struct skl_clk_ops *ops;
- unsigned long rate;
- void *pvt_data;
- u32 id;
+};
+struct skl_clk_data {
- struct skl_clk_parent parent[SKL_MAX_CLK_SRC];
- struct skl_clk *clk[SKL_MAX_CLK_CNT];
- u8 avail_clk_cnt;
+};
+static int skl_clk_prepare(struct clk_hw *hw) +{
- struct skl_clk *clkdev = to_skl_clk(hw);
- if (!clkdev->ops || !clkdev->ops->prepare)
return -EIO;Ok is this the "virtual" part? Because it is sort of odd. Why can't we give clk ops directly for everything and get rid of struct skl_clk_ops here? Bouncing through this layer must be
Yes makes sense. I think we can remove the wrappers and move the code here which sends the IPC to enable the clocks. Will work on that for v3.
because something isn't converted to CCF, but what is that?
- if (!clkdev->rate)
return -EINVAL;- return clkdev->ops->prepare(clkdev->pvt_data, clkdev->id, clkdev->rate);
+}
+static void skl_clk_unprepare(struct clk_hw *hw) +{
- struct skl_clk *clkdev = to_skl_clk(hw);
- if (!clkdev->ops || !clkdev->ops->unprepare)
return;- if (!clkdev->rate)
return;- clkdev->ops->unprepare(clkdev->pvt_data, clkdev->id, clkdev->rate);
+}
+static int skl_clk_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate)+{
- struct skl_clk *clkdev = to_skl_clk(hw);
- int ret;
- if (!clkdev->ops || !clkdev->ops->set_rate)
return -EIO;- ret = clkdev->ops->set_rate(clkdev->id, rate);
- if (!ret)
clkdev->rate = rate;- return ret;
+}
+unsigned long skl_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) +{
- struct skl_clk *clkdev = to_skl_clk(hw);
- if (clkdev->rate)
return clkdev->rate;- if (!clkdev->ops || !clkdev->ops->recalc_rate)
return -EIO;- clkdev->rate = clkdev->ops->recalc_rate(clkdev->id, parent_rate);
- return clkdev->rate;
+}
+/* Not supported by clk driver. Implemented to satisfy clk fw */ +long skl_clk_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *parent_rate)+{
- return rate;
+}
+static const struct clk_ops skl_clk_ops = {
- .prepare = skl_clk_prepare,
- .unprepare = skl_clk_unprepare,
- .set_rate = skl_clk_set_rate,
- .round_rate = skl_clk_round_rate,
- .recalc_rate = skl_clk_recalc_rate,
+};
+static void unregister_parent_src_clk(struct skl_clk_parent *pclk, u8 id)
Why is id a u8? Would be simpler as unsigned. Usually I think of registers when using u8/16/32/64, not array sizes.
Will fix this.
+{
- while (id--) {
clkdev_drop(pclk[id].lookup);clk_hw_unregister_fixed_rate(pclk[id].hw);- }
+}
+static void unregister_src_clk(struct skl_clk_data *dclk) +{
- u8 cnt = dclk->avail_clk_cnt;
- while (cnt--)
clkdev_drop(dclk->clk[cnt]->lookup);+}
+static int skl_register_parent_clks(struct device *dev,
struct skl_clk_parent *parent,struct skl_clk_parent_src *pclk)+{
- int i, ret;
- for (i = 0; i < SKL_MAX_CLK_SRC; i++) {
/* Register Parent clock */parent[i].hw = clk_hw_register_fixed_rate(dev, pclk[i].name,pclk[i].parent_name, 0, pclk[i].rate);if (IS_ERR(parent[i].hw)) {ret = PTR_ERR_OR_ZERO(parent[i].hw);If it's an IS_ERR then we just need PTR_ERR.
Yes it should be PTR_ERR only. Will fix it.
goto err;}parent[i].lookup = clkdev_hw_create(parent[i].hw, pclk[i].name,NULL);if (!parent[i].lookup) {clk_hw_unregister_fixed_rate(parent[i].hw);ret = PTR_ERR_OR_ZERO(parent[i].lookup);If it's not NULL then we always unregister parent and return some random number? Maybe I'm missing something.
You are right. Will fix this.
goto err;}- }
- return 0;
+err:
- unregister_parent_src_clk(parent, i);
- return ret;
+}
[...]
- return 0;
+err_unreg_skl_clk:
- unregister_src_clk(data);
- unregister_parent_src_clk(data->parent, SKL_MAX_CLK_SRC);
- return ret;
+}
+static int skl_clk_dev_remove(struct platform_device *pdev) +{
- struct skl_clk_data *data;
- data = platform_get_drvdata(pdev);
- unregister_parent_src_clk(data->parent, SKL_MAX_CLK_SRC);
- unregister_src_clk(data);
This is opposite path of error path in probe, so something smells wrong.
Yes this sequence is wrong. Will fix this as well.
Regards, Subhransu
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--