[RFC PATCH] clk: fix clk_get_rate() always return ulong

[RFC PATCH] clk: fix clk_get_rate() always return ulong

[Julien Masson]

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;
 }
-- 
2.37.2

Re: [RFC PATCH] clk: fix clk_get_rate() always return ulong

[Sean Anderson]

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.

>   }
> 

Re: [RFC PATCH] clk: fix clk_get_rate() always return ulong

[Simon Glass]

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

Re: [RFC PATCH] clk: fix clk_get_rate() always return ulong

[Julien Masson]

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

-- 
Julien Masson