Re: [PATCH 08/24] clk: rockchip: Remove negative error returns from clk_get_rate

[Tom Rini <trini@konsulko.com>]

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

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

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