Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Jon Hunter <jonathanh@nvidia.com>]

Hi Satya, Vladimir,

On 13/08/2024 21:01, Vladimir Zapolskiy wrote:
> On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
>> In zonda_pll_adjust_l_val() replace the divide operator with comparison
>> operator since comparisons are faster than divisions.
>>
>> Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for 
>> Zonda PLL")
> 
> Apparently the change is not a fix, therefore I believe the Fixes tag
> shall be removed.


 From the commit message it is not clear that this is a fix, but I
believe that it is. With the current -next I am seeing the following
build error (with GCC 7.3.1) on ARM ...

drivers/clk/qcom/clk-alpha-pll.o: In function `clk_zonda_pll_set_rate':
clk-alpha-pll.c:(.text+0x45dc): undefined reference to `__aeabi_uldivmod'
  
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/

There is also the above smatch warning that was reported.

>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>> ---
>>   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c 
>> b/drivers/clk/qcom/clk-alpha-pll.c
>> index 2f620ccb41cb..fd8a82bb3690 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned long 
>> rate, unsigned long prate, u32
>>       remainder = do_div(quotient, prate);
>>       *l = quotient;
> 
> Since it's not a fix, but a simplification, you may wish to remove
> an unnecessary 'quotient' local variable:
> 
> remainder = do_div(rate, prate);
> 
>> -    if ((remainder * 2) / prate)
>> +    if ((remainder * 2) >= prate)
>>           *l = *l + 1;
> 
> *l = rate + (u32)(remainder * 2 >= prate);


The above change does fix this build error for me.

Satya, did you intend this to be a fix? Can we get this into -next?

Thanks
Jon

-- 
nvpublic