On Mon 29 Aug 2022 at 10:38, Simon Glass <s...@chromium.org> wrote:
> Hi, > > On Fri, 26 Aug 2022 at 08:00, Sean Anderson <sean...@gmail.com> wrote: >> >> On 8/26/22 6:31 AM, Julien Masson wrote: >>> According to clk_ops struct definition, the callback `get_rate()` >>> return current clock rate value as ulong. >>> `clk_get_rate()` should handle the clock rate returned as ulong also. >>> >>> Otherwise we may have invalid/truncated clock rate value returned by >>> `clk_get_rate()`. >>> >>> `log_ret` has also been removed since it use an `int` in the macro >>> definition. >>> >>> Signed-off-by: Julien Masson <jmas...@baylibre.com> >>> --- >>> drivers/clk/clk-uclass.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c >>> index b89c77bf79..446f7c49b8 100644 >>> --- a/drivers/clk/clk-uclass.c >>> +++ b/drivers/clk/clk-uclass.c >>> @@ -469,7 +469,7 @@ void clk_free(struct clk *clk) >>> ulong clk_get_rate(struct clk *clk) >>> { >>> const struct clk_ops *ops; >>> - int ret; >>> + ulong ret; >>> >>> debug("%s(clk=%p)\n", __func__, clk); >>> if (!clk_valid(clk)) >>> @@ -481,7 +481,7 @@ ulong clk_get_rate(struct clk *clk) >>> >>> ret = ops->get_rate(clk); >>> if (ret) >>> - return log_ret(ret); > > How about: > > if (IS_ERR(ret)) > return log_ret(ret) Maybe we can let the caller do this check ? This is how we do for these functions: clk_round_rate and clk_set_rate > >>> + return ret; >>> >>> return 0; >> >> This can just be return ret no if required. Yes correct I can directly return the callback, I will change that in v2. > > Yes, but it is nice to have the 'success' path clear and at the end. hummm it's not clear to me if return 0 is considered as 'success' path. I guess some drivers doesn't expect to get 0 from a clock ... As I suggested previously maybe we can let the caller handle the value returned by the callback ? (like clk_round_rate and clk_set_rate) > >> >>> } >>> > > Regards, > Simon -- Julien Masson