Seeking advice on API return type inconsistency

[Andrew Goodbody]

On 12/08/2025 16:03, Tom Rini wrote:
> On Tue, Aug 12, 2025 at 03:46:56PM +0100, Andrew Goodbody wrote:
>> On 12/08/2025 15:33, Tom Rini wrote:
>>> On Tue, Aug 12, 2025 at 10:17:47AM +0100, Andrew Goodbody wrote:
>>>> On 11/08/2025 17:36, Quentin Schulz wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> On 8/11/25 5:24 PM, Andrew Goodbody wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I was wondering what people's thoughts were on API return types. In
>>>>>> particular there is this and other examples in include/clk-uclass.h
>>>>>>
>>>>>> /**
>>>>>>     * get_rate() - Get current clock rate.
>>>>>>     * @clk:    The clock to query.
>>>>>>     *
>>>>>>     * This returns the current rate of a clock. If the clock is
>>>>>> disabled, it
>>>>>>     * returns the rate at which the clock would run if it was enabled. The
>>>>>>     * following pseudo-code should hold::
>>>>>>     *
>>>>>>     *   disable(clk)
>>>>>>     *   rate = get_rate(clk)
>>>>>>     *   enable(clk)
>>>>>>     *   assert(get_rate(clk) == rate)
>>>>>>     *
>>>>>>     * Return:
>>>>>>     * * The rate of @clk
>>>>>>     * * -%ENOSYS if this function is not implemented for @clk
>>>>>>     * * -%ENOENT if @clk->id is invalid. Prefer using an assert
>>>>>> instead, and doing
>>>>>>     *   this check in request().
>>>>>>     * * Another negative error value (such as %EIO or %ECOMM) if the
>>>>>> rate could
>>>>>>     *   not be determined due to a bus error.
>>>>>>     */
>>>>>> ulong get_rate(struct clk *clk);
>>>>>>
>>>>>>
>>>>>> get_rate is declared as returning a ulong but the description says
>>>>>> that it can return negative errors. A simple test of the return
>>>>>> value for being less than 0 will always fail so errors can go
>>>>>> undetected. Casting to a signed type seems less than ideal.
>>>>>>
>>>>>> What is the best way to deal with this? Cast to a signed or update
>>>>>> the API to be signed or...?
>>>>>>
>>>>>
>>>>> Note that clk_get_rate() in the kernel has the same function signature
>>>>> so I would refrain from changing the type otherwise we'll have some
>>>>> "funny" bugs to handle considering it isn't that uncommon to import
>>>>> drivers almost as-is from the Linux kernel.
>>>>
>>>> Ah yes. The difference being that the kernel does not seem to attempt to
>>>> push an error code through this API, you get a rate or you get 0.
>>>
>>> How is the error code pushed? Or is it up to the caller to decide that 0
>>> means on a case by case basis?
>>
>> In the Linux kernel almost no code checks the return of clk_get_rate at all.
>> Some code will check the value is sensible and 0 is obviously not sensible.
>> But pretty much the call to clk_get_rate is not expected to fail.
> 
> Perhaps getting lost in the specifics then, but perhaps for this case we
> should just do the same? But your question was more general, so another
> example might help.

I suspect that the answer is always going to that it depends on the 
specifics of each case. The U-Boot implementation of clk_get_rate seems 
to have become more complicated leading to the addition of returning 
error codes. This leads to the question about what level of 
compatibility should there be maintained with the kernel? That addition 
of returning error codes in U-Boot already means that the API is no 
longer compatible with that of the kernel.

Another example is a patch I just submitted [1] where the called 
function is a static so a reasonable candidate for just changing the 
signature maybe? But in this case it is only possible for it to return a 
single error code so the choice I made was to alter the test from '< 0' 
to '== -EBUSY' which I made on the basis of keeping to as small a change 
as is reasonable. If however the called function was able to return two 
or more different error codes then I would likely have opted to changing 
the signature to return a signed type as long as I was confident that 
overflow was not possible.

Andrew

1) 
https://patchwork.ozlabs.org/project/uboot/patch/20250813-tpm_tis_infineon-v1-1-c95434a98efd@linaro.org/