Re: [PATCH v5 2/4] clk: vc3: Fix 64 by 64 division

[Andy Shevchenko <andy@black.fi.intel.com>]

On Thu, Aug 24, 2023 at 11:48:10AM +0100, Biju Das wrote:
> Fix the below cocci warnings by replacing do_div()->div64_ul() and
> bound the result with a max value of U16_MAX.
> 
> cocci warnings:
> 	drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a
> 	64-by-32 division, please consider using div64_ul instead.

It's nice, but there is a room for a couple of improvements. See below.

...

>  		/* Determine best fractional part, which is 16 bit wide */
>  		div_frc = rate % *parent_rate;
>  		div_frc *= BIT(16) - 1;
> -		do_div(div_frc, *parent_rate);
>  
> -		vc3->div_frc = (u32)div_frc;
> +		vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), U16_MAX);

First of all, as Linus Torvalds pointed out [1] min_t() is often used as
a shortcut for clamp(). Second one, the BIT(16) - 1 is specifically used
as the value related to the bits in the hardware and u16 is a software
type that coincidentially has the same maximum as the above mentioned
bitfield.

That said, here this should be clamped to the [0 .. BIT(16) - 1] range.

Since the patch is applied perhaps you can cook a followup.

To everyone the message is simple: try to not use typed version of min()
and clamp() at all.

>  		rate = (*parent_rate *
> -			(vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16);
> +			(vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16);

[1]: https://lore.kernel.org/lkml/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/

-- 
With Best Regards,
Andy Shevchenko