Re: [PATCH 1/2] spi: Add MXIC controller driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Boris,

On Fri, Aug 3, 2018 at 9:48 AM Boris Brezillon
<boris.brezillon@xxxxxxxxxxx> wrote:
> On Thu, 2 Aug 2018 19:27:45 +0000
> Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
> > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@xxxxxxxxxxx wrote:
> > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));
> >
> > Can also be written as freq*9/25000000.
>
> I'd like to keep 360 here. The phase is in degree, hence the 360. The
> compiler should be smart enough to optimize that at compilation
> time ;-).

It may have a hard time optimizing that, while preserving the semantics
of the original expression (rounding behavior!)...

Speaking about that, there's a serious issue due to loss of accuracy when
doing two divisions like that. "360 * freq / 1000000000" is better, assumed
the multiplication will be done in 64-bit arithmetic to avoid overflow:

    u64 t = 360ULL * freq;
    ret = clk_set_phase(mxic->send_dly_clk, do_div(t, 1000000000));

BTW, can freq < 1000000000 / 360?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html