Seeking advice on API return type inconsistency

Seeking advice on API return type inconsistency

[Andrew Goodbody]

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...?

Andrew

Re: Seeking advice on API return type inconsistency

[Quentin Schulz]

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.

You have get_rate, clk_get_rate and clk_ops.get_rate that should I 
believe share the same signature?

This for sure isn't helping you but I also couldn't come up with 
something the 5min I thought about it :/

Cheers,
Quentin

Re: Seeking advice on API return type inconsistency

[Andrew Goodbody]

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.

> You have get_rate, clk_get_rate and clk_ops.get_rate that should I 
> believe share the same signature?
> 
> This for sure isn't helping you but I also couldn't come up with 
> something the 5min I thought about it :/

Thanks for looking anyway.

Andrew

> Cheers,
> Quentin

Re: Seeking advice on API return type inconsistency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Seeking advice on API return type inconsistency

[Heinrich Schuchardt]

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

Re: Seeking advice on API return type inconsistency

[Andrew Goodbody]

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.

Andrew

Re: Seeking advice on API return type inconsistency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: 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/

Re: Seeking advice on API return type inconsistency

[Michal Suchánek]

Hello,

On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
> 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?

Presumably it returns -EBUSY which is then implicitly converted to a large
positive value. For pointers there is IS_ERR macro to decode that, and
for integers there is another corresponding macro (I don't recall the
name).

For memory this assumes that the last page that holds the 'error'
pointers cannot be mapped, and there was already a bug in the kernel
that this was not true.

For clock rates this assumes that rates close to the maximum value are
invalid which is not guaranteed in any way whatsoever.

Consequently, this should not be used.

0 is the only clock rate guaranteed to be invalid.

As far as I have seen in the u-boot code almost no caller checks the
result, and those that do cannot do anything meaningful with it anyway,
at most a message is printed. That can be done by the driver without
passing around the error.

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

I would suggest to abolish returning 'errors' as clock rates.

Thanks

Michal

Re: Seeking advice on API return type inconsistency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5266 bytes --]

On Wed, Aug 13, 2025 at 01:41:21PM +0200, Michal Suchánek wrote:
> Hello,
> 
> On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
> > 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?
> 
> Presumably it returns -EBUSY which is then implicitly converted to a large
> positive value. For pointers there is IS_ERR macro to decode that, and
> for integers there is another corresponding macro (I don't recall the
> name).
> 
> For memory this assumes that the last page that holds the 'error'
> pointers cannot be mapped, and there was already a bug in the kernel
> that this was not true.
> 
> For clock rates this assumes that rates close to the maximum value are
> invalid which is not guaranteed in any way whatsoever.
> 
> Consequently, this should not be used.
> 
> 0 is the only clock rate guaranteed to be invalid.
> 
> As far as I have seen in the u-boot code almost no caller checks the
> result, and those that do cannot do anything meaningful with it anyway,
> at most a message is printed. That can be done by the driver without
> passing around the error.
> 
> > > > 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.
> 
> I would suggest to abolish returning 'errors' as clock rates.

Yes, I think this is a case where we should change our internal API,
both for easier driver migrations from the linux kernel and also because
it seems our design here can't quite work as intended.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Seeking advice on API return type inconsistency

[Andrew Goodbody]

On 13/08/2025 16:40, Tom Rini wrote:
> On Wed, Aug 13, 2025 at 01:41:21PM +0200, Michal Suchánek wrote:
>> Hello,
>>
>> On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
>>> 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?
>>
>> Presumably it returns -EBUSY which is then implicitly converted to a large
>> positive value. For pointers there is IS_ERR macro to decode that, and
>> for integers there is another corresponding macro (I don't recall the
>> name).
>>
>> For memory this assumes that the last page that holds the 'error'
>> pointers cannot be mapped, and there was already a bug in the kernel
>> that this was not true.
>>
>> For clock rates this assumes that rates close to the maximum value are
>> invalid which is not guaranteed in any way whatsoever.
>>
>> Consequently, this should not be used.
>>
>> 0 is the only clock rate guaranteed to be invalid.
>>
>> As far as I have seen in the u-boot code almost no caller checks the
>> result, and those that do cannot do anything meaningful with it anyway,
>> at most a message is printed. That can be done by the driver without
>> passing around the error.
>>
>>>>> 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.
>>
>> I would suggest to abolish returning 'errors' as clock rates.
> 
> Yes, I think this is a case where we should change our internal API,
> both for easier driver migrations from the linux kernel and also because
> it seems our design here can't quite work as intended.

OK, I will see if I can put a patch series together.

Andrew

Re: Seeking advice on API return type inconsistency

[Andrew Goodbody]

