[PATCH] usb: dwc3: core: Use IS_ERR_VALUE() for ref_clk rate check

[Marek Vasut]

On 2/10/26 1:52 PM, Jonas Karlman wrote:

[...]

> I think main issue is that U-Boot != Linux, and in U-Boot we can disable
> subsystems such as CLK, and the normal U-Boot way to signal that is to
> return -ENOSYS from functions, what clk_get_rate() does when disabled.

Sure, but if the function is called the same, people will make the 
assumption that the function return value handling is the same. So 
either the function has to be renamed and maybe a wrapper added that 
makes it linux-compatible, or simply make the function linux-compatible.

> Other clk_*() related functions also differs from U-Boot and Linux, some
> only exists in U-Boot, others behave differently.

Which is really not good.

> Last attempt at changing clk_get_rate() behavior in U-Boot I gave a NAK
> on the Rockchip driver side. I am not fully against changing behavior
> of clk_get_rate(), but the driver ops should continue to work the same.
> There is a difference between an unsupported and an invalid clock.
> 
> Something like below, and similar for other clk ops, could possible be
> fine if someone really want to align clk_*() funcs with Linux.
> 
> Back to this patch, I will send other patches to implement the missing/
> unsupported dwc3 ref clocks on Rockchip, this patch is still valid and
> matches current and documented behavior for clk_get_rate() in U-Boot
> and prevent lockups or crashes when ref clk is missing/unsupported.
> 
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 748359de287b..4603ecef82b5 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -485,6 +485,7 @@ int clk_request(struct udevice *dev, struct clk *clk)
>   ulong clk_get_rate(struct clk *clk)
>   {
>   	const struct clk_ops *ops;
> +	ulong rate;
>   
>   	debug("%s(clk=%p)\n", __func__, clk);
>   	if (!clk_valid(clk))
> @@ -492,9 +493,17 @@ ulong clk_get_rate(struct clk *clk)
>   	ops = clk_dev_ops(clk->dev);
>   
>   	if (!ops->get_rate)
> -		return -ENOSYS;
> +		rate = -ENOSYS;

This would have to be PTR_ERR() , right ?

> +	else
> +		rate = ops->get_rate(clk);
> +
> +	if (IS_ERR_VALUE(rate)) {
> +		dm_warn("clk_get_rate(clk=%p, id=%lu) failed: %ld\n",
> +			clk, clk->id, (long)rate);
> +		return 0;
> +	}
>   
> -	return ops->get_rate(clk);
> +	return rate;
>   }
>   
>   struct clk *clk_get_parent(struct clk *clk)
[...]