Hi Michal, On Wed, 13 Aug 2025 at 05:41, Michal Suchánek <msucha...@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
