| Message ID | 87a67rwav0.fsf@baylibre.com |
|---|---|
| State | RFC |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | [RFC] clk: fix clk_get_rate() always return ulong | expand |
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 <jmasson@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); > + return ret; > > return 0; This can just be return ret no if required. > } >
Hi, On Fri, 26 Aug 2022 at 08:00, Sean Anderson <seanga2@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 <jmasson@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) > > + return ret; > > > > return 0; > > This can just be return ret no if required. Yes, but it is nice to have the 'success' path clear and at the end. > > > } > > Regards, Simon
On Mon 29 Aug 2022 at 10:38, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Fri, 26 Aug 2022 at 08:00, Sean Anderson <seanga2@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 <jmasson@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
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); + return ret; return 0; }
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 <jmasson@baylibre.com> --- drivers/clk/clk-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)