On 2/26/22 1:36 PM, Simon Glass wrote:
Hi Sean,
On Tue, 1 Feb 2022 at 21:24, Sean Anderson <sean...@gmail.com> wrote:
On 2/1/22 10:59 PM, Simon Glass wrote:
Hi Sean,
On Tue, 1 Feb 2022 at 07:49, Sean Anderson <sean...@gmail.com> wrote:
On 1/27/22 4:35 PM, Simon Glass wrote:
Hi Sean,
On Thu, 27 Jan 2022 at 08:43, Sean Anderson <sean...@gmail.com> wrote:
On 1/27/22 10:05 AM, Simon Glass wrote:
Hi Sean,
On Sat, 15 Jan 2022 at 15:25, Sean Anderson <sean...@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 <sean...@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.
What does this all look like in Linux?
Their equivalent (clk_put) returns void, and generally so do most other
cleanup functions, since .device_remove also returns void.
We really cannot ignore errors from device_remove().
Once you are at device_remove, all the users are gone and it's up to the
device to clean up after itself. And often there is nothing we can do
once remove is called. As you yourself say in device_remove,
/* We can't put the children back */
Well this assumes that device_remove() is actually removing the
device, not just disabling DMA, etc.
Really the only sensible thing is to print an error and continue booting
if possible.
And of course no clock drivers actually use this function anyway, nor do
(all but 5) users check it.
Anyway I think what you say about the 'thick pointer' makes sense. But
my expectation was that removing a clock might turn off a clock above
it in the tree, for example.
No, this just frees resources (as is documented). If you want to turn
off a clock, you have to call clk_disable. In fact, a very common use
case is just like the example above, where the consmer frees the clock
after enabling it.
(This is also why clk->enable_count/rate are basically useless for
anything other than CCF clocks)
How about a clock provided by an audio codec on an I2C bus? Should
clk_free() do anything in that case? I assume not. I think the
compelling part of your argument is that it is a 'think pointer' and
disable does nothing. So can you update clk_rfree() etc. to document
what is allowed to be done in that function?
The ideal case would be if you wanted to allocate some per-struct-clk
data. Then, the correct place to free it would be rfree. But no one
does this, and if they did it would probably be better to free things
in remove.
Actually... no one in clk, reset, or power-domain does anything with
rfree. So I am inclined to just remove it altogether.
--Sean
Reviewed-by: Simon Glass <s...@chromium.org>
Regards,
Simon
