Message ID | 87bks3wgzw.fsf@baylibre.com |
---|---|
State | RFC |
Delegated to: | Sean Anderson |
Headers | show |
Series | [RFC,v2] clk: fix clk_get_rate() always return ulong | expand |
Hi Julien, On Mon, 29 Aug 2022 at 06:06, Julien Masson <jmasson@baylibre.com> 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 | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> I would prefer to create a new log_rete() to handle this, with a long argument and return value. But this is OK, I suppose. > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index b89c77bf79..c351fa97d1 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)) > @@ -479,11 +479,7 @@ ulong clk_get_rate(struct clk *clk) > if (!ops->get_rate) > return -ENOSYS; > > - ret = ops->get_rate(clk); > - if (ret) > - return log_ret(ret); > - > - return 0; > + return ops->get_rate(clk); > } > > struct clk *clk_get_parent(struct clk *clk) > -- > 2.37.2 > Regards, Simon
Hi Simon, On Tue 30 Aug 2022 at 10:32, Simon Glass <sjg@chromium.org> wrote: > Hi Julien, > > On Mon, 29 Aug 2022 at 06:06, Julien Masson <jmasson@baylibre.com> 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 | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks for the review, I appreciate it. > > I would prefer to create a new log_rete() to handle this, with a long > argument and return value. But this is OK, I suppose. Yes and I guess that could be used in other functions in clk-uclass.c > >> >> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c >> index b89c77bf79..c351fa97d1 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)) >> @@ -479,11 +479,7 @@ ulong clk_get_rate(struct clk *clk) >> if (!ops->get_rate) >> return -ENOSYS; >> >> - ret = ops->get_rate(clk); >> - if (ret) >> - return log_ret(ret); >> - >> - return 0; >> + return ops->get_rate(clk); >> } >> >> struct clk *clk_get_parent(struct clk *clk) >> -- >> 2.37.2 >> > > Regards, > Simon
On 8/29/22 05:11, 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 | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index b89c77bf79..c351fa97d1 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)) > @@ -479,11 +479,7 @@ ulong clk_get_rate(struct clk *clk) > if (!ops->get_rate) > return -ENOSYS; > > - ret = ops->get_rate(clk); > - if (ret) > - return log_ret(ret); > - > - return 0; > + return ops->get_rate(clk); > } > > struct clk *clk_get_parent(struct clk *clk) Reviewed-by: Sean Anderson <seanga2@gmail.com>
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index b89c77bf79..c351fa97d1 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)) @@ -479,11 +479,7 @@ ulong clk_get_rate(struct clk *clk) if (!ops->get_rate) return -ENOSYS; - ret = ops->get_rate(clk); - if (ret) - return log_ret(ret); - - return 0; + return ops->get_rate(clk); } struct clk *clk_get_parent(struct clk *clk)
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 | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)