Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230622113341.657842-1-fabrizio.castro.jz@renesas.com>
 <20230622113341.657842-4-fabrizio.castro.jz@renesas.com> <ZKVI4XPbPXfzQa9J@surfacebook>
 <TYWPR01MB8775144ADA4B4FF2CB9B940BC237A@TYWPR01MB8775.jpnprd01.prod.outlook.com>
In-Reply-To: <TYWPR01MB8775144ADA4B4FF2CB9B940BC237A@TYWPR01MB8775.jpnprd01.prod.outlook.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 13 Jul 2023 19:37:20 +0300
Message-ID: <CAHp75VcJvRq7BotoODW_BOh84+TD_1Q3vbXSQv3FCiJfnBx8Vw@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
To:     Fabrizio Castro <fabrizio.castro.jz@renesas.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jul 13, 2023 at 6:52=E2=80=AFPM 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 informat=
ion
> > 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?

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

...

> > > +static const unsigned char x_trg[] =3D {
> > > +   0, 1, 1, 2, 2, 2, 2, 3,
> > > +   3, 3, 3, 3, 3, 3, 3, 4,
> > > +   4, 4, 4, 4, 4, 4, 4, 4,
> > > +   4, 4, 4, 4, 4, 4, 4, 5
> > > +};
> > > +
> > > +static const unsigned char x_trg_words[] =3D {
> > > +   1,  2,  2,  4,  4,  4,  4,  8,
> > > +   8,  8,  8,  8,  8,  8,  8,  16,
> > > +   16, 16, 16, 16, 16, 16, 16, 16,
> > > +   16, 16, 16, 16, 16, 16, 16, 32
> > > +};
> >
> > Why do you need tables? At the first glance these values can be easily
> > calculated from indexes.
>
> I think I am going to replace those tables with the below (and of course
> adjust the callers accordingly since the argument is not an index anymore=
):
>
> 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.

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

...

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

...

> > > +   controller->mode_bits =3D 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.

--=20
With Best Regards,
Andy Shevchenko