Re: Seeking advice on API return type inconsistency

[Tom Rini <trini@konsulko.com>]

[-- 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 --]