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

[Andrew Goodbody]

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