[PATCH 08/24] clk: rockchip: Remove negative error returns from clk_get_rate

[Jonas Karlman]

Hi Andrew,

On 10/20/2025 2:03 PM, 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

I have no idea what the original discussion this relates to.

In U-Boot most/all? uclass functions is documented to return >= 0 on
success and -ve on error, I do not really see a reason why clk funcs
should be changed away from this documented behavior.

> 
> /*
>   * 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.

The U-Boot documentation for clk_get_rate() clearly state that we should
use -ve error code for errors. And the safest way to test for error
codes will be to use something like IS_ERR_VALUE(), so why should we not use it as
it does the job very well?

Also for Rockchip the clock rates do not really collide with error codes
as the typical max clock rate go up to 2-3 GHz, and in U-Boot we
typically only reference clocks with less than 1 GHz rates.

/**
 * clk_get_rate() - Get current clock rate.
 * @clk:	A clock struct that was previously successfully requested by
 *		clk_request/get_by_*().
 *
 * Return: clock rate in Hz on success, 0 for invalid clock, or -ve error code
 *	   for other errors.
 */

To me this patch/series just look to break this documented and working
error handling without any justified benefit or replacement.

Regards,
Jonas

> 
> Is there any official guidance on this use of IS_ERR_VALUE()?
> 
> Andrew
> 
>> The current working clk error handling has really helped me time and
>> time again on Rockchip platform during soc and board bring-up.
>>
>> This is a NAK for me without first having a proper replacement for all
>> existing working error handling that is just being left ignored after
>> this patch/series.
>>
>> Regards,
>> Jonas