Re: [RFC 5/5] clk: Add floor and ceiling constraints to clock rates

[Tomeu Vizoso <tomeu.vizoso@collabora.com>]

On 06/28/2014 12:57 AM, Stephen Warren wrote:
> On 06/27/2014 01:57 AM, Tomeu Vizoso wrote:
>> Adds a way for clock consumers to set maximum and minimum rates. This can be
>> used for thermal drivers to set ceiling rates, or by misc. drivers to set
>> floor rates to assure a minimum performance level.
>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>
>> +static struct rate_constraint *__ensure_constraint(struct clk *clk_user,
>> +						   enum constraint_type type)
>
>> +	if (!found) {
>> +		constraint = kzalloc(sizeof(*constraint), GFP_KERNEL);
>> +		if (!constraint) {
>> +			pr_err("%s: could not allocate constraint\n", __func__);
>
> Doesn't kzalloc print an error itself if the allocation fails? I've
> certainly seen quite a few patches ripping out custom "allocation
> failed" errors in code.

Thanks for pointing this out, have fixed it in a new version that will 
be sent soon.

>> +void __clk_free_clk(struct clk *clk_user)
>> +{
>> +	struct clk_core *clk = clk_to_clk_core(clk_user);
>> +	struct rate_constraint *constraint;
>> +	struct hlist_node *tmp;
>> +
>> +	hlist_for_each_entry_safe(constraint, tmp, &clk->rate_constraints, node) {
>> +		if (constraint->dev_id == clk_user->dev_id &&
>> +		    constraint->con_id == clk_user->con_id) {
>> +			hlist_del(&constraint->node);
>> +			kfree(constraint);
>
> Perhaps the list of constraints should be indexed by the client clk
> structure, so that test should be:
>
> if (constraint->clk_user == clk_user)
>
> It might be a bit more work, but perhaps the constraints should simply
> be stored directly in the struct clk rater than the struct clk_core.
> That would require a nested loop to apply constraints though; first over
> each struct clk associated with a struct clk_core, then over each
> constraints in that struct clk. It would slightly simplify
> adding/removing constraints though, and store the constraints at their
> "source".

Yeah, I like this alternative from a code organization point of view, 
but I found the increased code complexity hard to justify. Wonder if 
anybody else has an opinion on this.

>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>
>> +int clk_set_floor_rate(struct clk *clk, unsigned long rate);
>
>> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
>
> Additions functions to explicitly remove any previously requested
> floor/ceiling rate might be useful. The same effect could be achieved by
> a floor of 0 or a very high ceiling, but it feels cleaner to remove them.

I'm also on the fence on whether the improved readability justifies the 
additional API, because I find convenient that one just has to grep for 
a single function call to find all usages (it was certainly useful when 
I looked at how downstream does the equivalent). I'll be glad to add the 
functions in a future version if there's consensus on this.

> Overall, this series seems to implement the right kind of concept to me.
> It'll certainly stop us (NVIDIA at least) wanting to create all kinds of
> "virtual" clock objects (and associated clock IDs and device tree clock
> IDs) to achieve a similar effect.

Thanks for the great feedback,

Tomeu