[PATCH] clk: fix clk_get_rate() documentation

[PATCH] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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);
 
-- 
2.25.1

[PATCH] clk: fix clk_get_rate() documentation

[Sean Anderson]

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.
> + * @return clock rate in Hz on success, or 0 on error.
>    */
>   ulong clk_get_rate(struct clk *clk);
>   
> 

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

[PATCH] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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.
>> + * @return clock rate in Hz on success, or 0 on error.
>>     */
>>    ulong clk_get_rate(struct clk *clk);
>>    
>>
> 
> 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
-- 
Giulio Benetti
Benetti Engineering sas

[PATCH] clk: fix clk_get_rate() documentation

[Simon Glass]

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>

[PATCH] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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

[PATCH] clk: fix clk_get_rate() documentation

[Simon Glass]

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

[PATCH] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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
-- 
Giulio Benetti
Benetti Engineering sas

[PATCH v2] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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

[PATCH v2] clk: fix clk_get_rate() documentation

[Jesse T]

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
>
>

[PATCH v2] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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
>> 

[PATCH v2] clk: fix clk_get_rate() documentation

[Jesse T]

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 at 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
>>
>>

[PATCH v2] clk: fix clk_get_rate() documentation

[Heinrich Schuchardt]

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 at 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 at 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
>>>
>>>

Cc: Lukasz Majewski <lukma@denx.de>
Lukasz is maintainer for CLOCK.

[PATCH v2] clk: fix clk_get_rate() documentation

[Giulio Benetti]

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 at 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 at 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
>>>>
>>>>
> 
> 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.

-- 
Giulio Benetti
Benetti Engineering sas

[PATCH v2] clk: fix clk_get_rate() documentation

[Giulio Benetti]

Hi Lukasz,

kindly ping

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

On 2/23/21 1:25 AM, Giulio Benetti wrote:
> 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 at 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 at 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
>>>>>
>>>>>
>>
>> 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.
> 

Re: [PATCH v2] clk: fix clk_get_rate() documentation

[Sean Anderson]

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
> + *	   for other errors.
>    */
>   ulong clk_get_rate(struct clk *clk);
>   
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>

Re: [PATCH v2] clk: fix clk_get_rate() documentation

[Sean Anderson]

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,
-- 
Sean Anderson <seanga2@gmail.com>