On Mon, Oct 20, 2025 at 01:03:15PM +0100, Andrew Goodbody wrote: > On 19/10/2025 21:28, Jonas Karlman wrote: > > Hi Andrew, > > > > On 10/15/2025 4:32 PM, Andrew Goodbody wrote: > > > clk_get_rate() returns a ulong so do not attempt to pass negative error > > > codes through it. > > > > On Rockchip the clk drivers only implement support for a subset of all > > clocks supported by the hardware, mostly to save space for TPL/SPL and > > not having to implement logic for clocks not really needed by U-Boot. > > > > Because support for all clocks is not implemented the existing working > > error handling actually help us catch clock issues on the Rockchip > > platform. E.g. when a clk is referenced by assigned-clock-rates in DT we > > get an error and can fix the issue, or decide if we can safely ignore > > setting the clk rate. > > > > I do not understand why you need to change the way the clk ops work, > > just check with IS_ERR_VALUE() inside the clk-uclass or similar if you > > really must remove working error handling from the callers. > > OK, so here we have a question and it is one that I wish had come up in the > original discussion. IS_ERR_VALUE() and other related macros are from > include/linux/err.h and are introduced with this comment > > /* > * Kernel pointers have redundant information, so we can use a > * scheme where we can return either an error code or a dentry > * pointer with the same return value. > * > * This should be a per-architecture thing, to allow different > * error and pointer decisions. > */ > > So they are clearly intended to be unambiguous and used to add information > about errors into kernel pointers. It also suggests that there may be other > implementations of these macros for other architectures. > > Much of the error checking of the return value from clk_get_rate() is done > with IS_ERR_VALUE() despite the return value not being a kernel pointer and > with this default implementation it does happen to work just as long as your > clock rate does not collide with an error. OK collisions are very unlikely > but it does still feel like an abuse of the macro to me. > > Is there any official guidance on this use of IS_ERR_VALUE()?
We don't have any, really. So my question is, given the use case Jonas outlined (thanks!), what do you see as a good way to handle this? -- Tom
signature.asc
Description: PGP signature
