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: <20230420055131.2048959-1-joychakr@google.com> <20230420055131.2048959-6-joychakr@google.com>
 <20230421085354.34dwrgr3enlxqhtc@mobilestation>
In-Reply-To: <20230421085354.34dwrgr3enlxqhtc@mobilestation>
From:   Joy Chakraborty <joychakr@google.com>
Date:   Fri, 21 Apr 2023 14:51:58 +0530
Message-ID: <CAOSNQF1aK2EdgeUbNN4Bpp8hjPHTzBwt-q6+-Wb24VSsUOtSqA@mail.gmail.com>
Subject: Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
        manugautam@google.com, rohitner@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 21, 2023 at 2:23=E2=80=AFPM Serge Semin <fancer.lancer@gmail.co=
m> wrote:
>
> On Thu, Apr 20, 2023 at 05:51:31AM +0000, Joy Chakraborty wrote:
> > n_bytes variable in the driver represents the number of bytes per word
> > that needs to be sent/copied to fifo. Bits/word can be between 8 and 32
> > bits from the client but in memory they are a power of 2, same is menti=
oned
> > in spi.h header:
> > "
> >  * @bits_per_word: Data transfers involve one or more words; word sizes
> >  *    like eight or 12 bits are common.  In-memory wordsizes are
> >  *    powers of two bytes (e.g. 20 bit samples use 32 bits).
> >  *    This may be changed by the device's driver, or left at the
> >  *    default (0) indicating protocol words are eight bit bytes.
> >  *    The spi_transfer.bits_per_word can override this for each transfe=
r.
> > "
> >
> > Hence, round of n_bytes to a power of 2 to avoid values like 3 which
> > would generate unalligned/odd accesses to memory/fifo.
> >
> > Fixes: a51acc2400d4 ("spi: dw: Add support for 32-bits max xfer size")
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  drivers/spi/spi-dw-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > index c3bfb6c84cab..a6486db46c61 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -426,7 +426,7 @@ static int dw_spi_transfer_one(struct spi_controlle=
r *master,
> >       int ret;
> >
> >       dws->dma_mapped =3D 0;
>
> > -     dws->n_bytes =3D DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_B=
YTE);
> > +     dws->n_bytes =3D roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_p=
er_word, BITS_PER_BYTE));
>
> Almost 100 symbols looks too bulky. Moreover single-lined nested call
> makes things a bit harder to read. What about formatting it up like
> this?
>
>         dws->n_bytes =3D
>                 roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
>                                                 BITS_PER_BYTE));
>
>         or like this:
>
>         dws->n_bytes =3D roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_p=
er_word,
>                                                        BITS_PER_BYTE));
>
> Splitting the line into chunks will simplify the visual
> differentiation between the outer and inner calls.
>
> * Note even though the 80-char columns limit isn't that strict rule
> now, but it's still preferable unless exceeding the limit significantly
> increases readability. The update you suggest doesn't seem like the case
> which would improve the readability.
>

Sure, I can make the following change in the formatting and send the
patch series:
         dws->n_bytes =3D
                 roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
                                                 BITS_PER_BYTE));

Thanks
Joy
> -Serge(y)
>
> >       dws->tx =3D (void *)transfer->tx_buf;
> >       dws->tx_len =3D transfer->len / dws->n_bytes;
> >       dws->rx =3D transfer->rx_buf;
> > --
> > 2.40.0.634.g4ca3ef3211-goog
> >