On 13/08/2025 16:40, Tom Rini wrote:
> On Wed, Aug 13, 2025 at 01:41:21PM +0200, Michal Suchánek wrote:
>> Hello,
>>
>> On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
>>> 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?
>>
>> Presumably it returns -EBUSY which is then implicitly converted to a large
>> positive value. For pointers there is IS_ERR macro to decode that, and
>> for integers there is another corresponding macro (I don't recall the
>> name).
>>
>> For memory this assumes that the last page that holds the 'error'
>> pointers cannot be mapped, and there was already a bug in the kernel
>> that this was not true.
>>
>> For clock rates this assumes that rates close to the maximum value are
>> invalid which is not guaranteed in any way whatsoever.
>>
>> Consequently, this should not be used.
>>
>> 0 is the only clock rate guaranteed to be invalid.
>>
>> As far as I have seen in the u-boot code almost no caller checks the
>> result, and those that do cannot do anything meaningful with it anyway,
>> at most a message is printed. That can be done by the driver without
>> passing around the error.
>>
>>>>> 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.
>>
>> I would suggest to abolish returning 'errors' as clock rates.
> 
> Yes, I think this is a case where we should change our internal API,
> both for easier driver migrations from the linux kernel and also because
> it seems our design here can't quite work as intended.

I have prepared two series of patches to address this. The first series 
will remove all attempts to return negative error codes through the 
.get_rate element of struct clk_ops. This is called from clk_get_rate in 
clk-uclass.c.
The second series updates clk_get_rate itself to remove negative error 
returns and also all the calling sites to correct error detection.

Unfortunately both series affect a moderate number of files although 
each individual changes is minor.  This also leads to a larger than 
normal Cc: list. I hope it will not cause issues with sending the email.

I will send the first series soon.

Andrew

Re: Seeking advice on API return type inconsistency

[Andrew Goodbody]

That 1st series ended up in list moderation as there were too many on the Cc: list.

Can someone release it please?

Andrew

Re: Seeking advice on API return type inconsistency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]

On Wed, Oct 15, 2025 at 04:53:18PM +0100, Andrew Goodbody wrote:

> That 1st series ended up in list moderation as there were too many on the Cc: list.
> 
> Can someone release it please?

Done.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Seeking advice on API return type inconsistency

[Simon Glass]

Hi Michal,

On Wed, 13 Aug 2025 at 05:41, Michal Suchánek <msuchanek@suse.de> wrote:
>
> Hello,
>
> On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
> > 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?
>
> Presumably it returns -EBUSY which is then implicitly converted to a large
> positive value. For pointers there is IS_ERR macro to decode that, and
> for integers there is another corresponding macro (I don't recall the
> name).
>
> For memory this assumes that the last page that holds the 'error'
> pointers cannot be mapped, and there was already a bug in the kernel
> that this was not true.
>
> For clock rates this assumes that rates close to the maximum value are
> invalid which is not guaranteed in any way whatsoever.
>
> Consequently, this should not be used.
>
> 0 is the only clock rate guaranteed to be invalid.
>
> As far as I have seen in the u-boot code almost no caller checks the
> result, and those that do cannot do anything meaningful with it anyway,
> at most a message is printed. That can be done by the driver without
> passing around the error.
>
> > > > 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.
>
> I would suggest to abolish returning 'errors' as clock rates.

I don't think that is a good idea. Debugging drivers then becomes
quite complicated since there is no feedback and you must head into
the clock subsystem to see what is going on. Bring this into U-Boot
more generally and it will be very painful. At the moment at least you
can enable CONFIG_LOG_ERROR_RETURN and see what happened.

Better I think to create some new functions with the API you want
(e.g. panic on failure, ignore failure). Or rename clk_get_rate_int()
- for internal - and create a new clk_get_rate() that does what you
want / what Linux does.

Regards,
Simon

Re: Seeking advice on API return type inconsistency

[Michal Suchánek]

Hello,

On Fri, Sep 19, 2025 at 09:51:15AM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Wed, 13 Aug 2025 at 05:41, Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > Hello,
> >
> > On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
> > > 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?
> >
> > Presumably it returns -EBUSY which is then implicitly converted to a large
> > positive value. For pointers there is IS_ERR macro to decode that, and
> > for integers there is another corresponding macro (I don't recall the
> > name).
> >
> > For memory this assumes that the last page that holds the 'error'
> > pointers cannot be mapped, and there was already a bug in the kernel
> > that this was not true.
> >
> > For clock rates this assumes that rates close to the maximum value are
> > invalid which is not guaranteed in any way whatsoever.
> >
> > Consequently, this should not be used.
> >
> > 0 is the only clock rate guaranteed to be invalid.
> >
> > As far as I have seen in the u-boot code almost no caller checks the
> > result, and those that do cannot do anything meaningful with it anyway,
> > at most a message is printed. That can be done by the driver without
> > passing around the error.
> >
> > > > > 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.
> >
> > I would suggest to abolish returning 'errors' as clock rates.
> 
> I don't think that is a good idea. Debugging drivers then becomes
> quite complicated since there is no feedback and you must head into
> the clock subsystem to see what is going on. Bring this into U-Boot
> more generally and it will be very painful. At the moment at least you
> can enable CONFIG_LOG_ERROR_RETURN and see what happened.

Does that really work with unsigned return values containing negative
error values as large positive integers?

> Better I think to create some new functions with the API you want
> (e.g. panic on failure, ignore failure). Or rename clk_get_rate_int()
> - for internal - and create a new clk_get_rate() that does what you
> want / what Linux does.

The problem is that once an error is returned as 'clock rate' there is
really no way to determine if what you got is really a clock rate or an
error. You can assume that the values IS_ERR decodes as errors are not
clock rates but there is no guarantee that is the case. While IS_ERR
only uses small part of the avaiale range the valid values are not
spread evenly over the range, and hardware-specific.

Thanks

Michal