RE: [PATCH] clk: vc3: Use clamp() instead of min_t()

[David Laight <David.Laight@ACULAB.COM>]

From: Andy Shevchenko
> Sent: 05 October 2023 10:07
> 
> +David
> 
> On Wed, Oct 04, 2023 at 09:50:09AM +0200, Geert Uytterhoeven wrote:
> > On Wed, Oct 4, 2023 at 8:42 AM Biju Das <biju.das.jz@bp.renesas.com> 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@linux.intel.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  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...
> 
> That's what Linus actually suggested to do.
> 
> > 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.
> 
> Either way, but U16_MAX is really semantically wrong here.

That code all looks completely horrid and strange.
I'd have thought the 16-bit fractional part (0..0xffff) would
basically be ((rate * 0x10000)/parent_rate) & 0xffff;
But that isn't what is being calculated.
(It may need tweaking to avoid the multiply overflowing.)
Then there is the multiply and divide by 0x10001 which is
equally strange.

But I'd just write 0x10000u and/or 0xffffu.

> 
> > 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]
> 
> David, is your series fix this as well?

It would let min() be used.
The compilers should really be less pedantic about tests for
unsigned being >= 0 - especially when there is an upper limit check.

I could make clamp() act as min() for unsigned with the low limit
is zero - but it would be rather over-complicated.

	David

> 
> > >                 rate = (*parent_rate *
> > >                         (vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16);
> > >         } else {
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)