On 20/10/2025 15:23, Tom Rini wrote:
On Mon, Oct 20, 2025 at 01:03:15PM +0100, Andrew Goodbody wrote:
On 19/10/2025 21:28, Jonas Karlman wrote:
Hi Andrew,
On 10/15/2025 4:32 PM, Andrew Goodbody wrote:
clk_get_rate() returns a ulong so do not attempt to pass negative error
codes through it.
On Rockchip the clk drivers only implement support for a subset of all
clocks supported by the hardware, mostly to save space for TPL/SPL and
not having to implement logic for clocks not really needed by U-Boot.
Because support for all clocks is not implemented the existing working
error handling actually help us catch clock issues on the Rockchip
platform. E.g. when a clk is referenced by assigned-clock-rates in DT we
get an error and can fix the issue, or decide if we can safely ignore
setting the clk rate.
I do not understand why you need to change the way the clk ops work,
just check with IS_ERR_VALUE() inside the clk-uclass or similar if you
really must remove working error handling from the callers.
OK, so here we have a question and it is one that I wish had come up in the
original discussion. IS_ERR_VALUE() and other related macros are from
include/linux/err.h and are introduced with this comment
/*
* Kernel pointers have redundant information, so we can use a
* scheme where we can return either an error code or a dentry
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
So they are clearly intended to be unambiguous and used to add information
about errors into kernel pointers. It also suggests that there may be other
implementations of these macros for other architectures.
Much of the error checking of the return value from clk_get_rate() is done
with IS_ERR_VALUE() despite the return value not being a kernel pointer and
with this default implementation it does happen to work just as long as your
clock rate does not collide with an error. OK collisions are very unlikely
but it does still feel like an abuse of the macro to me.
Is there any official guidance on this use of IS_ERR_VALUE()?
We don't have any, really. So my question is, given the use case Jonas
outlined (thanks!), what do you see as a good way to handle this?
I would hesitate to call it good as it still seems wrong to me to pass a
negative error as an unsigned return value, but given the push back I
guess the pragmatic thing to do here is accept the use of IS_ERR_VALUE()
and that clk_get_rate() will be limited to < (ULONG_MAX - 4095). We can
drop the current series and I will take another look over the code to
see where there still may be needed some fixes.
Andrew
