Messages in this thread Patch in this message | | | Date | Wed, 1 Jun 2022 16:15:59 +0530 | Subject | Re: [PATCH] tty: serial: qcom-geni-serial: minor fixes to get_clk_div_rate() | From | Vijaya Krishna Nivarthi <> |
| |
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@quicinc.com> wrote: >> Add missing initialisation and correct type casting >> >> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> >> --- >> 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/
| |