Message-ID: <542D953F.3030701@codeaurora.org>
Date:	Thu, 02 Oct 2014 11:11:11 -0700
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Georgi Djakov <gdjakov@...sol.com>
CC:	mturquette@...aro.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v1] clk: qcom: Add support for regmap clock dividers

On 09/30/14 10:20, Georgi Djakov wrote:
> This patch expands the regmap support to allow registration of clock
> dividers. It just prepares for the introduction of a clkdiv driver,
> that will be in a separate patch.
> Such dividers are found in the Qualcomm PMIC chips such as PM8941,
> PMA8084 and others.

We're going to need to rework the Makefile in clk/qcom so that we only
build certain pieces of the "library" when we need them. Right now the
directory is focused entirely on mmio clock controllers and if we put
the pmic clocks in there then we need to figure out a way to only build
the pmic pieces if only the pmic driver is selected and only build the
mmio pieces if the mmio drivers are selected.

>
> Signed-off-by: Georgi Djakov <gdjakov@...sol.com>
> ---
>  drivers/clk/qcom/clk-regmap.c |   68 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap.h |   24 +++++++++++++++
>  2 files changed, 92 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-regmap.c b/drivers/clk/qcom/clk-regmap.c
> index a58ba39..d63b8ca 100644
> --- a/drivers/clk/qcom/clk-regmap.c
> +++ b/drivers/clk/qcom/clk-regmap.c
> @@ -112,3 +112,71 @@ struct clk *devm_clk_register_regmap(struct device *dev,
>  	return devm_clk_register(dev, &rclk->hw);
>  }
>  EXPORT_SYMBOL_GPL(devm_clk_register_regmap);
> +
> +/**
> + * clkdiv_recalc_rate_regmap - Calculate clock divider output rate

Arguments should be documented?
> + */
> +unsigned long clkdiv_recalc_rate_regmap(struct clk_hw *hw,

Or this should be static.

> +					unsigned long parent_rate)
> +{
> +	struct clk_regmap *rclk = to_clk_regmap(hw);
> +	struct clkdiv_regmap *clkdiv = to_clkdiv_regmap(rclk);
> +	unsigned int div, val;
> +
> +	regmap_read(rclk->regmap, clkdiv->reg, &val);
> +	if (!val)
> +		return parent_rate;
> +
> +	div = (val >> clkdiv->shift) & ((1 << clkdiv->width)-1);
> +
> +	return parent_rate / div;

I don't know if you saw the patch to split out the clk-divider.c logic
from the readl/writel patch I sent[1]? That could make this slightly
smaller.
tabl
> +}
> +EXPORT_SYMBOL_GPL(clkdiv_recalc_rate_regmap);
> +
> +static const struct clk_ops clkdiv_ops = {
> +	.enable = clk_enable_regmap,
> +	.disable = clk_disable_regmap,

This isn't exactly a divider if it also has enable/disable control. At
which point this starts to look like a composite clock. Perhaps call
this clk_div_gate_ops?

> +	.is_enabled = clk_is_enabled_regmap,
> +	.recalc_rate = clkdiv_recalc_rate_regmap,

No .set_rate? So call it clk_fixed_div_gate_ops?

> +};
> +EXPORT_SYMBOL_GPL(clkdiv_ops);
> +
> +/**
> + * devm_clkdiv_register_regmap - register a clk_regmap clock divider
> + *
> + * @dev: device that is registering this clock
> + * @name: name of this clock
> + * @parent_name: name of clock's parent
> + * @flags: framework-specific flags
> + * @clkdiv: clock divider to operate on
> + *
> + * Clocks that use regmap for their register I/O should register their
> + * clk_regmap struct via this function so that the regmap is initialized
> + * and so that the clock is registered with the common clock framework.
> + */
> +struct clk *devm_clkdiv_register_regmap(struct device *dev, const char *name,
> +					const char *parent_name,
> +					unsigned long flags,
> +					struct clkdiv_regmap *clkdiv)
> +{
> +	struct clk_init_data init;
> +
> +	if (!clkdiv)
> +		return ERR_PTR(-ENODEV);

Looks like a useless check. Just blow up instead so we don't have to
tolerate bad code.

> +
> +	clkdiv->rclk.regmap = dev_get_regmap(dev->parent, NULL);
> +
> +	if (!clkdiv->rclk.regmap)
> +		return ERR_PTR(-ENXIO);
> +
> +	init.name = name;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +	init.flags = flags;
> +	init.ops = &clkdiv_ops;
> +
> +	clkdiv->rclk.hw.init = &init;
> +
> +	return devm_clk_register(dev, &clkdiv->rclk.hw);
> +}
> +EXPORT_SYMBOL_GPL(devm_clkdiv_register_regmap);
> diff --git a/drivers/clk/qcom/clk-regmap.h b/drivers/clk/qcom/clk-regmap.h
> index 491a63d..02582cf 100644
> --- a/drivers/clk/qcom/clk-regmap.h
> +++ b/drivers/clk/qcom/clk-regmap.h
> @@ -42,4 +42,28 @@ void clk_disable_regmap(struct clk_hw *hw);
>  struct clk *
>  devm_clk_register_regmap(struct device *dev, struct clk_regmap *rclk);
>  
> +/**
> + * struct clkdiv_regmap - regmap supporting clock divider
> + * @rclk:   regmap supporting clock struct
> + * @reg:    offset into regmap for the control register
> + * @shift:  bit position for divider value
> + * @width:  number of bits used for divider value
> + * @table:  pointer to table containing an array of divider/value pairs
> + */
> +struct clkdiv_regmap {
> +	struct clk_regmap rclk;
> +	unsigned int reg;
> +	unsigned int shift;
> +	unsigned int width;
> +	struct clk_div_table *table;

Is this used?

> +};
> +
> +#define to_clkdiv_regmap(_rclk) container_of(_rclk, struct clkdiv_regmap, rclk)
> +
> +unsigned long
> +clkdiv_recalc_rate_regmap(struct clk_hw *hw, unsigned long parent_rate);
> +struct clk *
> +devm_clkdiv_register_regmap(struct device *dev, const char *name,
> +			    const char *parent_name, unsigned long flags,
> +			    struct clkdiv_regmap *clkdiv);
>  #endif


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists