Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Geert Uytterhoeven]

Hi Fabrizio,

On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro
<fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> >
> > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> > <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> >
> > ...
> >
> > > > > +#define CSI_CKS_MAX                0x3FFF
> > > >
> > > > If it's limited by number of bits, i would explicitly use that
> > information
> > > > as
> > > > (BIT(14) - 1).
> > >
> > > That value represents the register setting for the maximum clock
> > divider.
> > > The maximum divider and corresponding register setting are plainly
> > stated
> > > in the HW User Manual, therefore I would like to use either (plain)
> > value
> > > to make it easier for the reader.
> > >
> > > I think perhaps the below makes this clearer:
> > > #define CSI_CKS_MAX_DIV_RATIO   32766
> >
> > Hmm... To me it's a bit confusing now. Shouldn't it be 32767?
>
> 32766 is the correct value.
>
> Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
> serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
> means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).
>
> >
> > > #define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)
> >
> > Whatever you choose it would be better to add a comment to explain
> > this. Because the above is more clear to me with BIT(14)-1 if the
> > register field is 14-bit long.
> > With this value(s) I'm lost. Definitely needs a comment.
>
> To cater for a wider audience (and not just for those who have read the
> HW manual), I think perhaps the below would probably be the best compromise:
>
> /*
>  * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
>  * serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
>  * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).
>  */
> #define CSI_CKS_MAX             (BIT(14)-1)

Or GENMASK(13, 0)

As we have

    #define CSI_CLKSEL_CKS          GENMASK(14, 1)

and bit 0 must of the CLKSEL register must always be zero, the actual
divider is incidentally FIELD_GET(GENMASK(14, 0), clksel).
No idea if that can be useful to simplify the code, though ;-)

> > > static inline unsigned int x_trg(unsigned int words)
> > > {
> > >         return fls(words) - 1;
> > > }
> >
> > OK, but I think you can use it just inplace, no need to have such as a
> > standalone function.
>
> The above is actually equivalent to ilog2()
>
> >
> > > static inline unsigned int x_trg_words(unsigned int words)
> > > {
> > >         return 1 << x_trg(words);
> > > }
> >
> > Besides a better form of BIT(...) this looks to me like NIH
> > roundup_pow_of_two().
>
> rounddown_pow_of_two().
>
> I have tested the driver with s/x_trg/ilog2 and
> s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny bit of
> performance (probably down to the use of ternary operators in both macros)
> but I think it's okay, let's not reinvent the wheel and let's keep it more
> readable, I'll switch to using the above macros.

You mean this is not lost in the noise of the big loop in
rzv2m_csi_pio_transfer(), which is even waiting on an event?
I find that a bit surprising...

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