Re: [alsa-devel] [PATCH v2 5/7] ASoC: Intel: Skylake: Add ssp clock driver

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?

+#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 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.

+{

• 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.

• 	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.

• 	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.