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

From: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Mark Brown <broonie@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <magnus.damm@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>
Subject: RE: [PATCH v2 3/5] spi: Add support for Renesas CSI
Date: Thu, 13 Jul 2023 22:35:08 +0000	[thread overview]
Message-ID: <TYWPR01MB8775E433ACFA2F78D357FF05C237A@TYWPR01MB8775.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAHp75VcJvRq7BotoODW_BOh84+TD_1Q3vbXSQv3FCiJfnBx8Vw@mail.gmail.com>

Hi Andy,

Thanks for your reply!

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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@renesas.com> 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)

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

> 
> ...
> 
> > > > +   /* Setup clock polarity and phase timing */
> > > > +   rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> > > > +                           !(spi->mode & SPI_CPOL));
> > > > +   rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> > > > +                           !(spi->mode & SPI_CPHA));
> > >
> > > Is it a must to do in a sequential writes?
> >
> > It's not a must, I'll combine those 2 statements into 1.
> 
> If so, you can use SPI_MODE_X_MASK.

That's the plan.

Thanks for your help Andy.

Cheers,
Fab

> 
> ...
> 
> > > > +   controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
> > >
> > > SPI_MODE_X_MASK
> >
> > This statement sets the mode_bits. Using a macro meant to be used as
> a
> > mask in this context is something I would want to avoid if possible.
> 
> Hmm... not a big deal, but I think that's what covers all mode_bits,
> and mode_bits by nature _is_ a mask.
> 
> --
> With Best Regards,
> Andy Shevchenko