Re: [PATCH] tty: serial: qcom-geni-serial: minor fixes to get_clk_div_rate()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 6/1/2022 12:58 AM, Doug Anderson wrote:
Hi,

On Tue, May 31, 2022 at 11:18 AM Vijaya Krishna Nivarthi
<quic_vnivarth@xxxxxxxxxxx> wrote:
Add missing initialisation and correct type casting

Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@xxxxxxxxxxx>
---
  drivers/tty/serial/qcom_geni_serial.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 4733a23..08f3ad4 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -943,11 +943,11 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
  static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
                         unsigned int sampling_rate, unsigned int *clk_div)
  {
-       unsigned long ser_clk;
+       unsigned long ser_clk = 0;
In this patch it's not at all obvious why you'd need to init to 0. I
think the "for loop" is guaranteed to run at least once because
"max_div" is known at compile time. ...and currently each time through
the "for" loop you'll always set "ser_clk".

Ok, I realised we will never break out of for loop exceeding ULONG_MAX in 1st pass, so yes ser_clk will always be set.

I think in a future patch you'll want to _remove_ this from the for loop:

if (!prev)
   ser_clk = freq;

Intent is to save (and use) 1st freq if we cannot find an exact divider.

Isn't it ok?

For example please find debug output for a required frequency of 51.2MHz.

We try dividers 1, 2, 3 and end up with 52.1MHz the first result.

[   18.815432] 20220509 get_clk_div_rate desired_clk:51200000
[   18.821081] 20220509 get_clk_div_rate maxdiv:4095
[   18.825924] 20220509 get_clk_div_rate div:1
[   18.830239] 20220509 get_clk_div_rate freq:52174000
[   18.835288] 20220509 get_clk_div_rate div:2
[   18.839628] 20220509 get_clk_div_rate freq:100000000
[   18.844794] 20220509 get_clk_div_rate div:3
[   18.849119] 20220509 get_clk_div_rate freq:100000000
[   18.854254] 20220509 get_clk_div_rate reached max frequency breaking...
[   18.861072] 20220509 get_clk_div_rate clk_div=1, ser_clk=52174000

The behaviour was same earlier too when root_freq table was present.

The table did contain 51.2MHz and we would exit with same but on call to clk_set_rate(51.2MHz) we were ending up with 52.1MHz


...and _that's_ when you should init "ser_clk" to 0. Until then I'd
leave it as uninitialized...

Honestly, I'd throw all the fixes into one series, too.

My concern was if there would be a requirement to split the changes.

Will put in all in 1 series with Fixes tag.



         unsigned long desired_clk;
         unsigned long freq, prev;
         unsigned long div, maxdiv;
-       int64_t mult;
+       unsigned long long mult;

         desired_clk = baud * sampling_rate;
         if (!desired_clk) {
@@ -959,8 +959,8 @@ static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
         prev = 0;

         for (div = 1; div <= maxdiv; div++) {
-               mult = div * desired_clk;
-               if (mult > ULONG_MAX)
+               mult = (unsigned long long)div * (unsigned long long)desired_clk;
I think you only need to cast one of the two. The other will be
up-cast automatically.
Will change.


+               if (mult > (unsigned long long)ULONG_MAX)
I don't think you need this cast. As far as I know the C language will
"upcast" to the larger of the two types.
Will change.


-Doug

Thank you.

-Vijay/




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux