Re: Seeking advice on API return type inconsistency

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 12.08.25 16: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?
> 

Using macro IS_ERR_VALUE() in the caller should do the needed.

See include/linux/err.h:21:
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

Best regards

Heinrich