Quoting Maxime Ripard (2021-02-25 07:59:02)
> Some devices might need to access the current available range of a clock
> to discover their capabilities. Let's add those accessors.
This needs more than two sentences to describe what's required.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
> drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++
> include/linux/clk.h | 16 ++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8c1d04db990d..b7307d4f090d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
> }
> EXPORT_SYMBOL_GPL(clk_set_max_rate);
>
> +long clk_get_min_rate(struct clk *clk)
I need to read the rest of the patches but I don't see the justification
for this sort of API vs. having the consumer constrain the clk frequency
that it wants. Is the code that's setting the min/max constraints not
the same as the code that's calling this API? Would an OPP table better
serve this so the device knows what frequencies are valid?s Please
provide the use case/justification in the commit text.
Why two functions instead of one function to get both min and max?
> +{
> + unsigned long min_rate, max_rate;
> +
> + if (!clk)
> + return 0;
> +
> + clk_prepare_lock();
Please add a comment indicating why we grab the lock. Yes
clk_core_get_boundaries() has a lock held assertion, but I don't know
why we care about the lock here besides that we don't want the consumers
to change while we calculate the boundaries as it may be some inaccurate
number.
> + clk_core_get_boundaries(clk->core, &min_rate, &max_rate);
> + clk_prepare_unlock();
> +
> + return min_rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_min_rate);
> +
> +long clk_get_max_rate(struct clk *clk)
> +{
> + unsigned long min_rate, max_rate;
> +
> + if (!clk)
> + return 0;
ULONG_MAX?
> +
> + clk_prepare_lock();
> + clk_core_get_boundaries(clk->core, &min_rate, &max_rate);
> + clk_prepare_unlock();
> +
> + return max_rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_max_rate);
> +
> /**
> * clk_get_parent - return the parent of a clk
> * @clk: the clk whose parent gets returned
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 31ff1bf1b79f..6f0c00ddf3ac 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -709,6 +709,22 @@ int clk_set_min_rate(struct clk *clk, unsigned long rate);
> */
> int clk_set_max_rate(struct clk *clk, unsigned long rate);
>
> +/**
> + * clk_get_min_rate - get the minimum clock rate for a clock source
> + * @clk: clock source
> + *
> + * Returns the minimum rate or negative errno.
Hmm?
> + */
> +long clk_get_min_rate(struct clk *clk);
Why long instead of unsigned long? Error values don't seem to be
returned.
> +
> +/**
> + * clk_get_max_rate - get the maximum clock rate for a clock source
> + * @clk: clock source
> + *
> + * Returns the maximum rate or negative errno.
> + */
> +long clk_get_max_rate(struct clk *clk);
> +
> /**
> * clk_set_parent - set the parent clock source for this clock
> * @clk: clock source
> --
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
