[PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

On 1/27/22 10:05 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2 at gmail.com> wrote:
>>
>> When freeing a clock there is not much we can do if there is an error, and
>> most callers do not actually check the return value. Even e.g. checking to
>> make sure that clk->id is valid should have been done in request() in the
>> first place (unless someone is messing with the driver behind our back).
>> Just return void and don't bother returning an error.
>>
>> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
>> ---
>>
>>   drivers/clk/clk-uclass.c  | 7 +++----
>>   drivers/clk/clk_sandbox.c | 6 +++---
>>   include/clk-uclass.h      | 8 +++-----
>>   3 files changed, 9 insertions(+), 12 deletions(-)
>>
> 
> We have the same thing in other places too, but I am a little worried
> about removing error checking. We try to avoid checking arguments too
> much in U-Boot, due to code-size concerns, so I suppose I agree that
> an invalid clk should be caught by a debug assertion rather than a
> full check. But with driver model we have generally added an error
> return to every uclass method, for consistency and to permit returning
> error information if needed.
> 
> Regards,
> Simon
> 

So there are a few reasons why I don't think a return value is useful
here. To illustrate this, consider a typical user of the clock API:

	struct clk a, b;

	ret = clk_get_by_name(dev, "a", &a);
	if (ret)
		return ret;

	ret = clk_get_by_name(dev, "b", &b);
	if (ret)
		goto free_a;

	ret = clk_set_rate(&a, 5000000);
	if (ret)
		goto free_b;

	ret = clk_enable(&b);

free_b:
	clk_free(&b);
free_a:
	clk_free(&a);
	return ret;

- Because a and b are "thick pointers" they do not need any cleanup to
   free their own resources. The only cleanup might be if the clock
   driver has allocated something in clk_request (more on this below)
- By the time we call clk_free, the mutable portions of the function
   have already completed. In effect, the function has succeeded,
   regardless of whether clk_free fails. Additionally, we cannot take any
   action if it fails, since we still have to free both clocks.
- clk_free occurs during the error path of the function. Even if it
   errored, we do not want to override the existing error from one of the
   functions doing "real" work.

The last thing is that no clock driver actually does anything in rfree.
The only driver with this function is the sandbox driver. I would like
to remove the function altogether. As I understand it, the existing API
is inspired by the reset drivers, so I would like to review its usage in
the reset subsystem before removing it for the clock subsystem. I also
want to make some changes to how rates and enables/disables are
calculated which might provide a case for rfree. But once that is
complete I think there will be no users still.

--Sean