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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

+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.

> 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?

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

-- 
With Best Regards,
Andy Shevchenko