[PATCH] clk: fix clk_get_rate() documentation
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com --- include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..a833d6a27b 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,7 @@ int clk_free(struct clk *clk); * * @clk: A clock struct that was previously successfully requested by * clk_request/get_by_*(). - * @return clock rate in Hz, or -ve error code. + * @return clock rate in Hz on success, or 0 on error. */ ulong clk_get_rate(struct clk *clk);
On 2/10/21 12:37 PM, Giulio Benetti wrote:
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..a833d6a27b 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,7 @@ int clk_free(struct clk *clk);
- @clk: A clock struct that was previously successfully requested by
clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
*/ ulong clk_get_rate(struct clk *clk);
- @return clock rate in Hz on success, or 0 on error.
NAK. This function *does* return negative errors (see e.g. drivers/clk/clk-uclass.c). However, it may return 0 if passed an invalid clock (see clk_valid).
--Sean
On 2/13/21 12:25 AM, Sean Anderson wrote:
On 2/10/21 12:37 PM, Giulio Benetti wrote:
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..a833d6a27b 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,7 @@ int clk_free(struct clk *clk); * * @clk: A clock struct that was previously successfully requested by * clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
ulong clk_get_rate(struct clk *clk);
- @return clock rate in Hz on success, or 0 on error. */
NAK. This function *does* return negative errors (see e.g. drivers/clk/clk-uclass.c). However, it may return 0 if passed an invalid clock (see clk_valid).
Oops, I didn't dig enough, sorry. It must pass negative value to signal if get_rate() pointer.
Best regards
On Fri, 12 Feb 2021 at 18:17, Giulio Benetti giulio.benetti@benettiengineering.com wrote:
On 2/13/21 12:25 AM, Sean Anderson wrote:
On 2/10/21 12:37 PM, Giulio Benetti wrote:
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Simon, Sean,
Il giorno 13 feb 2021, alle ore 05:17, Simon Glass sjg@chromium.org ha scritto:
On Fri, 12 Feb 2021 at 18:17, Giulio Benetti giulio.benetti@benettiengineering.com wrote:
On 2/13/21 12:25 AM, Sean Anderson wrote: On 2/10/21 12:37 PM, Giulio Benetti wrote:
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Sean has already given me a NAK because this function actually return negative values through a ulong return types. I didn’t check into clk-uclass.c and there if get_rate() pointer is not found then it returns a negative value.
Since it’s a bit ambiguous I’ve tried to find a different approach but nothing that easy came into my mind.
Best regards Giulio Benetti
Hi Giulio,
On Sat, 13 Feb 2021 at 01:47, Giulio Benetti giulio.benetti@benettiengineering.com wrote:
Hi Simon, Sean,
Il giorno 13 feb 2021, alle ore 05:17, Simon Glass sjg@chromium.org ha scritto:
On Fri, 12 Feb 2021 at 18:17, Giulio Benetti giulio.benetti@benettiengineering.com wrote:
On 2/13/21 12:25 AM, Sean Anderson wrote: On 2/10/21 12:37 PM, Giulio Benetti wrote:
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Sean has already given me a NAK because this function actually return negative values through a ulong return types. I didn’t check into clk-uclass.c and there if get_rate() pointer is not found then it returns a negative value.
Yes, wrong email...see below.
Since it’s a bit ambiguous I’ve tried to find a different approach but nothing that easy came into my mind.
Something like this:
* @return clock rate in Hz, 0 for invalid clock, or -ve error code for other error
You can use Regards, Simon
On 2/13/21 7:24 PM, Simon Glass wrote:
Hi Giulio,
On Sat, 13 Feb 2021 at 01:47, Giulio Benetti giulio.benetti@benettiengineering.com wrote:
Hi Simon, Sean,
Il giorno 13 feb 2021, alle ore 05:17, Simon Glass sjg@chromium.org ha scritto:
On Fri, 12 Feb 2021 at 18:17, Giulio Benetti giulio.benetti@benettiengineering.com wrote:
On 2/13/21 12:25 AM, Sean Anderson wrote: On 2/10/21 12:37 PM, Giulio Benetti wrote:
clk_get_rate() can't and doesn't return -ve on error, it actually returns 0 on error or a value greater than 0 on success. So let's fix its documentation.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Sean has already given me a NAK because this function actually return negative values through a ulong return types. I didn’t check into clk-uclass.c and there if get_rate() pointer is not found then it returns a negative value.
Yes, wrong email...see below.
Since it’s a bit ambiguous I’ve tried to find a different approach but nothing that easy came into my mind.
Something like this:
- @return clock rate in Hz, 0 for invalid clock, or -ve error code for
other error
That's a good idea Simon, thank you, going to send patch for it soon.
Kind regards
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com --- V1->V2: * previous comment was wrong, this function returns negative value, so let's improve it's @return documentation as suggested by Simon Glass --- include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk); * * @clk: A clock struct that was previously successfully requested by * clk_request/get_by_*(). - * @return clock rate in Hz, or -ve error code. + * @return clock rate in Hz on success, 0 for invalid clock, or -ve error code + * for other errors. */ ulong clk_get_rate(struct clk *clk);
This looks good to me, and helps beginners like me. As for the function itself, i have 2 concerns: If it does return a negative value why is it unsigned, if it is in fact signed that a clock above 2.2Ghz is a negative number. As for the IS_ERR_VALUE macro there still is a chance that it will error if the clock just so happens to be 2^31 through 2^31 + number of err values. Just voicing my concerns i assume as i learn more about uboot, linux,rtos's and different programs there will be minor issues like this.
On Sat, Feb 13, 2021 at 9:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
V1->V2:
- previous comment was wrong, this function returns negative value, so
let's improve it's @return documentation as suggested by Simon Glass
include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk);
- @clk: A clock struct that was previously successfully requested
by
clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
- @return clock rate in Hz on success, 0 for invalid clock, or -ve error
code
*/
for other errors.ulong clk_get_rate(struct clk *clk);
-- 2.25.1
Hi Jesse,
Il giorno 14 feb 2021, alle ore 03:58, Jesse T mr.bossman075@gmail.com ha scritto:
This looks good to me, and helps beginners like me. As for the function itself, i have 2 concerns: If it does return a negative value why is it unsigned, if it is in fact signed that a clock above 2.2Ghz is a negative number.
I was worried too at first sight but if you try to check negative numbers you see that -1 is 0xFFFFFFFF so in the worst case you only loose 4095 numbers from the maximum, try to check with hex calculator. And that is the trick.
As for the IS_ERR_VALUE macro there still is a chance that it will error if the clock just so happens to be 2^31 through 2^31 + number of err values.
This is answered from above and IS_ERR_VALUE is a very contracted macro that basically let you to keep value NOT valid if (0 > value > 4095).
Just voicing my concerns i assume as i learn more about uboot, linux,rtos's and different programs there will be minor issues like this.
Sure, no problem :-)
Giulio
On Sat, Feb 13, 2021 at 9:17 PM Giulio Benetti giulio.benetti@benettiengineering.com wrote: Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
V1->V2:
- previous comment was wrong, this function returns negative value, so let's improve it's @return documentation as suggested by Simon Glass
include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk);
- @clk: A clock struct that was previously successfully requested by
clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
- @return clock rate in Hz on success, 0 for invalid clock, or -ve error code
*/
for other errors.ulong clk_get_rate(struct clk *clk);
-- 2.25.1
Awesome, thanks! I must have forgotten how twos complement works for a sec...
On Sat, Feb 13, 2021 at 10:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Hi Jesse,
Il giorno 14 feb 2021, alle ore 03:58, Jesse T mr.bossman075@gmail.com ha scritto:
This looks good to me, and helps beginners like me. As for the function itself, i have 2 concerns: If it does return a negative value why is it unsigned, if it is in fact signed that a clock above 2.2Ghz is a negative number.
I was worried too at first sight but if you try to check negative numbers you see that -1 is 0xFFFFFFFF so in the worst case you only loose 4095 numbers from the maximum, try to check with hex calculator. And that is the trick.
As for the IS_ERR_VALUE macro there still is a chance that it will error if the clock just so happens to be 2^31 through 2^31 + number of err values.
This is answered from above and IS_ERR_VALUE is a very contracted macro that basically let you to keep value NOT valid if (0 > value > 4095).
Just voicing my concerns i assume as i learn more about uboot, linux,rtos's and different programs there will be minor issues like this.
Sure, no problem :-)
Giulio
On Sat, Feb 13, 2021 at 9:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
V1->V2:
- previous comment was wrong, this function returns negative value, so
let's improve it's @return documentation as suggested by Simon Glass
include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk);
- @clk: A clock struct that was previously successfully requested
by
clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
- @return clock rate in Hz on success, 0 for invalid clock, or -ve
error code
*/
for other errors.ulong clk_get_rate(struct clk *clk);
-- 2.25.1
On 2/14/21 4:49 AM, Jesse T wrote:
Awesome, thanks! I must have forgotten how twos complement works for a sec...
On Sat, Feb 13, 2021 at 10:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Hi Jesse,
Il giorno 14 feb 2021, alle ore 03:58, Jesse T mr.bossman075@gmail.com ha scritto:
This looks good to me, and helps beginners like me. As for the function itself, i have 2 concerns: If it does return a negative value why is it unsigned, if it is in fact signed that a clock above 2.2Ghz is a negative number.
I was worried too at first sight but if you try to check negative numbers you see that -1 is 0xFFFFFFFF so in the worst case you only loose 4095 numbers from the maximum, try to check with hex calculator. And that is the trick.
As for the IS_ERR_VALUE macro there still is a chance that it will error if the clock just so happens to be 2^31 through 2^31 + number of err values.
This is answered from above and IS_ERR_VALUE is a very contracted macro that basically let you to keep value NOT valid if (0 > value > 4095).
Just voicing my concerns i assume as i learn more about uboot, linux,rtos's and different programs there will be minor issues like this.
Sure, no problem :-)
Giulio
On Sat, Feb 13, 2021 at 9:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
V1->V2:
- previous comment was wrong, this function returns negative value, so
let's improve it's @return documentation as suggested by Simon Glass
include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk);
- @clk: A clock struct that was previously successfully requested
by
clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
- @return clock rate in Hz on success, 0 for invalid clock, or -ve
error code
*/ ulong clk_get_rate(struct clk *clk);
for other errors.-- 2.25.1
Cc: Lukasz Majewski lukma@denx.de Lukasz is maintainer for CLOCK.
On 2/22/21 8:13 PM, Heinrich Schuchardt wrote:
On 2/14/21 4:49 AM, Jesse T wrote:
Awesome, thanks! I must have forgotten how twos complement works for a sec...
On Sat, Feb 13, 2021 at 10:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Hi Jesse,
Il giorno 14 feb 2021, alle ore 03:58, Jesse T mr.bossman075@gmail.com ha scritto:
This looks good to me, and helps beginners like me. As for the function itself, i have 2 concerns: If it does return a negative value why is it unsigned, if it is in fact signed that a clock above 2.2Ghz is a negative number.
I was worried too at first sight but if you try to check negative numbers you see that -1 is 0xFFFFFFFF so in the worst case you only loose 4095 numbers from the maximum, try to check with hex calculator. And that is the trick.
As for the IS_ERR_VALUE macro there still is a chance that it will error if the clock just so happens to be 2^31 through 2^31 + number of err values.
This is answered from above and IS_ERR_VALUE is a very contracted macro that basically let you to keep value NOT valid if (0 > value > 4095).
Just voicing my concerns i assume as i learn more about uboot, linux,rtos's and different programs there will be minor issues like this.
Sure, no problem :-)
Giulio
On Sat, Feb 13, 2021 at 9:17 PM Giulio Benetti < giulio.benetti@benettiengineering.com> wrote:
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
V1->V2:
- previous comment was wrong, this function returns negative value, so
let's improve it's @return documentation as suggested by Simon Glass
include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk); * * @clk: A clock struct that was previously successfully requested by * clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
- @return clock rate in Hz on success, 0 for invalid clock, or -ve
error code
ulong clk_get_rate(struct clk *clk);
for other errors.-- 2.25.1
Cc: Lukasz Majewski lukma@denx.de Lukasz is maintainer for CLOCK.
Ah thank you that's right. I've missed it because ./scripts/get_maintainer.pl didn't list him.
Hi Lukasz,
kindly ping
Best regards
On 2/13/21 9:17 PM, Giulio Benetti wrote:
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com
V1->V2:
- previous comment was wrong, this function returns negative value, so let's improve it's @return documentation as suggested by Simon Glass
include/clk.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index ca6b85fa6f..5a8c7244d0 100644 --- a/include/clk.h +++ b/include/clk.h @@ -344,7 +344,8 @@ int clk_free(struct clk *clk);
- @clk: A clock struct that was previously successfully requested by
clk_request/get_by_*().
- @return clock rate in Hz, or -ve error code.
- @return clock rate in Hz on success, 0 for invalid clock, or -ve error code
*/ ulong clk_get_rate(struct clk *clk);
for other errors.
Reviewed-by: Sean Anderson seanga2@gmail.com
On Sun, 14 Feb 2021 03:17:18 +0100, Giulio Benetti wrote:
Improve clk_get_rate() @return documentation that otherwise is a bit ambiguous. At the moment I expect to return 0 as error since the return type is 'ulong', instead the function really returns negative value in case the corresponding function pointer is null and returns 0 if the clock is invalid.
[...]
Applied, thanks!
[1/1] clk: fix clk_get_rate() documentation commit: 9e578f6340ba3f4324cd823872afe6eda39857d2
Best regards,
participants (5)
-
Giulio Benetti -
Heinrich Schuchardt -
Jesse T -
Sean Anderson -
Simon Glass