Hi Biju,
Thanks for your patch!
On Wed, Oct 4, 2023 at 8:42 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> The min_t() is often used as a shortcut for clamp(). Secondly, the
> BIT(16) - 1 is specifically used as the value related to the bits in the
> hardware and u16 is a software type that coincidentally has the same
> maximum as the above mentioned bitfield.
Technically it is two byte-sized registers forming a 16-bit field ;-)
> Replace min_t()->clamp() in vc3_pll_round_rate().
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> drivers/clk/clk-versaclock3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
> index 3d7de355f8f6..50772f61096f 100644
> --- a/drivers/clk/clk-versaclock3.c
> +++ b/drivers/clk/clk-versaclock3.c
> @@ -402,7 +402,7 @@ static long vc3_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> div_frc = rate % *parent_rate;
> div_frc *= BIT(16) - 1;
>
> - vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), U16_MAX);
> + vc3->div_frc = clamp(div64_ul(div_frc, *parent_rate), 0, BIT(16) - 1);
I'm not sure this is actually an improvement...
While I agree "BIT(16) - 1" matches the expression two lines above,
I find it harder to read.
Perhaps introducing a VC3_PLL2_FB_FRC_DIV_MAX definition may help.
BTW, if the hardware wouldn't use two byte-sized registers, but a real
bitifield, one could use FIELD_GET(mask, mask) instead.
Second, clamping an unsigned value to zero is futile, and opens us to
warnings like:
warning: comparison of unsigned expression in ‘>= 0’ is always
true [-Wtype-limits]
> rate = (*parent_rate *
> (vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16);
> } else {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
