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@xxxxxxxxx>
> + * Author: Subhransu S. Prusty <subhransu.s.prusty@xxxxxxxxx>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * 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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
